Bug 1528175 - Include version in SSL_MakeAead arguments, r=ekr

Summary:
We should really include version with the ciphersuite in case we decide to reuse the ciphersuite definitions for TLS 1.4, but also change the way they operate.

I also included a fixup for the clang4 build error from the last set.

Reviewers: ekr

Tags: #secure-revision

Bug #: 1528175

Differential Revision: https://phabricator.services.mozilla.com/D20761

3 files changed, 48 insertions, 26 deletions

diff --git a/lib/ssl/sslexp.h b/lib/ssl/sslexp.h
index 447547d2f..44e8459a6 100644
--- a/lib/ssl/sslexp.h
+++ b/lib/ssl/sslexp.h
@@ -642,13 +642,16 @@ typedef SECStatus(PR_CALLBACK *SSLRecordWriteCallback)(
  */
 typedef struct SSLAeadContextStr SSLAeadContext;
 
-#define SSL_MakeAead(secret, cipherSuite, labelPrefix, labelPrefixLen, ctx) \
-    SSL_EXPERIMENTAL_API("SSL_MakeAead",                                    \
-                         (PK11SymKey * _secret, PRUint16 _cipherSuite,      \
-                          const char *_labelPrefix,                         \
-                          unsigned int _labelPrefixLen,                     \
-                          SSLAeadContext **_ctx),                           \
-                         (secret, cipherSuite, labelPrefix, labelPrefixLen, ctx))
+#define SSL_MakeAead(version, cipherSuite, secret,                  \
+                     labelPrefix, labelPrefixLen, ctx)              \
+    SSL_EXPERIMENTAL_API("SSL_MakeAead",                            \
+                         (PRUint16 _version, PRUint16 _cipherSuite, \
+                          PK11SymKey * _secret,                     \
+                          const char *_labelPrefix,                 \
+                          unsigned int _labelPrefixLen,             \
+                          SSLAeadContext **_ctx),                   \
+                         (version, cipherSuite, secret,             \
+                          labelPrefix, labelPrefixLen, ctx))
 
 #define SSL_AeadEncrypt(ctx, counter, aad, aadLen, in, inLen,            \
                         output, outputLen, maxOutputLen)                 \
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 68abbdfa1..076dd77d1 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1760,7 +1760,7 @@ SECStatus SSLExp_GetCurrentEpoch(PRFileDesc *fd, PRUint16 *readEpoch,
 
 #define SSLResumptionTokenVersion 2
 
-SECStatus SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
+SECStatus SSLExp_MakeAead(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *secret,
                           const char *labelPrefix, unsigned int labelPrefixLen,
                           SSLAeadContext **ctx);
 SECStatus SSLExp_DestroyAead(SSLAeadContext *ctx);
diff --git a/lib/ssl/sslprimitive.c b/lib/ssl/sslprimitive.c
index 72caf962f..c1906b518 100644
--- a/lib/ssl/sslprimitive.c
+++ b/lib/ssl/sslprimitive.c
@@ -23,8 +23,34 @@ struct SSLAeadContextStr {
     ssl3KeyMaterial keys;
 };
 
+static SECStatus
+tls13_GetHashAndCipher(PRUint16 version, PRUint16 cipherSuite,
+                       SSLHashType *hash, const ssl3BulkCipherDef **cipher)
+{
+    if (version < SSL_LIBRARY_VERSION_TLS_1_3) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
+    // Lookup and check the suite.
+    SSLVersionRange vrange = { version, version };
+    if (!ssl3_CipherSuiteAllowedForVersionRange(cipherSuite, &vrange)) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+    const ssl3CipherSuiteDef *suiteDef = ssl_LookupCipherSuiteDef(cipherSuite);
+    const ssl3BulkCipherDef *cipherDef = ssl_GetBulkCipherDef(suiteDef);
+    if (cipherDef->type != type_aead) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+    *hash = suiteDef->prf_hash;
+    *cipher = cipherDef;
+    return SECSuccess;
+}
+
 SECStatus
-SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
+SSLExp_MakeAead(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *secret,
                 const char *labelPrefix, unsigned int labelPrefixLen,
                 SSLAeadContext **ctx)
 {
@@ -41,20 +67,13 @@ SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
         goto loser;
     }
 
-    // Lookup and check the suite.
-    SSLVersionRange tls13 = { SSL_LIBRARY_VERSION_TLS_1_3,
-                              SSL_LIBRARY_VERSION_TLS_1_3 };
-    if (!ssl3_CipherSuiteAllowedForVersionRange(cipherSuite, &tls13)) {
-        PORT_SetError(SEC_ERROR_INVALID_ARGS);
-        goto loser;
-    }
-    const ssl3CipherSuiteDef *suiteDef = ssl_LookupCipherSuiteDef(cipherSuite);
-    const ssl3BulkCipherDef *cipher = ssl_GetBulkCipherDef(suiteDef);
-    if (cipher->type != type_aead) {
-        PORT_SetError(SEC_ERROR_INVALID_ARGS);
-        goto loser;
+    SSLHashType hash;
+    const ssl3BulkCipherDef *cipher;
+    SECStatus rv = tls13_GetHashAndCipher(version, cipherSuite,
+                                          &hash, &cipher);
+    if (rv != SECSuccess) {
+        goto loser; /* Code already set. */
     }
-    SSLHashType hash = suiteDef->prf_hash;
 
     out = PORT_ZNew(SSLAeadContext);
     if (out == NULL) {
@@ -66,10 +85,10 @@ SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
     memcpy(label + labelPrefixLen, ivSuffix, strlen(ivSuffix));
     unsigned int labelLen = labelPrefixLen + strlen(ivSuffix);
     unsigned int ivLen = cipher->iv_size + cipher->explicit_nonce_size;
-    SECStatus rv = tls13_HkdfExpandLabelRaw(secret, hash,
-                                            NULL, 0, // Handshake hash.
-                                            label, labelLen,
-                                            out->keys.iv, ivLen);
+    rv = tls13_HkdfExpandLabelRaw(secret, hash,
+                                  NULL, 0, // Handshake hash.
+                                  label, labelLen,
+                                  out->keys.iv, ivLen);
     if (rv != SECSuccess) {
         goto loser;
     }