1306103 - (CVE-2016-5285) Missing NULL check in PK11_SignWithSymKey / ssl3_ComputeRecordMACConstantTime causes server crash

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

We've received a report, that an NSS server can be crashed, by a client that sends an invalid DH key.

#0  PK11_SignWithSymKey (symKey=0x0, mechanism=3461563235, param=param@entry=0x7ffff2037e30, sig=sig@entry=0x7ffff2037e70, data=data@entry=0x7ffff2037e50)
    at pk11obj.c:792
#1  0x00007ffff7bb347d in ssl3_ComputeRecordMACConstantTime (outLen=0x7ffff2037e1c, outbuf=0x7ffff2037ec0 "P\001", originalLen=40, inputLen=40,
    input=0x6a3d30 "SCRUBBED", headerLen=13,
    header=0x7ffff2037eb0 "", useServerMacKey=0, spec=0x69dfd0) at ssl3con.c:2539


Marking as a sensitive bug for now, as this seems to enable remote denial of service.

Comment 1

[Robert Relyea]

So we should check for NULL in PK11_SignWithSymKey, but there is definitely a bug in the SSL engine. We should either not get this far on the error, or we should be using a fake key.

I've just looked at the code and the ssl3_HandleDHClientKeyExchange() properly returns failure, so the engine should not continue on.

Also, this isn't the only path through that could be an issue. The constant time mac is only used for block ciphers, AES_GCM would trigger ssl3_ComputeRecordMAC which will use a different NSS functions (PK11_DigestBegin, PK11_DigestOp PK11_DigestFinal). The bypass path may not actually fail, but it will allow MITM attacks on the mac, since it will generate macs with NULL keys.

bob

Do we have a test case for this yet?

Comment 2

[Robert Relyea]

OK, looking at the downstream bug. The bug was fixed in NSS 3.25, which is why I didn't see an issue (I'm looking at the latest NSS in hg, which is why I didn't see a problem).

bob

Comment 3

[Hubert Kario]

the problem is that NSS doesn't abort on receiving an invalid client key exchange message

the bug is not limited to client key share value of 1, if the key share is too large (e.g. 8192 bits long if the server uses 2048 bit prime), the segfault will happen too upon receiving the Finished message

Comment 4

[Hubert Kario]

reproducer that makes it easier to see handling of different variants of the malformed client key exchange message:
https://github.com/tomato42/tlsfuzzer/blob/master/scripts/test-dhe-rsa-key-exchange-with-bad-messages.py

the server will crash in "invalid dh_Yc value - 8192b" after uncommenting "node = node.add_child(FinishedGenerator())" lines

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Confirming what Bob said in comment #2. It affects only NSS 3.24 and below.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I wonder, are clients affected too? There certainly are no sanity checks either:

http://searchfox.org/nss/rev/84801b2f70d9adca49bec3c63610cc6436fde505/lib/ssl/ssl3con.c#7531

Comment 7

[Hubert Kario]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> I wonder, are clients affected too? There certainly are no sanity checks
> either:
> 
> http://searchfox.org/nss/rev/84801b2f70d9adca49bec3c63610cc6436fde505/lib/
> ssl/ssl3con.c#7531

No, there are simple checks on line 7566:

            dh_Ys_bits = SECKEY_BigIntegerBitLength(&dh_Ys);
            if (dh_Ys_bits > dh_p_bits || dh_Ys_bits <= 1)
                goto alert_loser;

so a server share of "0", "1", or a value much larger that the prime, will be rejected

Comment 8

[Hubert Kario]

I've tested with server that presented key share equal to p and p-1 and the client aborted with following error:

  tstclnt: read from socket failed: SEC_ERROR_NO_MEMORY: security library: memory allocation failure.

without sending any alerts. Valgrind didn't complain about any bad memory accesses. That was with nss-3.21.0-17.el7.x86_64.

With key share equal to 0 or 1, client correctly sends an illegal_parameter alert.

Comment 9

[Kai Engert (:KaiE:)]

Does Firefox use any server code, that is affected by this issue? Hubert suggested that potentially WebRTC, if it offers DH?

Comment 10

[Kai Engert (:KaiE:)]

Attachment: nss-dh.patch

This patch was provided by Jeffrey Arbuckle in the redhat bug.

Bob reviewed it, concluded it's sufficient to fix the issue on NSS 3.21 branch, and gave it r=rrelyea in the redhat bug.

Comment 11

[Kai Engert (:KaiE:)]

Is Mozilla interested to take this bug for Firefox ESR 45.x, potentially for the November 45.5 updated?

If Mozilla thinks you aren't affected, it would still be good to land this into the NSS 3.21 branch and make an NSS release.

Tim, do you think additional sanity checks should be added for the client side?

Comment 12

[Kai Engert (:KaiE:)]

I just notice this is a duplicate of bug 1278645.

Comment 14

[Huzaifa Sidhpurwala]

ping,

This is a security issue and it affects the versions we ship. Does this need a CVE id, if mozilla does not want to assign one, we can.

Comment 15

[Daniel Veditz [:dveditz]]

Use CVE-2016-5285

Comment 16

[Martin Thomson [:mt:]]

I couldn't line up the line numbers, but I assume that this patch is applied to ssl3_HandleDHClientKeyExchange.  After eyeballing the code, it looks like we narrowly avoid the bug on the client side as well in sendDHClientKeyExchange.  (Should someone add a call to a function that modifies rv before doing key derivation, the bug will be re-introduced.)

We don't need to fix the client on an old release.

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #16)
> I couldn't line up the line numbers, but I assume that this patch is applied
> to ssl3_HandleDHClientKeyExchange.

Correct.

Comment 18

[Kai Engert (:KaiE:)]

checked in to NSS 3.21.x branch:
https://hg.mozilla.org/projects/nss/rev/45c047d18ac4

It will be part of the NSS 3.21.3 release.

It's unclear if Firefox wants to pick up that release, that's tracked in bug 1310009.

Comment 19

[david]

(In reply to Robert Relyea from comment #1)

Also, this isn't the only path through that could be an issue. The constant
time mac is only used for block ciphers, AES_GCM would trigger
ssl3_ComputeRecordMAC which will use a different NSS functions
(PK11_DigestBegin, PK11_DigestOp PK11_DigestFinal). The bypass path may not
actually fail, but it will allow MITM attacks on the mac, since it will
generate macs with NULL keys.

bob

Do we have a test case for this yet?

Is this bug actually just a null deference then? Seems like it might have been misclassified based on this comment.

Comment 20

[Robert Relyea]

The original bug was a NULL dereference in the ssl engine, which has been fixed. My comment was about the bypass code with would treat the null as an empty key and merrily continue on with generating and accepting macs with known empty keys. It's not really and issue anymore because we've since removed all the bypass code (even at the time it wasn't used by anyone).