Byron Campen [:bwc] Assignee
	Description • 8 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.

	Byron Campen [:bwc] 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.

	Byron Campen [:bwc] 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.

	Byron Campen [:bwc] Assignee
	Comment 4 • 8 years ago

MozReview-Commit-ID: BGRFposcrBG

	Randell Jesup [:jesup] (needinfo me)
	Comment 5 • 8 years ago

+tuexen for upstream notification about this issue

	Michael Tüxen
	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;

	Byron Campen [:bwc] 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.

	Michael Tüxen
	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

	Byron Campen [:bwc] 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.

	Byron Campen [:bwc] Assignee
	Comment 10 • 8 years ago

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

	Byron Campen [:bwc] Assignee
	Comment 11 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93cfd86ddf03

	Michael Tüxen
	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.

	Michael Tüxen
	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.).

	Byron Campen [:bwc] Assignee
	Comment 14 • 8 years ago

Of course.

	Byron Campen [:bwc] 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.

	Michael Tüxen
	Comment 16 • 8 years ago

Committed upstream:
https://github.com/sctplab/usrsctp/commit/fb26455c2d56cd5f74c891c586b9ee2398188b8a

	Randell Jesup [:jesup] (needinfo me)
	Comment 17 • 8 years ago

Note: since the patch is committed upstream, I think we should not delay landing this.

	Al Billings [:abillings - ex-MoCo]
	Comment 18 • 8 years ago

Sec-approval+. We'll want branch patches, including ESR45, made and nominated.

	Al Billings [:abillings - ex-MoCo]
	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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 20 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4cd5568389b

	Randell Jesup [:jesup] (needinfo me)
	Comment 21 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2842fbb9b28
https://hg.mozilla.org/releases/mozilla-beta/rev/f3e74826733d

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/f4cd5568389b

Looks like this is missing an esr45 nomination still.

	Byron Campen [:bwc] 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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 8 years ago

https://hg.mozilla.org/releases/mozilla-esr45/rev/3e37ba5e0867

	Looben Yang
	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?

	Daniel Veditz [: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).

	Daniel Veditz [:dveditz]
	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.