Bug 1296986, Disable parameter unsafeAllowMissingParameters in _SGN_VerifyPKCS1DigestInfo, based on a patch contributed by David Benjamin (davidben@google.com), r=fkiefer

7 files changed, 145 insertions, 41 deletions

diff --git a/coreconf/config.mk b/coreconf/config.mk
index b62f6cef4..60a08411e 100644
--- a/coreconf/config.mk
+++ b/coreconf/config.mk
@@ -185,6 +185,10 @@ ifdef NSS_SEED_ONLY_DEV_URANDOM
 DEFINES += -DSEED_ONLY_DEV_URANDOM
 endif
 
+ifdef NSS_PKCS1_AllowMissingParameters
+DEFINES += -DNSS_PKCS1_AllowMissingParameters
+endif
+
 # Avoid building object leak test code for optimized library
 ifndef BUILD_OPT
 ifdef PKIX_OBJECT_LEAK_TEST
diff --git a/gtests/pk11_gtest/manifest.mn b/gtests/pk11_gtest/manifest.mn
index a3dff9d10..ea7b43a2b 100644
--- a/gtests/pk11_gtest/manifest.mn
+++ b/gtests/pk11_gtest/manifest.mn
@@ -16,6 +16,7 @@ CPPSRCS = \
       pk11_pbkdf2_unittest.cc \
       pk11_prf_unittest.cc \
       pk11_prng_unittest.cc \
+      pk11_rsapkcs1_unittest.cc \
       pk11_rsapss_unittest.cc \
       pk11_der_private_key_import_unittest.cc \
       $(NULL)
diff --git a/gtests/pk11_gtest/pk11_gtest.gyp b/gtests/pk11_gtest/pk11_gtest.gyp
index 076b4d37f..845093477 100644
--- a/gtests/pk11_gtest/pk11_gtest.gyp
+++ b/gtests/pk11_gtest/pk11_gtest.gyp
@@ -20,6 +20,7 @@
         'pk11_pbkdf2_unittest.cc',
         'pk11_prf_unittest.cc',
         'pk11_prng_unittest.cc',
+        'pk11_rsapkcs1_unittest.cc',
         'pk11_rsapss_unittest.cc',
         'pk11_der_private_key_import_unittest.cc',
         '<(DEPTH)/gtests/common/gtests.cc'
diff --git a/gtests/pk11_gtest/pk11_rsapkcs1_unittest.cc b/gtests/pk11_gtest/pk11_rsapkcs1_unittest.cc
new file mode 100644
index 000000000..862a7434e
--- /dev/null
+++ b/gtests/pk11_gtest/pk11_rsapkcs1_unittest.cc
@@ -0,0 +1,109 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <stdint.h>
+#include "cryptohi.h"
+#include "nss.h"
+#include "pk11pub.h"
+
+#include "gtest/gtest.h"
+#include "scoped_ptrs.h"
+#include "cpputil.h"
+
+namespace nss_test {
+
+// Test that the RSASSA-PKCS1-v1_5 implementation enforces the missing NULL
+// parameter.
+TEST(RsaPkcs1Test, RequireNullParameter) {
+  // kSpki is an RSA public key in an X.509 SubjectPublicKeyInfo.
+  const uint8_t kSpki[] = {
+      0x30, 0x81, 0x9f, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7,
+      0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x81, 0x8d, 0x00, 0x30, 0x81,
+      0x89, 0x02, 0x81, 0x81, 0x00, 0xf8, 0xb8, 0x6c, 0x83, 0xb4, 0xbc, 0xd9,
+      0xa8, 0x57, 0xc0, 0xa5, 0xb4, 0x59, 0x76, 0x8c, 0x54, 0x1d, 0x79, 0xeb,
+      0x22, 0x52, 0x04, 0x7e, 0xd3, 0x37, 0xeb, 0x41, 0xfd, 0x83, 0xf9, 0xf0,
+      0xa6, 0x85, 0x15, 0x34, 0x75, 0x71, 0x5a, 0x84, 0xa8, 0x3c, 0xd2, 0xef,
+      0x5a, 0x4e, 0xd3, 0xde, 0x97, 0x8a, 0xdd, 0xff, 0xbb, 0xcf, 0x0a, 0xaa,
+      0x86, 0x92, 0xbe, 0xb8, 0x50, 0xe4, 0xcd, 0x6f, 0x80, 0x33, 0x30, 0x76,
+      0x13, 0x8f, 0xca, 0x7b, 0xdc, 0xec, 0x5a, 0xca, 0x63, 0xc7, 0x03, 0x25,
+      0xef, 0xa8, 0x8a, 0x83, 0x58, 0x76, 0x20, 0xfa, 0x16, 0x77, 0xd7, 0x79,
+      0x92, 0x63, 0x01, 0x48, 0x1a, 0xd8, 0x7b, 0x67, 0xf1, 0x52, 0x55, 0x49,
+      0x4e, 0xd6, 0x6e, 0x4a, 0x5c, 0xd7, 0x7a, 0x37, 0x36, 0x0c, 0xde, 0xdd,
+      0x8f, 0x44, 0xe8, 0xc2, 0xa7, 0x2c, 0x2b, 0xb5, 0xaf, 0x64, 0x4b, 0x61,
+      0x07, 0x02, 0x03, 0x01, 0x00, 0x01,
+  };
+  // kHash is the SHA-256 hash of {1,2,3,4}.
+  const uint8_t kHash[] = {
+      0x9f, 0x64, 0xa7, 0x47, 0xe1, 0xb9, 0x7f, 0x13, 0x1f, 0xab, 0xb6,
+      0xb4, 0x47, 0x29, 0x6c, 0x9b, 0x6f, 0x02, 0x01, 0xe7, 0x9f, 0xb3,
+      0xc5, 0x35, 0x6e, 0x6c, 0x77, 0xe8, 0x9b, 0x6a, 0x80, 0x6a,
+  };
+  // kSignature is the signature of kHash with RSASSA-PKCS1-v1_5.
+  const uint8_t kSignature[] = {
+      0xa5, 0xf0, 0x8a, 0x47, 0x5d, 0x3c, 0xb3, 0xcc, 0xa9, 0x79, 0xaf, 0x4d,
+      0x8c, 0xae, 0x4c, 0x14, 0xef, 0xc2, 0x0b, 0x34, 0x36, 0xde, 0xf4, 0x3e,
+      0x3d, 0xbb, 0x4a, 0x60, 0x5c, 0xc8, 0x91, 0x28, 0xda, 0xfb, 0x7e, 0x04,
+      0x96, 0x7e, 0x63, 0x13, 0x90, 0xce, 0xb9, 0xb4, 0x62, 0x7a, 0xfd, 0x09,
+      0x3d, 0xc7, 0x67, 0x78, 0x54, 0x04, 0xeb, 0x52, 0x62, 0x6e, 0x24, 0x67,
+      0xb4, 0x40, 0xfc, 0x57, 0x62, 0xc6, 0xf1, 0x67, 0xc1, 0x97, 0x8f, 0x6a,
+      0xa8, 0xae, 0x44, 0x46, 0x5e, 0xab, 0x67, 0x17, 0x53, 0x19, 0x3a, 0xda,
+      0x5a, 0xc8, 0x16, 0x3e, 0x86, 0xd5, 0xc5, 0x71, 0x2f, 0xfc, 0x23, 0x48,
+      0xd9, 0x0b, 0x13, 0xdd, 0x7b, 0x5a, 0x25, 0x79, 0xef, 0xa5, 0x7b, 0x04,
+      0xed, 0x44, 0xf6, 0x18, 0x55, 0xe4, 0x0a, 0xe9, 0x57, 0x79, 0x5d, 0xd7,
+      0x55, 0xa7, 0xab, 0x45, 0x02, 0x97, 0x60, 0x42,
+  };
+  // kSignature is an invalid signature of kHash with RSASSA-PKCS1-v1_5 with the
+  // NULL parameter omitted.
+  const uint8_t kSignatureInvalid[] = {
+      0x71, 0x6c, 0x24, 0x4e, 0xc9, 0x9b, 0x19, 0xc7, 0x49, 0x29, 0xb8, 0xd4,
+      0xfb, 0x26, 0x23, 0xc0, 0x96, 0x18, 0xcd, 0x1e, 0x60, 0xe8, 0x88, 0x94,
+      0x8c, 0x59, 0xfb, 0x58, 0x5c, 0x61, 0x58, 0x7a, 0xae, 0xcc, 0xeb, 0xee,
+      0x1e, 0x85, 0x7d, 0x83, 0xa9, 0xdc, 0x6f, 0x4c, 0x34, 0x5c, 0xcb, 0xd9,
+      0xde, 0x58, 0x76, 0xdf, 0x1f, 0x5e, 0xd4, 0x57, 0x5b, 0xeb, 0xaf, 0x4f,
+      0x7a, 0xa7, 0x6b, 0x21, 0xf1, 0x0a, 0x96, 0x78, 0xc7, 0xa8, 0x02, 0x7a,
+      0xc2, 0x06, 0xd3, 0x18, 0x79, 0x72, 0x6b, 0xfe, 0x2d, 0xec, 0xd8, 0x8e,
+      0x98, 0x86, 0x89, 0xf4, 0x67, 0x14, 0x2b, 0xac, 0x6d, 0xd7, 0x04, 0xd8,
+      0xab, 0x05, 0xe6, 0x51, 0xf6, 0xee, 0x58, 0x63, 0xef, 0x6a, 0x3e, 0x89,
+      0x99, 0x2a, 0x1c, 0x10, 0xc2, 0xd0, 0x41, 0x9e, 0x1e, 0x9a, 0x9a, 0x57,
+      0x32, 0x0f, 0x49, 0xb4, 0x57, 0x37, 0xa4, 0x26,
+  };
+
+  // The test vectors may be verified with:
+  //
+  // openssl rsautl -keyform der -pubin -inkey spki.bin -in sig.bin | der2ascii
+  // openssl rsautl -keyform der -pubin -inkey spki.bin -in sig2.bin | der2ascii
+
+  // Import public key.
+  SECItem spkiItem = {siBuffer, toUcharPtr(kSpki), sizeof(kSpki)};
+  ScopedCERTSubjectPublicKeyInfo certSpki(
+      SECKEY_DecodeDERSubjectPublicKeyInfo(&spkiItem));
+  ASSERT_TRUE(certSpki);
+  ScopedSECKEYPublicKey pubKey(SECKEY_ExtractPublicKey(certSpki.get()));
+  ASSERT_TRUE(pubKey);
+
+  SECItem hash = {siBuffer, toUcharPtr(kHash), sizeof(kHash)};
+
+  // kSignature is a valid signature.
+  SECItem sigItem = {siBuffer, toUcharPtr(kSignature), sizeof(kSignature)};
+  SECStatus rv = VFY_VerifyDigestDirect(&hash, pubKey.get(), &sigItem,
+                                        SEC_OID_PKCS1_RSA_ENCRYPTION,
+                                        SEC_OID_SHA256, nullptr);
+  EXPECT_EQ(SECSuccess, rv);
+
+  // kSignatureInvalid is not.
+  sigItem = {siBuffer, toUcharPtr(kSignatureInvalid),
+             sizeof(kSignatureInvalid)};
+  rv = VFY_VerifyDigestDirect(&hash, pubKey.get(), &sigItem,
+                              SEC_OID_PKCS1_RSA_ENCRYPTION, SEC_OID_SHA256,
+                              nullptr);
+#ifdef NSS_PKCS1_AllowMissingParameters
+  EXPECT_EQ(SECSuccess, rv);
+#else
+  EXPECT_EQ(SECFailure, rv);
+#endif
+}
+
+}  // namespace nss_test
diff --git a/lib/cryptohi/secvfy.c b/lib/cryptohi/secvfy.c
index 83c9c579d..193738fed 100644
--- a/lib/cryptohi/secvfy.c
+++ b/lib/cryptohi/secvfy.c
@@ -161,7 +161,7 @@ verifyPKCS1DigestInfo(const VFYContext *cx, const SECItem *digest)
     pkcs1DigestInfo.len = cx->pkcs1RSADigestInfoLen;
     return _SGN_VerifyPKCS1DigestInfo(
         cx->hashAlg, digest, &pkcs1DigestInfo,
-        PR_TRUE /*XXX: unsafeAllowMissingParameters*/);
+        PR_FALSE /*XXX: unsafeAllowMissingParameters*/);
 }
 
 /*
diff --git a/lib/softoken/pkcs11c.c b/lib/softoken/pkcs11c.c
index 385d3c144..7eec3d7ee 100644
--- a/lib/softoken/pkcs11c.c
+++ b/lib/softoken/pkcs11c.c
@@ -3106,7 +3106,7 @@ RSA_HashCheckSign(SECOidTag digestOid, NSSLOWKEYPublicKey *key,
         digest.len = digestLen;
         rv = _SGN_VerifyPKCS1DigestInfo(
             digestOid, &digest, &pkcs1DigestInfo,
-            PR_TRUE /*XXX: unsafeAllowMissingParameters*/);
+            PR_FALSE /*XXX: unsafeAllowMissingParameters*/);
     }
 
     PORT_Free(pkcs1DigestInfoData);
diff --git a/lib/util/pkcs1sig.c b/lib/util/pkcs1sig.c
index 502119aa5..68588c7f8 100644
--- a/lib/util/pkcs1sig.c
+++ b/lib/util/pkcs1sig.c
@@ -15,13 +15,6 @@ struct pkcs1PrefixStr {
     PRUint8 *data;
 };
 
-typedef struct pkcs1PrefixesStr pkcs1Prefixes;
-struct pkcs1PrefixesStr {
-    unsigned int digestLen;
-    pkcs1Prefix prefixWithParams;
-    pkcs1Prefix prefixWithoutParams;
-};
-
 /* The value for SGN_PKCS1_DIGESTINFO_MAX_PREFIX_LEN_EXCLUDING_OID is based on
  * the possible prefix encodings as explained below.
  */
@@ -101,9 +94,8 @@ _SGN_VerifyPKCS1DigestInfo(SECOidTag digestAlg,
                            PRBool unsafeAllowMissingParameters)
 {
     SECOidData *hashOid;
-    pkcs1Prefixes pp;
-    const pkcs1Prefix *expectedPrefix;
-    SECStatus rv, rv2, rv3;
+    pkcs1Prefix prefix;
+    SECStatus rv;
 
     if (!digest || !digest->data ||
         !dataRecoveredFromSignature || !dataRecoveredFromSignature->data) {
@@ -117,17 +109,9 @@ _SGN_VerifyPKCS1DigestInfo(SECOidTag digestAlg,
         return SECFailure;
     }
 
-    pp.digestLen = digest->len;
-    pp.prefixWithParams.data = NULL;
-    pp.prefixWithoutParams.data = NULL;
+    prefix.data = NULL;
 
-    rv2 = encodePrefix(hashOid, pp.digestLen, &pp.prefixWithParams, PR_TRUE);
-    rv3 = encodePrefix(hashOid, pp.digestLen, &pp.prefixWithoutParams, PR_FALSE);
-
-    rv = SECSuccess;
-    if (rv2 != SECSuccess || rv3 != SECSuccess) {
-        rv = SECFailure;
-    }
+    rv = encodePrefix(hashOid, digest->len, &prefix, PR_TRUE);
 
     if (rv == SECSuccess) {
         /* We don't attempt to avoid timing attacks on these comparisons because
@@ -135,34 +119,39 @@ _SGN_VerifyPKCS1DigestInfo(SECOidTag digestAlg,
          * operation.
          */
 
-        if (dataRecoveredFromSignature->len ==
-            pp.prefixWithParams.len + pp.digestLen) {
-            expectedPrefix = &pp.prefixWithParams;
-        } else if (unsafeAllowMissingParameters &&
-                   dataRecoveredFromSignature->len ==
-                       pp.prefixWithoutParams.len + pp.digestLen) {
-            expectedPrefix = &pp.prefixWithoutParams;
-        } else {
-            PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
-            rv = SECFailure;
+        if (dataRecoveredFromSignature->len != prefix.len + digest->len) {
+            PRBool lengthMismatch = PR_TRUE;
+#ifdef NSS_PKCS1_AllowMissingParameters
+            if (unsafeAllowMissingParameters) {
+                if (prefix.data) {
+                    PORT_Free(prefix.data);
+                    prefix.data = NULL;
+                }
+                rv = encodePrefix(hashOid, digest->len, &prefix, PR_FALSE);
+                if (rv != SECSuccess ||
+                    dataRecoveredFromSignature->len == prefix.len + digest->len) {
+                    lengthMismatch = PR_FALSE;
+                }
+            }
+#endif
+            if (lengthMismatch) {
+                PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
+                rv = SECFailure;
+            }
         }
     }
 
     if (rv == SECSuccess) {
-        if (memcmp(dataRecoveredFromSignature->data, expectedPrefix->data,
-                   expectedPrefix->len) ||
-            memcmp(dataRecoveredFromSignature->data + expectedPrefix->len,
-                   digest->data, digest->len)) {
+        if (memcmp(dataRecoveredFromSignature->data, prefix.data, prefix.len) ||
+            memcmp(dataRecoveredFromSignature->data + prefix.len, digest->data,
+                   digest->len)) {
             PORT_SetError(SEC_ERROR_BAD_SIGNATURE);
             rv = SECFailure;
         }
     }
 
-    if (pp.prefixWithParams.data) {
-        PORT_Free(pp.prefixWithParams.data);
-    }
-    if (pp.prefixWithoutParams.data) {
-        PORT_Free(pp.prefixWithoutParams.data);
+    if (prefix.data) {
+        PORT_Free(prefix.data);
     }
 
     return rv;