Closed Bug 1209546 (CVE-2016-1978) Opened 4 years ago Closed 4 years ago

Potential UAF in ssl3_HandleECDHServerKeyExchange

Categories

(NSS :: Libraries, defect, P1, major)

3.18

Tracking

(firefox43 wontfix, firefox44+ fixed, firefox45+ fixed, firefox-esr3846+ fixed)

RESOLVED FIXED
Tracking Status
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox-esr38 46+ fixed

People

(Reporter: ekr, Assigned: ttaubert)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main44+][adv-esr38.8+])

Attachments

(2 files, 2 obsolete files)

If you look at the code, we have:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ecc.c#707
    ss->sec.peerKey = peerKey = PORT_ArenaZNew(arena, SECKEYPublicKey);
    if (peerKey == NULL) {
        goto no_memory;
    }

    peerKey->arena                 = arena;
    peerKey->keyType               = ecKey;

And then later:

   /* copy publicValue in peerKey */
    if (SECITEM_CopyItem(arena, &peerKey->u.ec.publicValue,  &ec_point))
    {
        PORT_FreeArena(arena, PR_FALSE);
        goto no_memory;
    }

The problem is that that ss->sec.peerKey is allocated out of arena and
has a pointer to arena. So if we then destroy it in:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsecur.c#1038

This looks like a potential UAF (following an OOM)
Keywords: sec-high
Attached patch bug1209546.patch (obsolete) — Splinter Review
I think that this is the right fix for the problem.  I've fixed it for DHE and ECDHE both.
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
Attachment #8667419 - Flags: review?(ekr)
Comment on attachment 8667419 [details] [diff] [review]
bug1209546.patch

Review of attachment 8667419 [details] [diff] [review]:
-----------------------------------------------------------------

Martin: you fixed the use-after-free, but didn't fix theleak of |arena|
on error returns.

::: lib/ssl/ssl3con.c
@@ +7032,2 @@
>      	if (peerKey == NULL) {
>  	    goto no_memory;

We need to free |arena| before going to no_memory. See line 6920
of this file.

::: lib/ssl/ssl3ecc.c
@@ +703,5 @@
>      if (arena == NULL) {
>          goto no_memory;
>      }
>  
> +    peerKey = PORT_ArenaZNew(arena, SECKEYPublicKey);

Martin: we need to make sure |arena| is freed before we
return on error. Right now we only do that here:

 725     /* copy publicValue in peerKey */
 726     if (SECITEM_CopyItem(arena, &peerKey->u.ec.publicValue,  &ec_point))
 727     {
 728         PORT_FreeArena(arena, PR_FALSE);
 729         goto no_memory;
 730     }

It would be better to do that right before the return SECFailure statements
under the alert_loser, loser, and no_memory labels. Or we can imitate how
it is done in ssl3con.c.
Attachment #8667419 - Flags: review-
Ack, I think that Tim knows what to do and can do so.
Ok, so that's pretty much just the patch from bug 1209152, applied to ssl3con.c as well. That should fix the arena leaks and assigns |ss.sec.peerKey| only when we constructed |peerKey| completely, thus no UAF after OOM anymore.
Assignee: martin.thomson → ttaubert
Attachment #8667419 - Attachment is obsolete: true
Attachment #8667419 - Flags: review?(ekr)
Attachment #8667867 - Flags: review?(wtc)
Attachment #8667867 - Flags: review?(ekr)
Blocks: 1209152
Comment on attachment 8667867 [details] [diff] [review]
0001-Bug-1209546-Fix-possible-PLArena-leaks-in-ssl3_Handl.patch

Review of attachment 8667867 [details] [diff] [review]:
-----------------------------------------------------------------

Please put this patch up on Rietveld. You can mark it private.

::: lib/ssl/ssl3con.c
@@ +7067,5 @@
>      PORT_SetError( errCode );
>      return SECFailure;
>  
>  no_memory:	/* no-memory error has already been set. */
> +    if (arena) {

IMPORTANT: https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#6737
seems to free the arena above and then you call PORT_FreeArena here too. Please audit the RSA branch as well
Comment on attachment 8667867 [details] [diff] [review]
0001-Bug-1209546-Fix-possible-PLArena-leaks-in-ssl3_Handl.patch

Review of attachment 8667867 [details] [diff] [review]:
-----------------------------------------------------------------

I believe you still have a bug in ssl3con.c. 

Please fix this (or tell me I'm wrong) and put it up on Rietveld for review.
You can mark it private and then only reviewers can see it.
(In reply to Eric Rescorla (:ekr) from comment #5)
> Please put this patch up on Rietveld. You can mark it private.

Yeah, wasn't sure that the trusted list includes non-chromium/google reviewers, should've checked.

> ::: lib/ssl/ssl3con.c
> @@ +7067,5 @@
> >      PORT_SetError( errCode );
> >      return SECFailure;
> >  
> >  no_memory:	/* no-memory error has already been set. */
> > +    if (arena) {
> 
> IMPORTANT:
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.
> c#6737
> seems to free the arena above and then you call PORT_FreeArena here too.
> Please audit the RSA branch as well

Thanks for catching that!
Attachment #8667867 - Attachment is obsolete: true
Attachment #8667867 - Flags: review?(wtc)
Attachment #8667867 - Flags: review?(ekr)
Patch at: https://codereview.appspot.com/269910043
Flags: needinfo?(wtc)
Flags: needinfo?(ekr)
I reviewed your patch on Rietveld.
Flags: needinfo?(wtc)
Updated version of the patch, addressed wtc's comment.
Comment on attachment 8667975 [details] [diff] [review]
0001-Bug-1209546-Fix-possible-PLArena-leaks-in-ssl3_Handl.patch

Review of attachment 8667975 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.
Attachment #8667975 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/a245a4ccd354
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.21
Flags: needinfo?(ekr)
Group: crypto-core-security → core-security-release
[Tracking Requested - why for this release]:
Pretty sure we've decided to stick with not landing NSS 3.21 until Firefox 44, but what about the ESR 38 branch? eventually we'll have a stack of NSS fixes pending for that branch.
Severity: normal → major
Priority: -- → P1
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
If we want to put an NSS update into 38.6.0esr then it should land on esr38 as soon as possible. 
I am looking to build esr38 on Thursday (already later than planned).
Tim and Steve, can you have a look?
Flags: needinfo?(ttaubert)
Flags: needinfo?(sworkman)
We currently don't have a NSS version appropriate for ESR 38 that fixes this bug.

Only NSS 3.21 fixes it, but it contains the root CA changes that we don't want the stable ESR branch to get.

If you decide to upgrade ESR 38 to NSS 3.21, then in directory security/nss/lib/ckfw/builtins/, you should keep the two files nssckbi.h and certdata.txt unchanged (as contained currently on the ESR branch, from NSS 3.19.2.2).

Ideally we should create a special version of NSS for that, like NSS 3.21 with CKBI 2.4, but today and tomorrow I don't have time to give you such a special version.
OK. Let's aim this for 38.7.0esr instead. Dan and Kai, are you ok with that?   
Should we plan to do an NSS update along with Firefox 45 and 38.7.0esr? Or are you waiting until 46?
Flags: needinfo?(dveditz)
Clearing my needinfo - Tim can handle this once the decision is confirmed re which ESR version.
Flags: needinfo?(sworkman)
Waiting until the next ESR is fine.
Flags: needinfo?(dveditz)
Can you please create a summary of fixes that you would prefer to pick up for ESR 38.7, and send that in an email to me, Dan, and other people who want to be involved?

We should plan for that, and coordinate and discuss which NSS version you should use, or if we need to create one for you. Please let's do that a few week prior to the deadline for 38.7.

Thank you
Include me as well please, we can get this into 38.7 as other said.
Flags: needinfo?(ttaubert)
2016-03-07 is the next uplift, is that also the date for ESR 38.7? Do we already have a list of fixes that should go into this ESR release or is that still pending?
Same as bug 1185033 - since this will be fixed by bug 1211568 being uplifted to ESR, and since that bug is already tracked for ESR, I'm not sure it makes sense to track this bug separately. WDYT, Liz?
Flags: needinfo?(lhenry)
I don't know if NSS 3.21 will be uplifted to the ESR 38.x branch.

I'll start a discussion with the NSS developers about the recommended way to fix this bug in ESR 38.x
(In reply to Martin Thomson [:mt:] from comment #12)
> https://hg.mozilla.org/projects/nss/rev/a245a4ccd354

This change applies cleanly on the NSS_3_19_2_X_BRANCH, with just a minor change in context.
Tracked for esr38.8
Clearing needinfo as ritu is handling this.
Flags: needinfo?(lhenry)
Alias: CVE-2016-2790
Alias: CVE-2016-2790 → CVE-2016-1978
Lowering to "moderate" because of the required failed allocation. Would be tricky to time this for an OOM situation that hasn't already caused the browser to crap out.
Blocks: 1254986
According to https://kuix.de/mozilla/versions/ ESR38.7 has NSS 3.19.2.4

Mozilla software version on branch mozilla-esr38: 38.7.1esrpre
NSPR (portable runtime) version: NSPR_4_10_10_RTM
NSS (security library) version: NSS_3_19_2_4_RTM
CKBI (builtin trust) version: 2.4

Did ESR38.8 take an NSS 3.19.2.5 or similar update?


This is marked tracking for ESR38.8.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(kaie)
Based on the latest version info in https://hg.mozilla.org/releases/mozilla-esr38/file/tip/configure.in, ESR38 branch seems to be using NSS 3.19.2.4 (also see Bug 1254986)
Flags: needinfo?(rkothari)
Now a sec moderate, doesn't match the esr criteria.
Flags: needinfo?(sledru)
Flags: needinfo?(kaie)
Sylvestre,

This is about whether ESR38 is taking the next NSS or not. I can't tell if it already did or not. I ni? you on three bugs where this is a question. This wasn't about a decision on this particular bug but on NSS as a whole.

See comment 30:

> Did ESR38.8 take an NSS 3.19.2.5 or similar update?
Flags: needinfo?(sledru)
(In reply to Ritu Kothari (:ritu) from comment #31)
> Based on the latest version info in
> https://hg.mozilla.org/releases/mozilla-esr38/file/tip/configure.in, ESR38
> branch seems to be using NSS 3.19.2.4 (also see Bug 1254986)

The implication here is that this is *fixed* in 3.19.2.4, which was checked into ESR38 on March 21 after this fix was checked into NSS on March 9. 

So, rather than "won't fix" this may be "fixed" already, Sylvestre. For this and the other two bugs, I wanted release management to be clear if we took a version of NSS that fixed these issues, not simply marking them as "won't fix" in status-ESR38.
Flags: needinfo?(rkothari)
NSS 3.19.2.4 landed on the ESR-38 branch in bug 1254986
Flags: needinfo?(rkothari)
Whiteboard: [post-critsmash-triage][adv-main44+] → [post-critsmash-triage][adv-main44+][adv-esr38.8+]
ok; thanks for the feedback
Flags: needinfo?(sledru)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.