363073 - ECC TLS client crashes if the server doesn't send the ServerKeyExchange message

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Based on the information Vipul provided in email, I commented out
all the ssl3_AppendXXX calls in ssl3_SendECDHServerKeyExchange so
that our ECC TLS server doesn't send the ServerKeyExchange message,
and ran all.sh.  I got tstclnt and strsclnt to crash.  This may or
may not be the Firefox crash that Vipul saw.  Here is a stack trace
of tstclnt: 

(gdb) where
#0  0x005cf095 in memcpy () from /lib/tls/libc.so.6
#1  0x009bd7b1 in sftk_NewAttribute (object=0x95115c8, type=384, value=0xa,
    len=156304975) at pkcs11u.c:98
#2  0x009c14d9 in sftk_AddAttributeType (object=0x95115c8, type=384,
    valPtr=0xa, length=156304975) at pkcs11u.c:2080
#3  0x009b8444 in NSC_GenerateKeyPair (hSession=1, pMechanism=0xbff928f0,
    pPublicKeyTemplate=0xbff92920, ulPublicKeyAttributeCount=7,
    pPrivateKeyTemplate=0xbff92ae0, ulPrivateKeyAttributeCount=7,
    phPublicKey=0xbff928d0, phPrivateKey=0xbff928d4) at pkcs11c.c:3371
#4  0x0013449a in PK11_GenerateKeyPairWithFlags (slot=0x94e2348, type=4160,
    param=0x95105a8, pubKey=0xbff92c00, attrFlags=138, wincx=0x0)
    at pk11akey.c:988
#5  0x0013490b in PK11_GenerateKeyPair (slot=0x94e2348, type=4160,
    param=0x95105a8, pubKey=0xbff92c00, token=0, sensitive=0, wincx=0x0)
    at pk11akey.c:1105
#6  0x0012c045 in SECKEY_CreateECPrivateKey (param=0x95105a8,
    pubk=0xbff92c00, cx=0x0) at seckey.c:258
#7  0x00ce4311 in ssl3_SendECDHClientKeyExchange (ss=0x94e9e90,
    svrPubKey=0x9510598) at ssl3ecc.c:350
#8  0x00cc6a8d in ssl3_SendClientKeyExchange (ss=0x94e9e90) at ssl3con.c:4322
#9  0x00cc873c in ssl3_HandleServerHelloDone (ss=0x94e9e90) at ssl3con.c:5214
#10 0x00ccd605 in ssl3_HandleHandshakeMessage (ss=0x94e9e90, b=0x94f1c26 "",
    length=0) at ssl3con.c:7706
#11 0x00ccd8cb in ssl3_HandleHandshake (ss=0x94e9e90, origBuf=0x94ea0ec)
    at ssl3con.c:7793
#12 0x00cce19e in ssl3_HandleRecord (ss=0x94e9e90, cText=0xbff92e00,
    databuf=0x94ea0ec) at ssl3con.c:8056
#13 0x00ccf38f in ssl3_GatherCompleteHandshake (ss=0x94e9e90, flags=0)
    at ssl3gthr.c:206
#14 0x00cd1d3b in ssl_GatherRecord1stHandshake (ss=0x94e9e90) at sslcon.c:1258
#15 0x00cd94e9 in ssl_Do1stHandshake (ss=0x94e9e90) at sslsecur.c:149
#16 0x00cdb406 in ssl_SecureSend (ss=0x94e9e90,
    buf=0xbff92f60 "GET / HTTP/1.0\r\n\r\nhare/dev1/nspr/wtchang/nss-3.11-branch
-extended-ecc/mozilla/security/nss/cmd/tstclnt/Linux2.6_x86_glibc_PTH_DBG.OBJ/ts
tclnt", len=18, flags=0) at sslsecur.c:1090
#17 0x00ce10fc in ssl_Send (fd=0x94e9cb8, buf=0xbff92f60, len=18, flags=0,
    timeout=4294967295) at sslsock.c:1414
#18 0x00a185b1 in PR_Send (fd=0x94e9cb8, buf=0xbff92f60, amount=18, flags=0,
    timeout=4294967295) at ../../../../pr/src/io/priometh.c:226
#19 0x0804d1df in main (argc=13, argv=0xbff945a4) at tstclnt.c:890

Comment 1

[Wan-Teh Chang]

Attachment: (Do not check in) patch to force the crash

Apply this patch to NSS_3_11_BRANCH, build Extended ECC, and
run all.sh to reproduce this bug.

strsclnt crashed at the same place:

(gdb) where
#0  0x005cf095 in memcpy () from /lib/tls/libc.so.6
#1  0x002447b1 in sftk_NewAttribute (object=0x9401210, type=384, value=0xa,
    len=155182999) at pkcs11u.c:98
#2  0x002484d9 in sftk_AddAttributeType (object=0x9401210, type=384,
    valPtr=0xa, length=155182999) at pkcs11u.c:2080
#3  0x0023f444 in NSC_GenerateKeyPair (hSession=16777217,
    pMechanism=0xb7fb79f0, pPublicKeyTemplate=0xb7fb7a20,
    ulPublicKeyAttributeCount=7, pPrivateKeyTemplate=0xb7fb7be0,
    ulPrivateKeyAttributeCount=7, phPublicKey=0xb7fb79d0,
    phPrivateKey=0xb7fb79d4) at pkcs11c.c:3371
#4  0x006be49a in PK11_GenerateKeyPairWithFlags (slot=0x93a8490, type=4160,
    param=0x93fe6f0, pubKey=0xb7fb7d00, attrFlags=138, wincx=0x0)
    at pk11akey.c:988
#5  0x006be90b in PK11_GenerateKeyPair (slot=0x93a8490, type=4160,
    param=0x93fe6f0, pubKey=0xb7fb7d00, token=0, sensitive=0, wincx=0x0)
    at pk11akey.c:1105
#6  0x006b6045 in SECKEY_CreateECPrivateKey (param=0x93fe6f0,
    pubk=0xb7fb7d00, cx=0x0) at seckey.c:258
#7  0x00998311 in ssl3_SendECDHClientKeyExchange (ss=0x93d9d98,
    svrPubKey=0x93fe6e0) at ssl3ecc.c:350
#8  0x0097aa8d in ssl3_SendClientKeyExchange (ss=0x93d9d98) at ssl3con.c:4322
#9  0x0097c73c in ssl3_HandleServerHelloDone (ss=0x93d9d98) at ssl3con.c:5214
#10 0x00981605 in ssl3_HandleHandshakeMessage (ss=0x93d9d98, b=0x93e17ee "",
    length=0) at ssl3con.c:7706
#11 0x009818cb in ssl3_HandleHandshake (ss=0x93d9d98, origBuf=0x93d9ff4)
    at ssl3con.c:7793
#12 0x0098219e in ssl3_HandleRecord (ss=0x93d9d98, cText=0xb7fb7f00,
    databuf=0x93d9ff4) at ssl3con.c:8056
#13 0x0098338f in ssl3_GatherCompleteHandshake (ss=0x93d9d98, flags=0)
    at ssl3gthr.c:206
#14 0x00985d3b in ssl_GatherRecord1stHandshake (ss=0x93d9d98) at sslcon.c:1258
#15 0x0098d4e9 in ssl_Do1stHandshake (ss=0x93d9d98) at sslsecur.c:149
#16 0x0098f406 in ssl_SecureSend (ss=0x93d9d98,
    buf=0x80576b6 "GET /abc HTTP/1.0\r\n\r\n", len=21, flags=0)
    at sslsecur.c:1090
#17 0x009950fc in ssl_Send (fd=0x93bb528, buf=0x80576b6, len=21, flags=0,
    timeout=4294967295) at sslsock.c:1414
#18 0x001d95b1 in PR_Send (fd=0x93bb528, buf=0x80576b6, amount=21, flags=0,
    timeout=4294967295) at ../../../../pr/src/io/priometh.c:226
#19 0x0804c997 in handle_connection (ssl_sock=0x93bb528, tid=0)
    at strsclnt.c:693
#20 0x0804d07c in do_connects (a=0xbfeae2f0, b=0x93af980, tid=0)
    at strsclnt.c:884
#21 0x0804c24d in thread_wrapper (arg=0x805fe40) at strsclnt.c:436
#22 0x001fc136 in _pt_root (arg=0x93bb400)
    at ../../../../pr/src/pthreads/ptthread.c:220
#23 0x007993ae in start_thread () from /lib/tls/libpthread.so.0
#24 0x0062baee in clone () from /lib/tls/libc.so.6

Comment 2

[Wan-Teh Chang]

I printed some values in the debugger:

#3  0x00263444 in NSC_GenerateKeyPair (hSession=16777217,
    pMechanism=0xb7fb79f0, pPublicKeyTemplate=0xb7fb7a20,
    ulPublicKeyAttributeCount=7, pPrivateKeyTemplate=0xb7fb7be0,
    ulPrivateKeyAttributeCount=7, phPublicKey=0xb7fb79d0,
    phPrivateKey=0xb7fb79d4) at pkcs11c.c:3371
3371            crv = sftk_AddAttributeType(publicKey,
(gdb) print pPublicKeyTemplate[0]
$5 = {type = 384, pValue = 0xa, ulValueLen = 167016343}
(gdb) print i
$6 = 0

Note: 384 is CKA_EC_PARAMS. ulValueLen seems to be garbge because
other core files have different values of ulValueLen.  pValue is 0xa
in all core files.

#4  0x00c7349a in PK11_GenerateKeyPairWithFlags (slot=0x9ef1490, type=4160,
    param=0x9f476f0, pubKey=0xb7fb7d00, attrFlags=138, wincx=0x0)
    at pk11akey.c:988
988         crv = PK11_GETTAB(slot)->C_GenerateKeyPair(session_handle, &mechanis
m,
(gdb) print param
$8 = (void *) 0x9f476f0
(gdb) print ecParams
$9 = (SECKEYECParams *) 0x9f476f0
(gdb) print *ecParams
$10 = {type = siBuffer, data = 0xa "", len = 167016343}

#7  0x00fcc311 in ssl3_SendECDHClientKeyExchange (ss=0x9f22d98,
    svrPubKey=0x9f476e0) at ssl3ecc.c:350
350         privKey = SECKEY_CreateECPrivateKey(&svrPubKey->u.ec.DEREncodedParam
s,
(gdb) print svrPubKey->u.ec
$17 = {DEREncodedParams = {type = siBuffer, data = 0xa "", len = 167016343},
  size = 128, publicValue = {type = siUnsignedInteger,
    data = 0x9f47819 "\001", len = 3}}

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Taking.  I invite discussion on the priority and target milestone of this bug.

Vipul, please add a comment here telling us about what kind (brand) of server
you were testing with that failed to send a Server Key Exchange?  I hope it
wasn't an NSS server.

Comment 4

[Wan-Teh Chang]

Attachment: results.html of all.sh with the patch to force the crash applied

The output.log file is too big to attach, so I reproduce the relevant
excerpts here.  It shows that all the crashes occurred with ECDHE RSA
cipher suites (C010 - C014):

ssl.sh: running TLS ECDHE RSA WITH AES 128 CBC SHA ----------------------------
kill -0 21012 >/dev/null 2>/dev/null
selfserv with PID 21012 found at Thu Dec  7 07:41:23 PST 2006
tstclnt -p 8443 -h earthquake.dsdev.sjc.redhat.com -c :C013  -B -s \
        -f -d ../client < /share/dev1/nspr/wtchang/nss-3.11-branch-extended-ecc/
mozilla/security/nss/tests/ssl/sslreq.dat
./ssl.sh: line 260: 21087 Segmentation fault      tstclnt -p ${PORT} -h ${HOSTAD
DR} -c ${param} ${TLS_FLAG} ${CLIENT_OPTIONS} -f -d ${P_R_CLIENTDIR} <${REQUEST_
FILE} >${TMP}/$HOST.tmp.$$ 2>&1
ssl.sh: TLS ECDHE RSA WITH AES 128 CBC SHA produced a returncode of 139, expecte
d is 0 FAILED

In contrast, ECDHE ECDSA cipher suites failed but didn't crash:

ssl.sh: running TLS ECDHE ECDSA WITH AES 128 CBC SHA ---------------------------
-
kill -0 24930 >/dev/null 2>/dev/null
selfserv with PID 24930 found at Thu Dec  7 07:42:29 PST 2006
tstclnt -p 8443 -h earthquake.dsdev.sjc.redhat.com -c :C009  -B -s \
        -f -d ../ext_client < /share/dev1/nspr/wtchang/nss-3.11-branch-extended-
ecc/mozilla/security/nss/tests/ssl/sslreq.dat
selfserv: HDX PR_Read returned error -12273:
SSL received a record with an incorrect Message Authentication Code.
tstclnt: write to SSL socket failed: SSL peer reports incorrect Message Authenti
cation Code.
ssl.sh: TLS ECDHE ECDSA WITH AES 128 CBC SHA produced a returncode of 254, expec
ted is 0 FAILED

This is consistent with the fact that all the core files have
serverKey->keyType equal to rsaKey in ssl3_SendClientKeyExchange.

Comment 5

[Wan-Teh Chang]

Attachment: Patch to fix the crash (the symptom only)

I think this is where things start to go wrong in
ssl3_SendClientKeyExchange:

4282     if (ss->sec.peerKey == NULL) {
4283         serverKey = CERT_ExtractPublicKey(ss->sec.peerCert);
4284         if (serverKey == NULL) {
4285             PORT_SetError(SSL_ERROR_EXTRACT_PUBLIC_KEY_FAILURE);
4286             return SECFailure;
4287         }
4288     } else {
4289         serverKey = ss->sec.peerKey;
4290         ss->sec.peerKey = NULL; /* we're done with it now */
4291     }

We incorrectly use the public key from the peer's cert as the
peer's ephemeral public key.  If the peer's cert has an ECDSA
public key, we go on to do EC Diffie-Hellman key derivation with
that key, but eventually will fail the handshake.  If the peer's
cert has an RSA public key, we crash in EC Diffie-Hellman key
derivation because RSA public key, when interpreted as an EC
public key, contains garbage.

This patch fixes the crash by making sure the server's public key
has the expected key type.  It only fixes the symptom (crash), not
the bug.  I also patched the equivalent code in sendDHClientKeyExchange
but don't have an easy way to test it.

Comment 6

[Wan-Teh Chang]

Comment on attachment 247904 [details] [diff] [review]
Patch to fix the crash (the symptom only)

I tested both changes in this patch with Firefox 2.0 in a debugger.
I injected the error in the debugger by setting ss->sec.peerKey to
NULL in ssl3_SendClientKeyExchange for DHE-RSA and ECDHE-RSA, and
verified that this patch was effective.  Note that for DHE-RSA the
original code doesn't crash because we eventually find out that the
peer's ephemeral public key is of the wrong type in the PK11_PubDerive
call made by sendDHClientKeyExchange, so the ssl3con.c change in this
patch isn't necessary, but it catches that error sooner and avoids the
useless SECKEY_CreateDHPrivateKey call (which succeeded in my test).

Nelson, the bug is now all yours.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 247904 [details] [diff] [review]
Patch to fix the crash (the symptom only)

r=nelson
I think it is entirely appropriate to commit this fix now. 
That will eliminate the DOS potential.
We can do something more elegant for 3.12.  
But this work will not have been wasted.
Requesting second review for the 3.11 branch.
Thanks for the patch, Wan-Teh!

Comment 8

[Wan-Teh Chang]

Attachment: Patch to fix the crash (the symptom only) v1.1

I checked in the "patch to fix the crash (the symptom only)", with
a minor comment change (to not separate the XXX comment from the
code it refers to), on the NSS trunk (NSS 3.12).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.99; previous revision: 1.98
done
Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.18; previous revision: 1.17
done

Comment 9

[Wan-Teh Chang]

Comment on attachment 248016 [details] [diff] [review]
Patch to fix the crash (the symptom only) v1.1

Vipul confirmed that the ssl3ecc.c change in this patch fixed the
crash for him, too.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 248016 [details] [diff] [review]
Patch to fix the crash (the symptom only) v1.1

r=nelson
Carrying my r= forward to this patch from the previous one.

Comment 11

[Alexei Volkov]

Comment on attachment 248016 [details] [diff] [review]
Patch to fix the crash (the symptom only) v1.1

r=alexei.volkov.bugs

Comment 12

[Wan-Teh Chang]

I checked in the "patch to fix the crash (the symptom only) v1.1"
on the NSS_3_11_BRANCH (NSS 3.11.5).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.19; previous revision: 1.76.2.18
done
Checking in ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.11; previous revision: 1.3.2.10
done

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

This bug reports a crash.  Wan-Teh fixed the crash in NSS 3.11.5.
I am reassigning this bug to him (to record that he fixed it),
and then will mark it resolved/fixed.

A new bug needs to be opened about the fact that the client uses the
wrong key if/when the server omits the necessary SKE message.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

See bug 379557 for the continuation of this issue.