Bug 1578238 - Validate tag size in AES_GCM. r=kjacobs,jcj

Validate tag size in AES_GCM.

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

3 files changed, 78 insertions, 49 deletions

diff --git a/gtests/pk11_gtest/pk11_aes_gcm_unittest.cc b/gtests/pk11_gtest/pk11_aes_gcm_unittest.cc
index 9505b43f5..2c58063d4 100644
--- a/gtests/pk11_gtest/pk11_aes_gcm_unittest.cc
+++ b/gtests/pk11_gtest/pk11_aes_gcm_unittest.cc
@@ -37,47 +37,57 @@ class Pkcs11AesGcmTest : public ::testing::TestWithParam<gcm_kat_value> {
     }
 
     // Prepare AEAD params.
-    CK_GCM_PARAMS gcmParams;
-    gcmParams.pIv = iv.data();
-    gcmParams.ulIvLen = iv.size();
-    gcmParams.pAAD = aad.data();
-    gcmParams.ulAADLen = aad.size();
-    gcmParams.ulTagBits = 128;
+    CK_GCM_PARAMS gcm_params;
+    gcm_params.pIv = iv.data();
+    gcm_params.ulIvLen = iv.size();
+    gcm_params.pAAD = aad.data();
+    gcm_params.ulAADLen = aad.size();
+    gcm_params.ulTagBits = 128;
 
-    SECItem params = {siBuffer, reinterpret_cast<unsigned char*>(&gcmParams),
-                      sizeof(gcmParams)};
+    SECItem params = {siBuffer, reinterpret_cast<unsigned char*>(&gcm_params),
+                      sizeof(gcm_params)};
 
     ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
-    SECItem keyItem = {siBuffer, key.data(),
-                       static_cast<unsigned int>(key.size())};
+    SECItem key_item = {siBuffer, key.data(),
+                        static_cast<unsigned int>(key.size())};
 
     // Import key.
-    ScopedPK11SymKey symKey(PK11_ImportSymKey(
-        slot.get(), mech, PK11_OriginUnwrap, CKA_ENCRYPT, &keyItem, nullptr));
-    ASSERT_TRUE(!!symKey) << msg;
+    ScopedPK11SymKey sym_key(PK11_ImportSymKey(
+        slot.get(), mech, PK11_OriginUnwrap, CKA_ENCRYPT, &key_item, nullptr));
+    ASSERT_TRUE(!!sym_key) << msg;
 
     // Encrypt with bogus parameters.
-    unsigned int outputLen = 0;
-    std::vector<uint8_t> output(plaintext.size() + gcmParams.ulTagBits / 8);
-    gcmParams.ulTagBits = 159082344;
+    unsigned int output_len = 0;
+    std::vector<uint8_t> output(plaintext.size() + gcm_params.ulTagBits / 8);
+    // "maxout" must be at least "inlen + tagBytes", or, in this case:
+    // "output.size()" must be at least "plaintext.size() + tagBytes"
+    gcm_params.ulTagBits = 128;
     SECStatus rv =
-        PK11_Encrypt(symKey.get(), mech, &params, output.data(), &outputLen,
-                     output.size(), plaintext.data(), plaintext.size());
+        PK11_Encrypt(sym_key.get(), mech, &params, output.data(), &output_len,
+                     output.size() - 10, plaintext.data(), plaintext.size());
     EXPECT_EQ(SECFailure, rv);
-    EXPECT_EQ(0U, outputLen);
-    gcmParams.ulTagBits = 128;
+    EXPECT_EQ(0U, output_len);
+
+    // The valid values for tag size in AES_GCM are:
+    // 32, 64, 96, 104, 112, 120 and 128.
+    gcm_params.ulTagBits = 110;
+    rv = PK11_Encrypt(sym_key.get(), mech, &params, output.data(), &output_len,
+                      output.size(), plaintext.data(), plaintext.size());
+    EXPECT_EQ(SECFailure, rv);
+    EXPECT_EQ(0U, output_len);
 
     // Encrypt.
-    rv = PK11_Encrypt(symKey.get(), mech, &params, output.data(), &outputLen,
+    gcm_params.ulTagBits = 128;
+    rv = PK11_Encrypt(sym_key.get(), mech, &params, output.data(), &output_len,
                       output.size(), plaintext.data(), plaintext.size());
     if (invalid_iv) {
-      EXPECT_EQ(rv, SECFailure) << msg;
-      EXPECT_EQ(0U, outputLen);
+      EXPECT_EQ(SECFailure, rv) << msg;
+      EXPECT_EQ(0U, output_len);
       return;
     }
-    EXPECT_EQ(rv, SECSuccess) << msg;
+    EXPECT_EQ(SECSuccess, rv) << msg;
 
-    ASSERT_EQ(outputLen, output.size()) << msg;
+    ASSERT_EQ(output_len, output.size()) << msg;
 
     // Check ciphertext and tag.
     if (invalid_ct) {
@@ -87,48 +97,49 @@ class Pkcs11AesGcmTest : public ::testing::TestWithParam<gcm_kat_value> {
     }
 
     // Decrypt.
-    unsigned int decryptedLen = 0;
+    unsigned int decrypted_len = 0;
     // The PK11 AES API is stupid, it expects an explicit IV and thus wants
     // a block more of available output memory.
     std::vector<uint8_t> decrypted(output.size());
-    rv =
-        PK11_Decrypt(symKey.get(), mech, &params, decrypted.data(),
-                     &decryptedLen, decrypted.size(), output.data(), outputLen);
-    EXPECT_EQ(rv, SECSuccess) << msg;
-    ASSERT_EQ(decryptedLen, plaintext.size()) << msg;
+    rv = PK11_Decrypt(sym_key.get(), mech, &params, decrypted.data(),
+                      &decrypted_len, decrypted.size(), output.data(),
+                      output_len);
+    EXPECT_EQ(SECSuccess, rv) << msg;
+    ASSERT_EQ(decrypted_len, plaintext.size()) << msg;
 
     // Check the plaintext.
-    EXPECT_EQ(plaintext, std::vector<uint8_t>(decrypted.begin(),
-                                              decrypted.begin() + decryptedLen))
+    EXPECT_EQ(plaintext,
+              std::vector<uint8_t>(decrypted.begin(),
+                                   decrypted.begin() + decrypted_len))
         << msg;
   }
 
   SECStatus EncryptWithIV(std::vector<uint8_t>& iv) {
     // Generate a random key.
     ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
-    ScopedPK11SymKey symKey(
+    ScopedPK11SymKey sym_key(
         PK11_KeyGen(slot.get(), mech, nullptr, 16, nullptr));
-    EXPECT_TRUE(!!symKey);
+    EXPECT_TRUE(!!sym_key);
 
     std::vector<uint8_t> data(17);
     std::vector<uint8_t> output(33);
     std::vector<uint8_t> aad(0);
 
     // Prepare AEAD params.
-    CK_GCM_PARAMS gcmParams;
-    gcmParams.pIv = iv.data();
-    gcmParams.ulIvLen = iv.size();
-    gcmParams.pAAD = aad.data();
-    gcmParams.ulAADLen = aad.size();
-    gcmParams.ulTagBits = 128;
+    CK_GCM_PARAMS gcm_params;
+    gcm_params.pIv = iv.data();
+    gcm_params.ulIvLen = iv.size();
+    gcm_params.pAAD = aad.data();
+    gcm_params.ulAADLen = aad.size();
+    gcm_params.ulTagBits = 128;
 
-    SECItem params = {siBuffer, reinterpret_cast<unsigned char*>(&gcmParams),
-                      sizeof(gcmParams)};
+    SECItem params = {siBuffer, reinterpret_cast<unsigned char*>(&gcm_params),
+                      sizeof(gcm_params)};
 
     // Try to encrypt.
-    unsigned int outputLen = 0;
-    return PK11_Encrypt(symKey.get(), mech, &params, output.data(), &outputLen,
-                        output.size(), data.data(), data.size());
+    unsigned int output_len = 0;
+    return PK11_Encrypt(sym_key.get(), mech, &params, output.data(),
+                        &output_len, output.size(), data.data(), data.size());
   }
 
   const CK_MECHANISM_TYPE mech = CKM_AES_GCM;
@@ -144,17 +155,17 @@ INSTANTIATE_TEST_CASE_P(WycheproofTestVector, Pkcs11AesGcmTest,
 
 TEST_F(Pkcs11AesGcmTest, ZeroLengthIV) {
   std::vector<uint8_t> iv(0);
-  EXPECT_EQ(EncryptWithIV(iv), SECFailure);
+  EXPECT_EQ(SECFailure, EncryptWithIV(iv));
 }
 
 TEST_F(Pkcs11AesGcmTest, AllZeroIV) {
   std::vector<uint8_t> iv(16, 0);
-  EXPECT_EQ(EncryptWithIV(iv), SECSuccess);
+  EXPECT_EQ(SECSuccess, EncryptWithIV(iv));
 }
 
 TEST_F(Pkcs11AesGcmTest, TwelveByteZeroIV) {
   std::vector<uint8_t> iv(12, 0);
-  EXPECT_EQ(EncryptWithIV(iv), SECSuccess);
+  EXPECT_EQ(SECSuccess, EncryptWithIV(iv));
 }
 
 }  // namespace nss_test
diff --git a/lib/freebl/gcm.c b/lib/freebl/gcm.c
index 9902d79bf..6edf0e8f3 100644
--- a/lib/freebl/gcm.c
+++ b/lib/freebl/gcm.c
@@ -541,6 +541,15 @@ GCM_CreateContext(void *context, freeblCipherFunc cipher,
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return NULL;
     }
+
+    if (gcmParams->ulTagBits != 128 && gcmParams->ulTagBits != 120 &&
+        gcmParams->ulTagBits != 112 && gcmParams->ulTagBits != 104 &&
+        gcmParams->ulTagBits != 96 && gcmParams->ulTagBits != 64 &&
+        gcmParams->ulTagBits != 32) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return NULL;
+    }
+
     gcm = PORT_ZNew(GCMContext);
     if (gcm == NULL) {
         return NULL;
diff --git a/lib/freebl/intel-gcm-wrap.c b/lib/freebl/intel-gcm-wrap.c
index 34d4c1283..7558ffe59 100644
--- a/lib/freebl/intel-gcm-wrap.c
+++ b/lib/freebl/intel-gcm-wrap.c
@@ -62,6 +62,15 @@ intel_AES_GCM_CreateContext(void *context,
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return NULL;
     }
+
+    if (gcmParams->ulTagBits != 128 && gcmParams->ulTagBits != 120 &&
+        gcmParams->ulTagBits != 112 && gcmParams->ulTagBits != 104 &&
+        gcmParams->ulTagBits != 96 && gcmParams->ulTagBits != 64 &&
+        gcmParams->ulTagBits != 32) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return NULL;
+    }
+
     // Limit AADLen in accordance with SP800-38D
     if (sizeof(AAD_whole_len) >= 8 && AAD_whole_len > (1ULL << 61) - 1) {
         PORT_SetError(SEC_ERROR_INPUT_LEN);