1246567 - NULL out xSS and xES after PK11_FreeSymKey

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: 0001-NULL-xSS-and-xES-after-call-to-PK11_SymKeyFree.patch

Comment 2

[Eric Rescorla (:ekr)]

MT, this is a likely UAF but it only occurs in the TLS 1.3 code path.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8716860 [details] [diff] [review]
0001-NULL-xSS-and-xES-after-call-to-PK11_SymKeyFree.patch

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

Wasn't sure whether you CC'ed me to get another opinion or to just let me know. In any case, LGTM.

Comment 4

[Eric Rescorla (:ekr)]

Dan, Wan-Teh. This is a memory error, but it's in an experimental version of NSS that is off by default in Firefox. Any reason not to just land it?

Comment 5

[Wan-Teh Chang]

Comment on attachment 8716860 [details] [diff] [review]
0001-NULL-xSS-and-xES-after-call-to-PK11_SymKeyFree.patch

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

r=wtc. It is fine to check this in.

::: lib/ssl/tls13con.c
@@ +1209,5 @@
>  loser:
>      PK11_FreeSymKey(ss->ssl3.hs.xSS);
>      PK11_FreeSymKey(ss->ssl3.hs.xES);
> +    ss->ssl3.hs.xSS = NULL;
> +    ss->ssl3.hs.xES = NULL;

Nit: you may want to put them in this order:

    PK11_FreeSymKey(ss->ssl3.hs.xSS);
    ss->ssl3.hs.xSS = NULL;
    PK11_FreeSymKey(ss->ssl3.hs.xES);
    ss->ssl3.hs.xES = NULL;

Comment 6

[Eric Rescorla (:ekr)]

Committed as: https://hg.mozilla.org/projects/nss/rev/d7d940a5999d

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Eric Rescorla (:ekr) from comment #4)
> Dan, Wan-Teh. This is a memory error, but it's in an experimental version of
> NSS that is off by default in Firefox. Any reason not to just land it?

"by default"? If it's just a pref switch we should change the status flag to "disabled" instead of "unaffected" -- but either way sure, go ahead and land it if the NSS tree is open

Comment 8

[Martin Thomson [:mt:]]

:dveditz, this is off in the sense that it can't be turned on.  I guess that makes "by default" moot :)

Comment 9

[Eric Rescorla (:ekr)]

mt: actually it sort of can be. It's not compiled off in Firefox, and while you can set the variables in about:config, I'm not sure that it will actually negotiate 1.3 if you do that because a lot of changes in PSM are needed.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Did we forget to resolve this? Was it left open intentionally?