Bug 12560 - Stack address is returned from function epoll_check_reopen
Summary: Stack address is returned from function epoll_check_reopen
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.5.5
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-04 05:56 UTC by shqking
Modified: 2017-02-14 03:28 UTC (History)
1 user (show)

See Also:


Attachments
a possible patch for this undefined behavior (291 bytes, patch)
2017-02-04 05:56 UTC, shqking
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description shqking 2017-02-04 05:56:11 UTC
Created attachment 12905 [details]
a possible patch for this undefined behavior

According to the C Standard, 6.2.4 [ISO/IEC 9899:2011] (https://www.securecoding.cert.org/confluence/display/c/DCL30-C.+Declare+objects+with+appropriate+storage+durations), the address of local variables escaping through output parameters is one kind of undefined behaviors, and can lead to an exploitable vulnerability.

The vulnerable function is in "lib/tevent/tevent_epoll.c".
The code snippet is as following.

static void epoll_check_reopen(struct epoll_event_context *epoll_ev)
{
   ...
   bool panic_triggered = false;
   ...
   epoll_ev->panic_state = &panic_triggered;
   for (fde=epoll_ev->ev->fd_events;fde;fde=fde->next) {
      fde->additional_flags &= ~EPOLL_ADDITIONAL_FD_FLAG_HAS_EVENT;
      epoll_update_event(epoll_ev, fde);

      if (panic_triggered) {
         if (caller_panic_state != NULL) {
            *caller_panic_state = true;
         }
         return;
   ...
}

A stack address, i.e. the address of local variable "panic_triggered" is returned through the output parameter "epoll_ev" as a side effect.

We have examined all callers of "epoll_check_reopen" and found that they do NOT access "epoll_ev->panic_state" any more.

Even so, as we see it, this issue seems like a time bomb and we'd better fix it to improve the software quality.

Attached is a possible patch.
Note that this issue is reported by shqking and Zhenwei Zou together.

Thanks.
Comment 1 Jeremy Allison 2017-02-10 22:41:10 UTC
That patch looks correct to me. It is defensive programming, rather than a real bug but is certainly a good fix to make.

I'll get it pushed to master.

Thanks,

Jeremy.
Comment 2 Stefan Metzmacher 2017-02-11 11:55:25 UTC
(In reply to Jeremy Allison from comment #1)

I don't think the patch is correct,
we should not touch epoll_ev anymore if panic_triggered is true,
it means the world has changed (and epoll_ev may not exist anymore)
and we have to return immediately.
Comment 3 Jeremy Allison 2017-02-13 19:21:41 UTC
Yeah, I didn't prepare for master yet as when I looked closer, I wasn't sure of my initial accessment :-). I'll take a more complete look soon..
Comment 4 shqking 2017-02-14 03:28:56 UTC
(In reply to Jeremy Allison from comment #3)

Variable panic_triggered is set as true through dereferring epoll_ev->panic_state at function epoll_panic in the case when something get wrong.

We agree with Stefan Metzmacher's point. If the program will never access epoll_ev anymore after panic_triggered is true, this patch seems dispensable.

Even so, we still believe that the patch we proposed is just kind of defensive programming, making everything to be on the safe side. It would be fine if this patch is not accepted. It's all up to you. :)

Thanks.