1209546 - (CVE-2016-1978) Potential UAF in ssl3_HandleECDHServerKeyExchange

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Eric Rescorla (:ekr)]

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)

Comment 1

[Martin Thomson [:mt:]]

Attachment: bug1209546.patch

I think that this is the right fix for the problem.  I've fixed it for DHE and ECDHE both.

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Martin Thomson [:mt:]]

Ack, I think that Tim knows what to do and can do so.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1209546-Fix-possible-PLArena-leaks-in-ssl3_Handl.patch

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.

Comment 5

[Eric Rescorla (:ekr)]

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 6

[Eric Rescorla (:ekr)]

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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Patch at: https://codereview.appspot.com/269910043

Comment 9

[Wan-Teh Chang]

I reviewed your patch on Rietveld.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1209546-Fix-possible-PLArena-leaks-in-ssl3_Handl.patch

Updated version of the patch, addressed wtc's comment.

Comment 11

[Wan-Teh Chang]

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.

Comment 12

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/a245a4ccd354

Comment 13

[Daniel Veditz [:dveditz]]

[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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 15

[Kai Engert [:KaiE:]]

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.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Clearing my needinfo - Tim can handle this once the decision is confirmed re which ESR version.

Comment 18

[Daniel Veditz [:dveditz]]

Waiting until the next ESR is fine.

Comment 19

[Kai Engert [:KaiE:]]

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

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Include me as well please, we can get this into 38.7 as other said.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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?

Comment 23

[Kai Engert [:KaiE:]]

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

Comment 24

[Kai Engert [:KaiE:]]

(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.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: patch applies cleanly on the NSS_3_19_2_X_BRANCH

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for esr38.8

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Clearing needinfo as ritu is handling this.

Comment 28

[Daniel Veditz [:dveditz]]

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.

Comment 29

[Kai Engert [:KaiE:]]

Checked in to NSS_3_19_2_X_BRANCH
https://hg.mozilla.org/projects/nss/rev/2043304fb048

Comment 30

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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)

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Now a sec moderate, doesn't match the esr criteria.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 34

[Al Billings [:abillings - ex-MoCo]]

(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.

Comment 35

[Daniel Veditz [:dveditz]]

NSS 3.19.2.4 landed on the ESR-38 branch in bug 1254986

Comment 36

[Sylvestre Ledru [:Sylvestre]]

ok; thanks for the feedback