Support TLS Channel IDs in NSS

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
diff --git a/net/third_party/nss/ssl/ssl3con.c b/net/third_party/nss/ssl/ssl3con.c
index db9fad3dde5c2d84d4c14d7b0292f77a9ac743d2..508e7085ac7a673ef88775cb109e1246bed972c5 100644
--- a/net/third_party/nss/ssl/ssl3con.c
+++ b/net/third_party/nss/ssl/ssl3con.c
@@ -86,6 +86,7 @@ static SECStatus ssl3_SendCertificate(       sslSocket *ss);
 static SECStatus ssl3_SendEmptyCertificate(  sslSocket *ss);
 static SECStatus ssl3_SendCertificateRequest(sslSocket *ss);
 static SECStatus ssl3_SendNextProto(         sslSocket *ss);
+static SECStatus ssl3_SendEncryptedExtensions(sslSocket *ss);
 static SECStatus ssl3_SendFinished(          sslSocket *ss, PRInt32 flags);
 static SECStatus ssl3_SendServerHello(       sslSocket *ss);
 static SECStatus ssl3_SendServerHelloDone(   sslSocket *ss);
@@ -6238,6 +6239,10 @@ ssl3_SendClientSecondRound(sslSocket *ss)
 	if (rv != SECSuccess) {
 	    goto loser;	/* err code was set. */
 	}
+	rv = ssl3_SendEncryptedExtensions(ss);
+	if (rv != SECSuccess) {
+	    goto loser; /* err code was set. */
+	}
     }
 
     rv = ssl3_SendFinished(ss, 0);
@@ -8856,6 +8861,111 @@ ssl3_SendNextProto(sslSocket *ss)
 }
 
 /* called from ssl3_HandleServerHelloDone

[wtc, 2012/05/24 23:10:03]

This comment should be updated in the same way.

[agl, 2012/05/30 15:28:33]

Done.

+ */
+static SECStatus
+ssl3_SendEncryptedExtensions(sslSocket *ss)

[wtc, 2012/05/24 23:10:03]

Ideally this function should be structured so that it calls
encrypted extension senders.  Right now it is hardcoded to
send the Channel ID extension only.  This is acceptable as
a first implementation.

+{
+    SECStatus rv = SECFailure;
+    SECItem *spki = NULL;
+    SSL3Hashes hashes;
+    const unsigned char *pub_bytes;
+    static const char CHANNEL_ID_MAGIC[] = "TLS Channel ID signature";
+    unsigned char signed_data[sizeof(CHANNEL_ID_MAGIC) + sizeof(SSL3Hashes)];

[wtc, 2012/05/24 23:10:03]

IMPORTANT: By using sizeof(CHANNEL_ID_MAGIC), the terminating null
byte in the "TLS Channel ID signature" string literal will
be included.  Just wanted to confirm this is what you wanted.

If not, you need to subtract one from sizeof(CHANNEL_ID_MAGIC)
to get the string length.

[agl, 2012/05/30 15:28:33]

Yes, I intended to include the NUL byte. It's not terribly important but the
thinking was that we want signature context strings to be prefix free: i.e. we
don't want context strings "abc" and "abcdef" both to be in use because the
latter is valid for the former.

+    unsigned char digest[SHA256_LENGTH];
+    SECItem digest_item;
+    unsigned char signature[64];
+    SECItem signature_item;
+
+    if (!ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn))
+	return SECSuccess;

[wtc, 2012/05/24 23:10:03]

Move these two lines after the lock assertions:

    PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
    PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));

    if (!ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn))
        return SECSuccess;

    PORT_Assert(ss->ssl3.channelID);

[agl, 2012/05/30 15:28:33]

Done.

+
+    PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
+    PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
+    PORT_Assert(ss->ssl3.channelID);
+
+    /* ChannelIDs are always 128 bytes long: 64 bytes of P-256 public key and 64
+     * bytes of ECDSA signature. */
+    static const int CHANNEL_ID_PUBLIC_KEY_LENGTH = 64;
+    static const int CHANNEL_ID_LENGTH = 128;

[wtc, 2012/05/24 23:10:03]

Move these two lines up to the variable declaration section.
(NSS is still being compiled by pre-C99 compilers.)

[agl, 2012/05/30 15:28:33]

Done.

+
+    if (SECKEY_GetPrivateKeyType(ss->ssl3.channelID) != ecKey ||
+	PK11_SignatureLen(ss->ssl3.channelID) != sizeof(signature)) {
+	PORT_SetError(SSL_ERROR_INVALID_CHANNEL_ID_KEY);
+	return SECFailure;
+    }
+
+    rv = ssl3_ComputeHandshakeHashes(ss, ss->ssl3.cwSpec, &hashes, PR_TRUE);

[wtc, 2012/05/24 23:10:03]

Pass 0 as the last argument.  It is only used by SSLv3.

I suggest holding the spec read lock across this
ssl3_ComputeHandshakeHashes call.

Are you sure we should use cwSpec rather than pwSpec?

See the equivalent code in ssl3_SendCertificateVerify:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

[agl, 2012/05/30 15:28:33]

Done.
Done.
Unlike when sending the CertificateVerify, this message is sent under
encryption. Therefore I believe that the pending spec has become the current
spec at this point.

+    if (rv != SECSuccess)
+	goto loser;
+
+    rv = ssl3_AppendHandshakeHeader(ss, encrypted_extensions,
+				    2 + 2 + CHANNEL_ID_LENGTH);
+    if (rv != SECSuccess)
+	return rv;	/* error code set by AppendHandshakeHeader */
+    rv = ssl3_AppendHandshakeNumber(ss, ssl_channel_id_xtn, 2);
+    if (rv != SECSuccess)
+	return rv;	/* error code set by AppendHandshake */
+    rv = ssl3_AppendHandshakeNumber(ss, CHANNEL_ID_LENGTH, 2);
+    if (rv != SECSuccess)
+	return rv;	/* error code set by AppendHandshake */
+
+    spki = SECKEY_EncodeDERSubjectPublicKeyInfo(ss->ssl3.channelIDPub);
+
+    /* This is the ASN.1 prefix for a P-256 public key. Specifically it's:
+     * SEQUENCE
+     *   SEQUENCE
+     *     OID id-ecPublicKey
+     *     OID prime256v1
+     *   BIT STRING, length 66, 0 trailing bits: 0x04
+     *
+     * The 0x04 in the BIT STRING is the prefix for an uncompressed, X9.62
+     * public key. Following that are the two field elements as 32-byte,
+     * big-endian numbers, as required by the Channel ID. */
+    static const unsigned char P256_SPKI_PREFIX[] = {

[wtc, 2012/05/24 23:10:03]

Move this up to the variable declaration section.

[agl, 2012/05/30 15:28:33]

Done.

+	0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a, 0x86,
+	0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a,
+	0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03,
+	0x42, 0x00, 0x04
+    };
+
+    if (spki->len != sizeof(P256_SPKI_PREFIX) + CHANNEL_ID_PUBLIC_KEY_LENGTH ||
+	memcmp(spki->data, P256_SPKI_PREFIX, sizeof(P256_SPKI_PREFIX) != 0)) {
+	PORT_SetError(SSL_ERROR_INVALID_CHANNEL_ID_KEY);
+	return SECFailure;

[wtc, 2012/05/24 23:10:03]

Replace
        return SECFailure;
by
        rv = SECFailure;
        goto loser;

otherwise |spki| is leaked.

[agl, 2012/05/30 15:28:33]

Good catch, thanks!

+    }
+
+    pub_bytes = spki->data + sizeof(P256_SPKI_PREFIX);
+
+    memcpy(signed_data, CHANNEL_ID_MAGIC, sizeof(CHANNEL_ID_MAGIC));
+    memcpy(signed_data + sizeof(CHANNEL_ID_MAGIC), &hashes, sizeof(hashes));
+
+    rv = SHA256_HashBuf(digest, signed_data, sizeof(signed_data));

[wtc, 2012/05/24 23:10:03]

Please use the PK11_ function instead:

  rv = PK11_HashBuf(SEC_OID_SHA256, digest, signed_data, sizeof(signed_data));

SHA256_HashBuf can only be used when we are in bypassPKCS11
mode.

[agl, 2012/05/30 15:28:33]

Done.

+    if (rv != SECSuccess)
+	goto loser;
+
+    digest_item.data = (void*) &digest;

[wtc, 2012/05/24 23:10:03]

BUG: I believe this should be
    digest_item.data = digest;

Similarly line 8949 below should be
    signature_item.data = signature;

Perhaps for an array |digest|, &digest is the same as
&digest[0]?

[agl, 2012/05/30 15:28:33]

Yes, that's the case. However it's surprising so I've removed the &.

+    digest_item.len = sizeof(digest);
+
+    signature_item.data = (void*) &signature;
+    signature_item.len = sizeof(signature);
+
+    rv = PK11_Sign(ss->ssl3.channelID, &signature_item, &digest_item);
+    if (rv != SECSuccess)
+	goto loser;
+
+    rv = ssl3_AppendHandshake(ss, pub_bytes, CHANNEL_ID_PUBLIC_KEY_LENGTH);
+    rv = ssl3_AppendHandshake(ss, signature, sizeof(signature));
+
+    rv = SECSuccess;

[wtc, 2012/05/24 23:10:03]

BUG: remove this line because it suppresses the failure
of the ssl3_AppendHandshake call.

Add error checking:
    rv = ssl3_AppendHandshake(ss, pub_bytes, CHANNEL_ID_PUBLIC_KEY_LENGTH);
    if (rv != SECSuccess)
        goto loser;
    rv = ssl3_AppendHandshake(ss, signature, sizeof(signature));

[agl, 2012/05/30 15:28:33]

Thanks!

+
+loser:
+    if (spki)
+	SECITEM_FreeItem(spki, PR_TRUE);
+
+    return rv;
+}
+
+/* called from ssl3_HandleServerHelloDone
  *             ssl3_HandleClientHello
  *             ssl3_HandleFinished
  */
@@ -9110,6 +9220,10 @@ ssl3_HandleFinished(sslSocket *ss, SSL3Opaque *b, PRUint32 length,
 	    if (rv != SECSuccess) {
 		goto xmit_loser; /* err code was set. */
 	    }
+	    rv = ssl3_SendEncryptedExtensions(ss);

[wtc, 2012/05/24 23:10:03]

This means Channel ID must not be used in renegotiation,
right?

[agl, 2012/05/30 15:28:33]

It did, but that is no longer correct. I've moved the two calls outside of if
(!ss->firstHsDone) so that they are sent on renegotiation.

+	    if (rv != SECSuccess) {
+		goto xmit_loser; /* err code was set. */
+	    }
 	}
 
 	if (IS_DTLS(ss)) {
@@ -10415,6 +10529,11 @@ ssl3_DestroySSL3Info(sslSocket *ss)
     ssl3_DestroyCipherSpec(&ss->ssl3.specs[0], PR_TRUE/*freeSrvName*/);
     ssl3_DestroyCipherSpec(&ss->ssl3.specs[1], PR_TRUE/*freeSrvName*/);
 
+    if (ss->ssl3.channelID)
+	SECKEY_DestroyPrivateKey(ss->ssl3.channelID);
+    if (ss->ssl3.channelIDPub)
+	SECKEY_DestroyPublicKey(ss->ssl3.channelIDPub);

[wtc, 2012/05/24 23:10:03]

I suggest adding these right after line 10499 (where we
destroy ss->ssl3.clientCertChain) because this is similar
to client certificates.

[agl, 2012/05/30 15:28:33]

Done.

+
     /* Destroy the DTLS data */
     if (IS_DTLS(ss)) {
 	if (ss->ssl3.hs.lastMessageFlight) {