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

6 files changed, 95 insertions, 40 deletions

diff --git a/gtests/ssl_gtest/ssl_primitive_unittest.cc b/gtests/ssl_gtest/ssl_primitive_unittest.cc
index b46a3d97f..7966e063d 100644
--- a/gtests/ssl_gtest/ssl_primitive_unittest.cc
+++ b/gtests/ssl_gtest/ssl_primitive_unittest.cc
@@ -128,33 +128,62 @@ static const uint8_t kCiphertextChaCha20Poly1305[] = {
     0x4e, 0x89, 0x2c, 0xfa, 0xfc, 0x8c, 0x40, 0x55, 0x6d, 0x7e,
     0x99, 0xac, 0x8e, 0x54, 0x58, 0xb1, 0x18, 0xd2, 0x66, 0x22};
 
+TEST_F(AeadTest, AeadBadVersion) {
+  SSLAeadContext *ctx = nullptr;
+  ASSERT_EQ(SECFailure,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_2, TLS_AES_128_GCM_SHA256,
+                         secret_.get(), kLabel, strlen(kLabel), &ctx));
+  EXPECT_EQ(nullptr, ctx);
+}
+
+TEST_F(AeadTest, AeadUnsupportedCipher) {
+  SSLAeadContext *ctx = nullptr;
+  ASSERT_EQ(SECFailure,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_RSA_WITH_NULL_MD5,
+                         secret_.get(), kLabel, strlen(kLabel), &ctx));
+  EXPECT_EQ(nullptr, ctx);
+}
+
+TEST_F(AeadTest, AeadOlderCipher) {
+  SSLAeadContext *ctx = nullptr;
+  ASSERT_EQ(
+      SECFailure,
+      SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_RSA_WITH_AES_128_CBC_SHA,
+                   secret_.get(), kLabel, strlen(kLabel), &ctx));
+  EXPECT_EQ(nullptr, ctx);
+}
+
 TEST_F(AeadTest, AeadNoLabel) {
   SSLAeadContext *ctx = nullptr;
-  ASSERT_EQ(SECFailure, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
-                                     nullptr, 12, &ctx));
+  ASSERT_EQ(SECFailure,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
+                         secret_.get(), nullptr, 12, &ctx));
   EXPECT_EQ(nullptr, ctx);
 }
 
 TEST_F(AeadTest, AeadLongLabel) {
   SSLAeadContext *ctx = nullptr;
   ASSERT_EQ(SECFailure,
-            SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256, "", 254, &ctx));
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
+                         secret_.get(), "", 254, &ctx));
   EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError());
   EXPECT_EQ(nullptr, ctx);
 }
 
 TEST_F(AeadTest, AeadNoPointer) {
   SSLAeadContext *ctx = nullptr;
-  ASSERT_EQ(SECFailure, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
-                                     kLabel, strlen(kLabel), nullptr));
+  ASSERT_EQ(SECFailure,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
+                         secret_.get(), kLabel, strlen(kLabel), nullptr));
   EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError());
   EXPECT_EQ(nullptr, ctx);
 }
 
 TEST_F(AeadTest, AeadAes128Gcm) {
   SSLAeadContext *ctxInit;
-  ASSERT_EQ(SECSuccess, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
-                                     kLabel, strlen(kLabel), &ctxInit));
+  ASSERT_EQ(SECSuccess,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
+                         secret_.get(), kLabel, strlen(kLabel), &ctxInit));
   ScopedSSLAeadContext ctx(ctxInit);
   EXPECT_NE(nullptr, ctx);
 
@@ -163,8 +192,9 @@ TEST_F(AeadTest, AeadAes128Gcm) {
 
 TEST_F(AeadTest, AeadAes256Gcm) {
   SSLAeadContext *ctxInit;
-  ASSERT_EQ(SECSuccess, SSL_MakeAead(secret_.get(), TLS_AES_256_GCM_SHA384,
-                                     kLabel, strlen(kLabel), &ctxInit));
+  ASSERT_EQ(SECSuccess,
+            SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_256_GCM_SHA384,
+                         secret_.get(), kLabel, strlen(kLabel), &ctxInit));
   ScopedSSLAeadContext ctx(ctxInit);
   EXPECT_NE(nullptr, ctx);
 
@@ -173,9 +203,10 @@ TEST_F(AeadTest, AeadAes256Gcm) {
 
 TEST_F(AeadTest, AeadChaCha20Poly1305) {
   SSLAeadContext *ctxInit;
-  ASSERT_EQ(SECSuccess,
-            SSL_MakeAead(secret_.get(), TLS_CHACHA20_POLY1305_SHA256, kLabel,
-                         strlen(kLabel), &ctxInit));
+  ASSERT_EQ(
+      SECSuccess,
+      SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_CHACHA20_POLY1305_SHA256,
+                   secret_.get(), kLabel, strlen(kLabel), &ctxInit));
   ScopedSSLAeadContext ctx(ctxInit);
   EXPECT_NE(nullptr, ctx);
 
diff --git a/gtests/ssl_gtest/tls_hkdf_unittest.cc b/gtests/ssl_gtest/tls_hkdf_unittest.cc
index 2e82baafd..739e11409 100644
--- a/gtests/ssl_gtest/tls_hkdf_unittest.cc
+++ b/gtests/ssl_gtest/tls_hkdf_unittest.cc
@@ -58,7 +58,7 @@ const size_t kHashLength[] = {
 
 size_t GetHashLength(SSLHashType hash) {
   size_t i = static_cast<size_t>(hash);
-  if (i >= 0 && i < PR_ARRAY_SIZE(kHashLength)) {
+  if (i < PR_ARRAY_SIZE(kHashLength)) {
     return kHashLength[i];
   }
   ADD_FAILURE() << "Unknown hash: " << hash;
diff --git a/gtests/ssl_gtest/tls_protect.cc b/gtests/ssl_gtest/tls_protect.cc
index 1e1004b5e..de91982f7 100644
--- a/gtests/ssl_gtest/tls_protect.cc
+++ b/gtests/ssl_gtest/tls_protect.cc
@@ -5,6 +5,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "tls_protect.h"
+#include "sslproto.h"
 #include "tls_filter.h"
 
 namespace nss_test {
@@ -25,7 +26,8 @@ TlsCipherSpec::TlsCipherSpec(bool dtls, uint16_t epoc)
 bool TlsCipherSpec::SetKeys(SSLCipherSuiteInfo* cipherinfo,
                             PK11SymKey* secret) {
   SSLAeadContext* ctx;
-  SECStatus rv = SSL_MakeAead(secret, cipherinfo->cipherSuite, "",
+  SECStatus rv = SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3,
+                              cipherinfo->cipherSuite, secret, "",
                               0,  // Use the default labels.
                               &ctx);
   if (rv != SECSuccess) {
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;
     }