1260046 - Remotely-triggered crash due to insufficient validation of ECDH key material

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Mathieu Lafon]

Attachment: empty-ecdh-point-tls-server.py

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.75 Safari/537.36

Steps to reproduce:

A lack of validation of ECDH key material has been found in ssl3_HandleECDHServerKeyExchange
(nss/lib/ssl/ssl3ecc.c). More specifically, if the ECDH ServerKeyExchange record has an
"empty" ec_point (length set to 0), this will not been detected without dereferencing the
buffer, leading to a NULL-pointer-dereference crash.

This can be used to remotely crash (DoS) products using NSS, and especially Firefox, by
redirecting users to an evil server. Note that the server does not need to use a trusted
certificate to trigger the vulnerability, the crash will occur before checking if the
certificate is trusted.

The vulnerable code is:

    rv = ssl3_ConsumeHandshakeVariable(ss, &ec_point, 1, &b, &length);
    if (rv != SECSuccess) {
        goto loser; /* malformed. */
    }
    /* Fail if the ec point uses compressed representation */
    if (ec_point.data[0] != EC_POINT_FORM_UNCOMPRESSED) {
        ...
    }

This vulnerability can be triggered using the attached POC server.


Actual results:

Crash using tstclnt and attached POC server.

$ bin/tstclnt -h localhost -p 4433
Segmentation fault (core dumped)
$ gdb bin/tstclnt core
GNU gdb (Ubuntu 7.7.1-0ubuntu5~14.04.2) 7.7.1
[...]
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f5b8118b553 in ssl3_HandleECDHServerKeyExchange (ss=0xb13680,
    b=0xb1b9f8 "\002\\0\202\002X0\202\001\301\240\003\002\001\002\002\t", length=0)
    at ssl3ecc.c:790
790         if (ec_point.data[0] != EC_POINT_FORM_UNCOMPRESSED) {
(gdb) bt
#0  0x00007f5b8118b553 in ssl3_HandleECDHServerKeyExchange (ss=0xb13680,
    b=0xb1b9f8 "\002\\0\202\002X0\202\001\301\240\003\002\001\002\002\t", length=0)
    at ssl3ecc.c:790
#1  0x00007f5b81161a4c in ssl3_HandleServerKeyExchange (ss=0xb13680, b=0xb1b9f4 "\003",
    length=4) at ssl3con.c:7446
#2  0x00007f5b8116c5c3 in ssl3_HandlePostHelloHandshakeMessage (ss=0xb13680,
    b=0xb1b9f4 "\003", length=4, hashesPtr=0x0) at ssl3con.c:12325
#3  0x00007f5b8116c35a in ssl3_HandleHandshakeMessage (ss=0xb13680, b=0xb1b9f4 "\003",
    length=4) at ssl3con.c:12269
#4  0x00007f5b8116c9d7 in ssl3_HandleHandshake (ss=0xb13680, origBuf=0xb138b0)
    at ssl3con.c:12442
#5  0x00007f5b8116e16f in ssl3_HandleRecord (ss=0xb13680, cText=0x7fff614bb490,
    databuf=0xb138b0) at ssl3con.c:13130  
#6  0x00007f5b81170417 in ssl3_GatherCompleteHandshake (ss=0xb13680, flags=0)
    at ssl3gthr.c:482
#7  0x00007f5b8117103d in ssl_GatherRecord1stHandshake (ss=0xb13680) at sslcon.c:81
#8  0x00007f5b81179ed0 in ssl_Do1stHandshake (ss=0xb13680) at sslsecur.c:70
#9  0x00007f5b8117c52e in ssl_SecureRecv (ss=0xb13680, buf=0x7fff614bb8f0 "", len=4000,
    flags=0) at sslsecur.c:1099
#10 0x00007f5b81186732 in ssl_Recv (fd=0xafeb10, buf=0x7fff614bb8f0, len=4000, flags=0,
    timeout=4294967295) at sslsock.c:2565 
#11 0x00007f5b8052d67e in PR_Recv (fd=0xafeb10, buf=0x7fff614bb8f0, amount=4000, flags=0,
    timeout=4294967295) at ../../../../pr/src/io/priometh.c:188
#12 0x0000000000408b30 in main (argc=5, argv=0x7fff614bc998) at tstclnt.c:1609
(gdb) print ec_point.data
$1 = (unsigned char *) 0x0
(gdb) print ec_point.len
$2 = 0



Expected results:

No crash. The fix is trivial by verifying ec_point.len before dereferencing ev_point.data.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: asan_ff_call_stack.txt

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: properlyCheckPoints.patch

this patch fixes the issue by checking whether the ec_point actually contains data.

Comment 3

[Tyson Smith [:tsmith]]

Unless there is something more here than a null crash this should be sec-low.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8736306 [details] [diff] [review]
properlyCheckPoints.patch

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

::: lib/ssl/ssl3ecc.c
@@ +786,5 @@
>      if (rv != SECSuccess) {
>          goto loser; /* malformed. */
>      }
>      /* Fail if the ec point uses compressed representation */
> +    if (!ec_point.data || ec_point.data[0] != EC_POINT_FORM_UNCOMPRESSED) {

I feel like checking for ec_point.len would be better.

Also, can you please go through the rest of the code where points are read and make sure they don't have similar problems before landng this?

Comment 5

[Martin Thomson [:mt:]]

A test wouldn't hurt either.

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: properlyCheckPoints.patch

ok, using len and added a test. Ekr or mt do you want to have another look at it?

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: properlyCheckPoints.patch

(In reply to Eric Rescorla (:ekr) from comment #4)
> Comment on attachment 8736306 [details] [diff] [review]
> Also, can you please go through the rest of the code where points are read
> and make sure they don't have similar problems before landng this?

checked again. the client key exchange has the same problem. I added a test for that one as well and fixed it. We didn't set any error codes before in ssl3_HandleECDHClientKeyExchange. Can one of you review? I think we should get this in 3.24.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 8737197 [details] [diff] [review]
properlyCheckPoints.patch

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

Can you please check TLS 1.3 for this and produce a test/patch if needed.

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8737197 [details] [diff] [review]
properlyCheckPoints.patch

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

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +1130,5 @@
> +    }
> +
> +    // Add an empty point to the server key exchange message
> +    output->Allocate(4);
> +    output->Write(0, 0x0300, 2);

What is this?  (comment please)

@@ +1131,5 @@
> +
> +    // Add an empty point to the server key exchange message
> +    output->Allocate(4);
> +    output->Write(0, 0x0300, 2);
> +    uint32_t curve = 0x17;

You shouldn't need to initialize this.

@@ +1132,5 @@
> +    // Add an empty point to the server key exchange message
> +    output->Allocate(4);
> +    output->Write(0, 0x0300, 2);
> +    uint32_t curve = 0x17;
> +    input.Read(2, 1, &curve);

EXPECT_TRUE on the return value.

@@ +1134,5 @@
> +    output->Write(0, 0x0300, 2);
> +    uint32_t curve = 0x17;
> +    input.Read(2, 1, &curve);
> +    output->Write(2, curve, 1);
> +    output->Write(3, (uint32_t)0x00, 1);

0U

@@ +1141,5 @@
> +};
> +
> +TEST_P(TlsConnectGenericPre13, ConnectECDHEmptyServerPoint) {
> +  // disable DHE and RSA cipher suites to get only ECDH
> +  ResetEcdsa();

You get ECDHE by default, so I'm confused.

::: lib/ssl/ssl3ecc.c
@@ +424,5 @@
>      SECKEYPublicKey clntPubKey;
>      CK_MECHANISM_TYPE target;
>      PRBool isTLS, isTLS12;
> +    int errCode = SSL_ERROR_RX_MALFORMED_CLIENT_KEY_EXCH;
> +    SSL3AlertDescription desc = illegal_parameter;

Set only once please.

@@ +444,5 @@
> +
> +    // we have to catch the case when the server's public key has length 0
> +    if (!clntPubKey.u.ec.publicValue.len) {
> +        errCode = SEC_ERROR_UNSUPPORTED_EC_POINT_FORM;
> +        desc = handshake_failure;

illegal_parameter

@@ +480,5 @@
>      }
>      return SECSuccess;
> +
> +alert_loser:
> +    (void)SSL3_SendAlert(ss, alert_fatal, desc);

This is only used in one place.  You can save a goto by calling this on line 448.  Then you don't need the temporary.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8737197 [details] [diff] [review]
properlyCheckPoints.patch

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

Clearing review flag.

Comment 11

[Franziskus Kiefer [:franziskus]]

Attachment: properlyCheckPoints.patch

Thanks for your comments mt. This should address them

(In reply to Martin Thomson [:mt:] from comment #9)
> Comment on attachment 8737197 [details] [diff] [review]
> properlyCheckPoints.patch
> 
> Review of attachment 8737197 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +444,5 @@
> > +
> > +    // we have to catch the case when the server's public key has length 0
> > +    if (!clntPubKey.u.ec.publicValue.len) {
> > +        errCode = SEC_ERROR_UNSUPPORTED_EC_POINT_FORM;
> > +        desc = handshake_failure;
> 
> illegal_parameter

If we set illegal_parameter we should also do errCode = SSL_ERROR_RX_MALFORMED_CLIENT_KEY_EXCH. I've done that in both cases now.

> Can you please check TLS 1.3 for this and produce a test/patch if needed.

TLS1.3 uses tls13_ImportECDHKeyShare to read points from key exchange messages and checks the length of the point. That looks ok (we might want to write tests for that, but we can do that as a follow up).

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8737720 [details] [diff] [review]
properlyCheckPoints.patch

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

::: lib/ssl/ssl3ecc.c
@@ +428,5 @@
>  
>      rv = ssl3_ConsumeHandshakeVariable(ss, &clntPubKey.u.ec.publicValue,
>                                         1, &b, &length);
>      if (rv != SECSuccess) {
> +        goto loser;

I'm going to ask for one more change here, which should be mechanical.  Now that I've seen the changes here, I think that you can easily call PORT_SetError(...) and return SECFailure and avoid the goto loser part.

Comment 13

[Franziskus Kiefer [:franziskus]]

landed as https://hg.mozilla.org/projects/nss/rev/24993421bd00