Closed
Bug 1293347
Opened 8 years ago
Closed 8 years ago
UAF in sctp_iterator_inp_being_freed
Categories
(Core :: Networking, defect, P1)
Tracking
()
People
(Reporter: bwc, Assigned: bwc)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main49+][adv-esr45.4+])
Attachments
(2 files, 1 obsolete file)
34.45 KB,
text/plain
|
Details | |
2.55 KB,
patch
|
tuexen
:
review+
jduell.mcbugs
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
When running the test case from bug 1291244 on linux ASAN, I observed a UAF crash. At first glance it looks like a threadsafety problem.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
The problem is in |sctp_iterator_worker|: sctp_iterator_work(it); sctp_it_ctl.cur_it = NULL; |sctp_iterator_work| can destroy |sctp_it_ctl.cur_it| (aka |it|), but no lock is held by |sctp_iterator_worker|. This means that another thread can attempt to use |sctp_it_ctl.cur_it| which is now a dangling pointer. It is a super tight race though.
Assignee | ||
Comment 3•8 years ago
|
||
I think the fix is to unset |sctp_it_ctl.cur_it| in |sctp_iterator_work| before we unlock.
Assignee | ||
Updated•8 years ago
|
Version: 48 Branch → 51 Branch
Updated•8 years ago
|
Group: core-security → media-core-security
Assignee | ||
Comment 4•8 years ago
|
||
MozReview-Commit-ID: BGRFposcrBG
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Component: WebRTC: Networking → Networking
Assignee | ||
Updated•8 years ago
|
Attachment #8779362 -
Flags: review?(jduell.mcbugs)
Updated•8 years ago
|
Attachment #8779362 -
Flags: review?(tuexen)
Comment 6•8 years ago
|
||
What about: diff --git a/netinet/sctputil.c b/netinet/sctputil.c index eedc4ab..157b881 100755 --- a/netinet/sctputil.c +++ b/netinet/sctputil.c @@ -1518,11 +1518,11 @@ sctp_iterator_worker(void) CURVNET_SET(it->vn); #endif sctp_iterator_work(it); - sctp_it_ctl.cur_it = NULL; #if defined(__FreeBSD__) && __FreeBSD_version >= 801000 CURVNET_RESTORE(); #endif SCTP_IPI_ITERATOR_WQ_LOCK(); + sctp_it_ctl.cur_it = NULL; #if !defined(__FreeBSD__) if (sctp_it_ctl.iterator_flags & SCTP_ITERATOR_MUST_EXIT) { break;
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Michael Tüxen from comment #6) > What about: > > diff --git a/netinet/sctputil.c b/netinet/sctputil.c > index eedc4ab..157b881 100755 > --- a/netinet/sctputil.c > +++ b/netinet/sctputil.c > @@ -1518,11 +1518,11 @@ sctp_iterator_worker(void) > CURVNET_SET(it->vn); > #endif > sctp_iterator_work(it); > - sctp_it_ctl.cur_it = NULL; > #if defined(__FreeBSD__) && __FreeBSD_version >= 801000 > CURVNET_RESTORE(); > #endif > SCTP_IPI_ITERATOR_WQ_LOCK(); > + sctp_it_ctl.cur_it = NULL; > #if !defined(__FreeBSD__) > if (sctp_it_ctl.iterator_flags & SCTP_ITERATOR_MUST_EXIT) { > break; If anything that'll make the problem worse, as |sctp_it_ctl.cur_it| is a dangling pointer for longer.
Comment 8•8 years ago
|
||
Got it, you are right. I agree, setting it to NULL where you do it makes sense. Shouldn't it be only cleared there? So what about: diff --git a/src/netinet/sctputil.c b/src/netinet/sctputil.c index eedc4ab..a938088 100755 --- a/src/netinet/sctputil.c +++ b/src/netinet/sctputil.c @@ -1363,6 +1363,7 @@ sctp_iterator_work(struct sctp_iterator *it) if (it->inp == NULL) { /* iterator is complete */ done_with_iterator: + sctp_it_ctl.cur_it = NULL; SCTP_ITERATOR_UNLOCK(); SCTP_INP_INFO_RUNLOCK(); if (it->function_atend != NULL) { @@ -1518,7 +1519,6 @@ sctp_iterator_worker(void) CURVNET_SET(it->vn); #endif sctp_iterator_work(it); - sctp_it_ctl.cur_it = NULL; #if defined(__FreeBSD__) && __FreeBSD_version >= 801000 CURVNET_RESTORE(); #endif
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Michael Tüxen from comment #8) > Got it, you are right. I agree, setting it to NULL where you do it makes > sense. > Shouldn't it be only cleared there? So what about: > diff --git a/src/netinet/sctputil.c b/src/netinet/sctputil.c > index eedc4ab..a938088 100755 > --- a/src/netinet/sctputil.c > +++ b/src/netinet/sctputil.c > @@ -1363,6 +1363,7 @@ sctp_iterator_work(struct sctp_iterator *it) > if (it->inp == NULL) { > /* iterator is complete */ > done_with_iterator: > + sctp_it_ctl.cur_it = NULL; > SCTP_ITERATOR_UNLOCK(); > SCTP_INP_INFO_RUNLOCK(); > if (it->function_atend != NULL) { > @@ -1518,7 +1519,6 @@ sctp_iterator_worker(void) > CURVNET_SET(it->vn); > #endif > sctp_iterator_work(it); > - sctp_it_ctl.cur_it = NULL; > #if defined(__FreeBSD__) && __FreeBSD_version >= 801000 > CURVNET_RESTORE(); > #endif Ah, done_with_iterator is the only exit from that function, so yeah this works. Let me update.
Assignee | ||
Comment 10•8 years ago
|
||
And while we're at it, it probably makes sense to move the setting of cur_it.
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93cfd86ddf03
Assignee | ||
Updated•8 years ago
|
Attachment #8779362 -
Attachment is obsolete: true
Attachment #8779362 -
Flags: review?(tuexen)
Attachment #8779362 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8780136 -
Flags: review?(tuexen)
Attachment #8780136 -
Flags: review?(jduell.mcbugs)
Comment 12•8 years ago
|
||
Comment on attachment 8780136 [details] [diff] [review] Move the set/unset of |cur_it| to a better place The whitespace change in sctp_sorecvmsg() is not related. Other than that, I'm fine with the patch.
Attachment #8780136 -
Flags: review?(tuexen) → review+
Comment 13•8 years ago
|
||
BTW: Can I put a corresponding patch in public repos (usrsctp, FreeBSD) giving "Byron Campen" credit? (I'm asking since this report is closed.).
Assignee | ||
Comment 14•8 years ago
|
||
Of course.
Updated•8 years ago
|
Attachment #8780136 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8780136 [details] [diff] [review] Move the set/unset of |cur_it| to a better place Approval Request Comment [Feature/regressing bug #]: Bug 729511 [User impact if declined]: Possible UAF crashes. [Describe test coverage new/current, TreeHerder]: This is very hard to repro, even on ASAN. [Risks and why]: Extremely low, just moving a set/unset under the protection of a lock that is already being acquired. [String/UUID change made/needed]: None. [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably pretty tough. Catching this required lots of tries on ASAN, since the window is so narrow. It also doesn't show up on crash-stats. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? A careful observer could work out that we were moving modifications to |cur_it| somewhere it would be protected by a lock, and dxr would make it fairly easy to identify places where |cur_it| might be accessed. Which older supported branches are affected by this flaw? All. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I expect the patch to apply cleanly. How likely is this patch to cause regressions; how much testing does it need? Pretty unlikely.
Attachment #8780136 -
Flags: sec-approval?
Attachment #8780136 -
Flags: approval-mozilla-beta?
Attachment #8780136 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Attachment #8780136 -
Flags: review+
Comment 16•8 years ago
|
||
Committed upstream: https://github.com/sctplab/usrsctp/commit/fb26455c2d56cd5f74c891c586b9ee2398188b8a
Comment 17•8 years ago
|
||
Note: since the patch is committed upstream, I think we should not delay landing this.
Comment 18•8 years ago
|
||
Sec-approval+. We'll want branch patches, including ESR45, made and nominated.
status-firefox48:
--- → wontfix
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox49:
--- → +
tracking-firefox50:
--- → +
tracking-firefox51:
--- → +
tracking-firefox-esr45:
--- → 49+
Comment 19•8 years ago
|
||
Comment on attachment 8780136 [details] [diff] [review] Move the set/unset of |cur_it| to a better place Ah, I see the branch approvals now. I'll plus them. We will still need ESR45.
Attachment #8780136 -
Flags: sec-approval?
Attachment #8780136 -
Flags: sec-approval+
Attachment #8780136 -
Flags: approval-mozilla-beta?
Attachment #8780136 -
Flags: approval-mozilla-beta+
Attachment #8780136 -
Flags: approval-mozilla-aurora?
Attachment #8780136 -
Flags: approval-mozilla-aurora+
Comment 21•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2842fbb9b28 https://hg.mozilla.org/releases/mozilla-beta/rev/f3e74826733d
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f4cd5568389b Looks like this is missing an esr45 nomination still.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8780136 [details] [diff] [review] Move the set/unset of |cur_it| to a better place Sure, why not. See other approval requests.
Attachment #8780136 -
Flags: approval-mozilla-esr45?
Updated•8 years ago
|
Attachment #8780136 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify-
Updated•8 years ago
|
Whiteboard: [adv-main49+][adv-esr45.4+]
Comment 26•7 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #0) > When running the test case from bug 1291244 on linux ASAN, I observed a UAF > crash. At first glance it looks like a threadsafety problem. Looks like there is some miscommunication here. Someone used my test case to open a new bug and then you guys marked my bug as duplicate: https://bugzilla.mozilla.org/show_bug.cgi?id=1300716#c22 Would you consider a bounty for it?
Flags: needinfo?(dveditz)
Comment 27•7 years ago
|
||
I nominated this bug for a bounty to trigger a re-evaluation, but I suspect that this will be considered part of the bug 1291244 investigation for which you were already awarded a bounty (the "someone" who opened this bug was the guy fixing that one).
Flags: needinfo?(dveditz) → sec-bounty?
Updated•7 years ago
|
Group: core-security-release
Comment 28•7 years ago
|
||
The bounty committee agrees: this fix was part of the due dilligence from fixing bug 1291244, split into a separate bug to keep the comment history sane.
Flags: sec-bounty? → sec-bounty-
You need to log in
before you can comment on or make changes to this bug.
Description
•