Make the AES-GCM cipher suites work in DTLS, by moving the

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c	(revision 218090)
+++ net/third_party/nss/ssl/ssl3con.c	(working copy)
@@ -85,9 +85,8 @@
 static SECStatus ssl3_AESGCMBypass(ssl3KeyMaterial *keys, PRBool doDecrypt,
 				   unsigned char *out, int *outlen, int maxout,
 				   const unsigned char *in, int inlen,
-				   SSL3ContentType type,
-				   SSL3ProtocolVersion version,
-				   SSL3SequenceNumber seq_num);
+				   const unsigned char *additionalData,
+				   int additionalDataLen);
 #endif
 
 #define MAX_SEND_BUF_LENGTH 32000 /* watch for 16-bit integer overflow */
@@ -396,10 +395,10 @@
     {SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA, cipher_3des, mac_sha, kea_rsa_fips},
     {SSL_RSA_FIPS_WITH_DES_CBC_SHA, cipher_des,    mac_sha, kea_rsa_fips},
 
-    {TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_null, kea_dhe_rsa},
-    {TLS_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_null, kea_rsa},
-    {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_null, kea_ecdhe_rsa},
-    {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_null, kea_ecdhe_ecdsa},
+    {TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_dhe_rsa},
+    {TLS_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_rsa},
+    {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_rsa},
+    {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, cipher_aes_128_gcm, mac_aead, kea_ecdhe_ecdsa},
 
 #ifdef NSS_ENABLE_ECC
     {TLS_ECDH_ECDSA_WITH_NULL_SHA,        cipher_null, mac_sha, kea_ecdh_ecdsa},
@@ -474,6 +473,8 @@
 #define mmech_md5_hmac CKM_MD5_HMAC
 #define mmech_sha_hmac CKM_SHA_1_HMAC
 #define mmech_sha256_hmac CKM_SHA256_HMAC
+#define mmech_sha384_hmac CKM_SHA384_HMAC
+#define mmech_sha512_hmac CKM_SHA512_HMAC
 
 static const ssl3MACDef mac_defs[] = { /* indexed by SSL3MACAlgorithm */
     /* pad_size is only used for SSL 3.0 MAC. See RFC 6101 Sec. 5.2.3.1. */
@@ -484,6 +485,9 @@
     {hmac_md5,  mmech_md5_hmac,   0,  MD5_LENGTH },
     {hmac_sha,  mmech_sha_hmac,   0,  SHA1_LENGTH},
     {hmac_sha256, mmech_sha256_hmac, 0, SHA256_LENGTH},
+    {hmac_sha384, mmech_sha384_hmac, 0, SHA384_LENGTH},
+    {hmac_sha512, mmech_sha512_hmac, 0, SHA512_LENGTH},
+    { mac_aead, mmech_null,       0,  0          },

[wtc, 2013/08/17 00:21:15]

This array element for mac_aead needs to exist but won't be used for AEAD
ciphers. So the fields of this array element have invalid/0 values.

 };
 
 /* indexed by SSL3BulkCipher */
@@ -1766,8 +1770,18 @@
     return param;
 }
 
-/* ssl3_BuildRecordPseudoHeader writes the TLS pseudo-header (the data which
- * is included in the MAC) to |out| and returns its length. */
+/* ssl3_BuildRecordPseudoHeader writes the SSL/TLS pseudo-header (the data
+ * which is included in the MAC or AEAD additional data) to |out| and returns
+ * its length. See https://tools.ietf.org/html/rfc5246#section-6.2.3.3 for the
+ * definition of the AEAD additional data.
+ *
+ * TLS pseudo-header includes the record's version field, SSL's doesn't. Which
+ * pseudo-header defintiion to use should be decided based on the version of
+ * the protocol that was negotiated when the spec became current, NOT based on
+ * the version value in the record itself, and the decision is passed to this
+ * function as the |includesVersion| argument. But, the |version| argument
+ * should be the record's version value.
+ */
 static unsigned int
 ssl3_BuildRecordPseudoHeader(unsigned char *out,
 			     SSL3SequenceNumber seq_num,
@@ -1881,29 +1895,18 @@
 	    int maxout,
 	    const unsigned char *in,
 	    int inlen,
-	    SSL3ContentType type,
-	    SSL3ProtocolVersion version,
-	    SSL3SequenceNumber seq_num)
+	    const unsigned char *additionalData,
+	    int additionalDataLen)
 {
     SECItem            param;
     SECStatus          rv = SECFailure;
     unsigned char      nonce[12];
-    unsigned char      additionalData[13];
-    unsigned int       additionalDataLen;
     unsigned int       uOutLen;
     CK_GCM_PARAMS      gcmParams;
 
     static const int   tagSize = 16;
     static const int   explicitNonceLen = 8;
 
-    /* See https://tools.ietf.org/html/rfc5246#section-6.2.3.3 for the
-     * definition of the AEAD additional data. */
-    additionalDataLen = ssl3_BuildRecordPseudoHeader(
-	additionalData, seq_num, type, PR_TRUE /* includes version */,
-	version, PR_FALSE /* not DTLS */,

[wtc, 2013/08/17 00:21:15]

Note that we had to pass PR_FALSE as the |isDTLS| argument here because we
don't have that boolean available here.

-	inlen - (doDecrypt ? explicitNonceLen + tagSize : 0));
-    PORT_Assert(additionalDataLen <= sizeof(additionalData));
-
     /* See https://tools.ietf.org/html/rfc5288#section-3 for details of how the
      * nonce is formed. */
     memcpy(nonce, keys->write_iv, 4);
@@ -1930,7 +1933,7 @@
     param.len = sizeof(gcmParams);
     gcmParams.pIv = nonce;
     gcmParams.ulIvLen = sizeof(nonce);
-    gcmParams.pAAD = additionalData;
+    gcmParams.pAAD = (unsigned char *)additionalData;  /* const cast */
     gcmParams.ulAADLen = additionalDataLen;
     gcmParams.ulTagBits = tagSize * 8;
 
@@ -1955,14 +1958,11 @@
 		  int maxout,
 		  const unsigned char *in,
 		  int inlen,
-		  SSL3ContentType type,
-		  SSL3ProtocolVersion version,
-		  SSL3SequenceNumber seq_num)
+		  const unsigned char *additionalData,
+		  int additionalDataLen)
 {
     SECStatus          rv = SECFailure;
     unsigned char      nonce[12];
-    unsigned char      additionalData[13];
-    unsigned int       additionalDataLen;
     unsigned int       uOutLen;
     AESContext        *cx;
     CK_GCM_PARAMS      gcmParams;
@@ -1970,14 +1970,6 @@
     static const int   tagSize = 16;
     static const int   explicitNonceLen = 8;
 
-    /* See https://tools.ietf.org/html/rfc5246#section-6.2.3.3 for the
-     * definition of the AEAD additional data. */
-    additionalDataLen = ssl3_BuildRecordPseudoHeader(
-	additionalData, seq_num, type, PR_TRUE /* includes version */,
-	version, PR_FALSE /* not DTLS */,
-	inlen - (doDecrypt ? explicitNonceLen + tagSize : 0));
-    PORT_Assert(additionalDataLen <= sizeof(additionalData));
-
     /* See https://tools.ietf.org/html/rfc5288#section-3 for details of how the
      * nonce is formed. */
     PORT_Assert(keys->write_iv_item.len == 4);
@@ -2006,7 +1998,7 @@
 
     gcmParams.pIv = nonce;
     gcmParams.ulIvLen = sizeof(nonce);
-    gcmParams.pAAD = additionalData;
+    gcmParams.pAAD = (unsigned char *)additionalData;  /* const cast */
     gcmParams.ulAADLen = additionalDataLen;
     gcmParams.ulTagBits = tagSize * 8;
 
@@ -2307,10 +2299,8 @@
 ssl3_ComputeRecordMAC(
     ssl3CipherSpec *   spec,
     PRBool             useServerMacKey,
-    PRBool             isDTLS,
-    SSL3ContentType    type,
-    SSL3ProtocolVersion version,
-    SSL3SequenceNumber seq_num,
+    const unsigned char *header,
+    unsigned int       headerLen,
     const SSL3Opaque * input,
     int                inputLength,
     unsigned char *    outbuf,
@@ -2318,22 +2308,8 @@
 {
     const ssl3MACDef * mac_def;
     SECStatus          rv;
-    PRBool             isTLS;
-    unsigned int       tempLen;
-    unsigned char      temp[MAX_MAC_LENGTH];

[wtc, 2013/08/17 00:21:15]

|isTLS| is only used once, so I just inline it.

I moved |tempLen| and |temp| to the block where they are used.

 
-    /* TLS MAC includes the record's version field, SSL's doesn't.
-    ** We decide which MAC defintiion to use based on the version of 
-    ** the protocol that was negotiated when the spec became current,
-    ** NOT based on the version value in the record itself.
-    ** But, we use the record's version value in the computation.
-    */
-    isTLS = spec->version > SSL_LIBRARY_VERSION_3_0;
-    tempLen = ssl3_BuildRecordPseudoHeader(temp, seq_num, type, isTLS,
-					   version, isDTLS, inputLength);
-    PORT_Assert(tempLen <= sizeof(temp));
-
-    PRINT_BUF(95, (NULL, "frag hash1: temp", temp, tempLen));
+    PRINT_BUF(95, (NULL, "frag hash1: header", header, headerLen));
     PRINT_BUF(95, (NULL, "frag hash1: input", input, inputLength));
 
     mac_def = spec->mac_def;
@@ -2378,7 +2354,10 @@
 	    return SECFailure;
 	}
 
-	if (!isTLS) {
+	if (spec->version <= SSL_LIBRARY_VERSION_3_0) {
+	    unsigned int tempLen;
+	    unsigned char temp[MAX_MAC_LENGTH];
+
 	    /* compute "inner" part of SSL3 MAC */
 	    hashObj->begin(write_mac_context);
 	    if (useServerMacKey)
@@ -2390,7 +2369,7 @@
 				spec->client.write_mac_key_item.data,
 				spec->client.write_mac_key_item.len);
 	    hashObj->update(write_mac_context, mac_pad_1, pad_bytes);
-	    hashObj->update(write_mac_context, temp,  tempLen);
+	    hashObj->update(write_mac_context, header, headerLen);
 	    hashObj->update(write_mac_context, input, inputLength);
 	    hashObj->end(write_mac_context,    temp, &tempLen, sizeof temp);
 
@@ -2421,7 +2400,7 @@
 	    }
 	    if (rv == SECSuccess) {
 		HMAC_Begin(cx);
-		HMAC_Update(cx, temp, tempLen);
+		HMAC_Update(cx, header, headerLen);
 		HMAC_Update(cx, input, inputLength);
 		rv = HMAC_Finish(cx, outbuf, outLength, spec->mac_size);
 		HMAC_Destroy(cx, PR_FALSE);
@@ -2435,7 +2414,7 @@
 	    (useServerMacKey ? spec->server.write_mac_context
 	                     : spec->client.write_mac_context);
 	rv  = PK11_DigestBegin(mac_context);
-	rv |= PK11_DigestOp(mac_context, temp, tempLen);
+	rv |= PK11_DigestOp(mac_context, header, headerLen);
 	rv |= PK11_DigestOp(mac_context, input, inputLength);
 	rv |= PK11_DigestFinal(mac_context, outbuf, outLength, spec->mac_size);
     }
@@ -2475,10 +2454,8 @@
 ssl3_ComputeRecordMACConstantTime(
     ssl3CipherSpec *   spec,
     PRBool             useServerMacKey,
-    PRBool             isDTLS,
-    SSL3ContentType    type,
-    SSL3ProtocolVersion version,
-    SSL3SequenceNumber seq_num,
+    const unsigned char *header,
+    unsigned int       headerLen,
     const SSL3Opaque * input,
     int                inputLen,
     int                originalLen,
@@ -2490,9 +2467,7 @@
     PK11Context *                mac_context;
     SECItem                      param;
     SECStatus                    rv;
-    unsigned char                header[13];
     PK11SymKey *                 key;
-    int                          recordLength;
 
     PORT_Assert(inputLen >= spec->mac_size);
     PORT_Assert(originalLen >= inputLen);
@@ -2508,42 +2483,15 @@
 	return SECSuccess;
     }
 
-    header[0] = (unsigned char)(seq_num.high >> 24);
-    header[1] = (unsigned char)(seq_num.high >> 16);
-    header[2] = (unsigned char)(seq_num.high >>  8);
-    header[3] = (unsigned char)(seq_num.high >>  0);
-    header[4] = (unsigned char)(seq_num.low  >> 24);
-    header[5] = (unsigned char)(seq_num.low  >> 16);
-    header[6] = (unsigned char)(seq_num.low  >>  8);
-    header[7] = (unsigned char)(seq_num.low  >>  0);
-    header[8] = type;
-
     macType = CKM_NSS_HMAC_CONSTANT_TIME;
-    recordLength = inputLen - spec->mac_size;
     if (spec->version <= SSL_LIBRARY_VERSION_3_0) {
 	macType = CKM_NSS_SSL3_MAC_CONSTANT_TIME;
-	header[9] = recordLength >> 8;
-	header[10] = recordLength;
-	params.ulHeaderLen = 11;
-    } else {
-	if (isDTLS) {
-	    SSL3ProtocolVersion dtls_version;
-
-	    dtls_version = dtls_TLSVersionToDTLSVersion(version);
-	    header[9] = dtls_version >> 8;
-	    header[10] = dtls_version;
-	} else {
-	    header[9] = version >> 8;
-	    header[10] = version;
-	}
-	header[11] = recordLength >> 8;
-	header[12] = recordLength;
-	params.ulHeaderLen = 13;
     }
 
     params.macAlg = spec->mac_def->mmech;
     params.ulBodyTotalLen = originalLen;
-    params.pHeader = header;
+    params.pHeader = (unsigned char *) header;  /* const cast */
+    params.ulHeaderLen = headerLen;
 
     param.data = (unsigned char*) &params;
     param.len = sizeof(params);
@@ -2576,9 +2524,8 @@
     /* ssl3_ComputeRecordMAC expects the MAC to have been removed from the
      * length already. */
     inputLen -= spec->mac_size;
-    return ssl3_ComputeRecordMAC(spec, useServerMacKey, isDTLS, type,
-				 version, seq_num, input, inputLen,
-				 outbuf, outLen);
+    return ssl3_ComputeRecordMAC(spec, useServerMacKey, header, headerLen,
+				 input, inputLen, outbuf, outLen);
 }
 
 static PRBool
@@ -2630,6 +2577,8 @@
     PRUint16                  headerLen;
     int                       ivLen = 0;
     int                       cipherBytes = 0;
+    unsigned char             pseudoHeader[13];
+    unsigned int              pseudoHeaderLen;
 
     cipher_def = cwSpec->cipher_def;
     headerLen = isDTLS ? DTLS_RECORD_HEADER_LENGTH : SSL3_RECORD_HEADER_LENGTH;
@@ -2675,6 +2624,11 @@
 	contentLen = outlen;
     }
 
+    pseudoHeaderLen = ssl3_BuildRecordPseudoHeader(pseudoHeader,

[Ryan Sleevi, 2013/08/17 02:00:23]

This is really weird indentation of that first param. Just $.02

[wtc, 2013/08/19 23:31:56]

Done.

+	cwSpec->write_seq_num, type,
+	cwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_0, cwSpec->version,
+	isDTLS, contentLen);
+    PORT_Assert(pseudoHeaderLen <= sizeof(pseudoHeader));
     if (cipher_def->type == type_aead) {
 	const int nonceLen = cipher_def->explicit_nonce_size;
 	const int tagLen = cipher_def->tag_size;
@@ -2692,7 +2646,7 @@
 		&cipherBytes,                               /* out len */
 		wrBuf->space - headerLen,                   /* max out */
 		pIn, contentLen,                            /* input   */
-		type, cwSpec->version, cwSpec->write_seq_num);
+		pseudoHeader, pseudoHeaderLen);
 	if (rv != SECSuccess) {
 	    PORT_SetError(SSL_ERROR_ENCRYPTION_FAILURE);
 	    return SECFailure;
@@ -2701,8 +2655,8 @@
 	/*
 	 * Add the MAC
 	 */
-	rv = ssl3_ComputeRecordMAC( cwSpec, isServer, isDTLS,
-	    type, cwSpec->version, cwSpec->write_seq_num, pIn, contentLen,
+	rv = ssl3_ComputeRecordMAC(cwSpec, isServer,
+	    pseudoHeader, pseudoHeaderLen, pIn, contentLen,
 	    wrBuf->buf + headerLen + ivLen + contentLen, &macLen);
 	if (rv != SECSuccess) {
 	    ssl_MapLowLevelError(SSL_ERROR_MAC_COMPUTATION_FAILURE);
@@ -11407,6 +11361,8 @@
     unsigned int         originalLen = 0;
     unsigned int         good;
     unsigned int         minLength;
+    unsigned char        header[13];
+    unsigned int         headerLen;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
 
@@ -11560,6 +11516,14 @@
 
     rType = cText->type;
     if (cipher_def->type == type_aead) {
+	/* XXX Is this true for all AEAD ciphers? */

[Ryan Sleevi, 2013/08/17 02:00:23]

No.

This is covered in http://tools.ietf.org/html/rfc5116#section-4 pp12

   Each AEAD algorithm MUST provide a description relating the length of
   the plaintext to that of the ciphertext.  This relation MUST NOT
   depend on external parameters, such as an authentication strength
   parameter (e.g., authentication tag length).  That sort of dependence
   would complicate the use of the algorithm by creating a situation in
   which the information from the AEAD registry was not sufficient to
   ensure interoperability.


So the existing ciphers:
http://tools.ietf.org/html/rfc5116#section-5.1
   An AEAD_AES_128_GCM ciphertext is exactly 16 octets longer than its
   corresponding plaintext.

http://tools.ietf.org/html/rfc5116#section-5.2
   This algorithm is identical to AEAD_AES_128_GCM, but with the
   following differences:

http://tools.ietf.org/html/rfc5116#section-5.3
   An AEAD_AES_128_CCM ciphertext is exactly 16 octets longer than its
   corresponding plaintext.

http://tools.ietf.org/html/rfc5116#section-5.4
   This algorithm is identical to AEAD_AES_128_CCM, but with the
   following differences:


Also considering the proposed
http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-02

see
http://tools.ietf.org/html/draft-mcgrew-aead-aes-cbc-hmac-sha2-02#section-2.3

Note that because CBC is used, there is padding.

[wtc, 2013/08/19 23:31:56]

Done. Thank you for the research. I updated the comment.

+	unsigned int decryptedLen =
+	    cText->buf->len - cipher_def->explicit_nonce_size -
+	    cipher_def->tag_size;
+	headerLen = ssl3_BuildRecordPseudoHeader(header,
+	    IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num, rType,
+	    isTLS, cText->version, IS_DTLS(ss), decryptedLen);
+	PORT_Assert(headerLen <= sizeof(header));
 	rv = crSpec->aead(
 		ss->sec.isServer ? &crSpec->client : &crSpec->server,
 		PR_TRUE,                          /* do decrypt */
@@ -11568,9 +11532,7 @@
 		plaintext->space,                 /* maxout */
 		cText->buf->buf,                  /* in */
 		cText->buf->len,                  /* inlen */
-		rType,                            /* record type */
-		cText->version,
-		IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num);
+		header, headerLen);
 	if (rv != SECSuccess) {
 	    good = 0;
 	}
@@ -11597,7 +11559,7 @@
 	    const unsigned int blockSize = cipher_def->block_size;
 	    const unsigned int macSize = crSpec->mac_size;
 
-	    if (crSpec->version <= SSL_LIBRARY_VERSION_3_0) {
+	    if (!isTLS) {
 		good &= SECStatusToMask(ssl_RemoveSSLv3CBCPadding(
 			    plaintext, blockSize, macSize));
 	    } else {
@@ -11607,11 +11569,14 @@
 	}
 
 	/* compute the MAC */
+	headerLen = ssl3_BuildRecordPseudoHeader(header,
+	    IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num, rType,
+	    isTLS, cText->version, IS_DTLS(ss),
+	    plaintext->len - crSpec->mac_size);
+	PORT_Assert(headerLen <= sizeof(header));
 	if (cipher_def->type == type_block) {
 	    rv = ssl3_ComputeRecordMACConstantTime(
-		crSpec, (PRBool)(!ss->sec.isServer),
-		IS_DTLS(ss), rType, cText->version,
-		IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num,
+		crSpec, (PRBool)(!ss->sec.isServer), header, headerLen,
 		plaintext->buf, plaintext->len, originalLen,
 		hash, &hashBytes);
 
@@ -11629,11 +11594,8 @@
 	    plaintext->len -= crSpec->mac_size;
 
 	    rv = ssl3_ComputeRecordMAC(
-		crSpec, (PRBool)(!ss->sec.isServer),
-		IS_DTLS(ss), rType, cText->version,
-		IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num,
-		plaintext->buf, plaintext->len,
-		hash, &hashBytes);
+		crSpec, (PRBool)(!ss->sec.isServer), header, headerLen,
+		plaintext->buf, plaintext->len, hash, &hashBytes);
 
 	    /* We can read the MAC directly from the record because its location
 	     * is public when a stream cipher is used. */