UAF in sctp_iterator_inp_being_freed

RESOLVED FIXED in Firefox 49

Status

()

defect
P1
normal
Rank:
10
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-uaf, sec-high})

51 Branch
mozilla51
Points:
---
Bug Flags:
sec-bounty -
qe-verify -

Firefox Tracking Flags

(firefox48 wontfix, firefox49+ fixed, firefox-esr4549+ fixed, firefox50+ fixed, firefox51+ fixed)

Details

(Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
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

3 years ago
Posted file asan_output.txt
Assignee

Comment 2

3 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

3 years ago
I think the fix is to unset |sctp_it_ctl.cur_it| in |sctp_iterator_work| before we unlock.
Assignee

Updated

3 years ago
Version: 48 Branch → 51 Branch
Group: core-security → media-core-security
Assignee

Comment 4

3 years ago
Posted patch Zero out a pointer (obsolete) — Splinter Review
MozReview-Commit-ID: BGRFposcrBG
Assignee

Updated

3 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Component: WebRTC: Networking → Networking
Assignee

Updated

3 years ago
Attachment #8779362 - Flags: review?(jduell.mcbugs)
+tuexen for upstream notification about this issue
Rank: 10
Priority: -- → P1
Attachment #8779362 - Flags: review?(tuexen)

Comment 6

3 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

3 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

3 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

3 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

3 years ago
And while we're at it, it probably makes sense to move the setting of cur_it.
Assignee

Updated

3 years ago
Attachment #8779362 - Attachment is obsolete: true
Attachment #8779362 - Flags: review?(tuexen)
Attachment #8779362 - Flags: review?(jduell.mcbugs)
Assignee

Updated

3 years ago
Attachment #8780136 - Flags: review?(tuexen)
Attachment #8780136 - Flags: review?(jduell.mcbugs)

Comment 12

3 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

3 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

3 years ago
Of course.
Attachment #8780136 - Flags: review?(jduell.mcbugs)
Assignee

Comment 15

3 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

3 years ago
Attachment #8780136 - Flags: review+
Note: since the patch is committed upstream, I think we should not delay landing this.
Sec-approval+. We'll want branch patches, including ESR45, made and nominated.
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+
https://hg.mozilla.org/mozilla-central/rev/f4cd5568389b

Looks like this is missing an esr45 nomination still.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee

Comment 23

3 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?
Attachment #8780136 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main49+][adv-esr45.4+]
Duplicate of this bug: 1300716

Comment 26

2 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)
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?
Group: core-security-release
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.