Closed Bug 1293347 Opened 3 years ago Closed 3 years ago

UAF in sctp_iterator_inp_being_freed

Categories

(Core :: Networking, defect, P1)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 49+ fixed
firefox50 + fixed
firefox51 + fixed

People

(Reporter: bwc, Assigned: bwc)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(2 files, 1 obsolete file)

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.
Attached file asan_output.txt
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.
I think the fix is to unset |sctp_it_ctl.cur_it| in |sctp_iterator_work| before we unlock.
Version: 48 Branch → 51 Branch
Group: core-security → media-core-security
Attached patch Zero out a pointer (obsolete) — Splinter Review
MozReview-Commit-ID: BGRFposcrBG
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Component: WebRTC: Networking → Networking
Attachment #8779362 - Flags: review?(jduell.mcbugs)
+tuexen for upstream notification about this issue
Rank: 10
Priority: -- → P1
Attachment #8779362 - Flags: review?(tuexen)
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;
(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.
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
(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.
And while we're at it, it probably makes sense to move the setting of cur_it.
Attachment #8779362 - Attachment is obsolete: true
Attachment #8779362 - Flags: review?(tuexen)
Attachment #8779362 - Flags: review?(jduell.mcbugs)
Attachment #8780136 - Flags: review?(tuexen)
Attachment #8780136 - Flags: review?(jduell.mcbugs)
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+
BTW: Can I put a corresponding patch in public repos (usrsctp, FreeBSD) giving "Byron Campen" credit?
(I'm asking since this report is closed.).
Of course.
Attachment #8780136 - Flags: review?(jduell.mcbugs)
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?
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
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
(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.