Bug 360126 - Stop accepting certs that use RSA1023 or weaker; Original patch by Richard van den Berg. r=briansmith
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 15 Jul 2014 19:49:00 -0400
changeset 208179 83b81059b2a2c5af28632891978c3ee589958f0f
parent 208178 c311ca13e83038db0448c39c11e0f6e1c3686870
child 208180 7c53c5e9bd3a2e7b3e4253047825ac9d257d2db1
push idunknown
push userunknown
push dateunknown
reviewersbriansmith
bugs360126
milestone33.0a1
Bug 360126 - Stop accepting certs that use RSA1023 or weaker; Original patch by Richard van den Berg. r=briansmith
security/apps/AppTrustDomain.cpp
security/apps/AppTrustDomain.h
security/build/nss.def
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/pkix/include/pkix/pkix.h
security/pkix/include/pkix/pkixtypes.h
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixkey.cpp
security/pkix/test/gtest/pkixbuild_tests.cpp
security/pkix/test/gtest/pkixcert_extension_tests.cpp
security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
--- a/security/apps/AppTrustDomain.cpp
+++ b/security/apps/AppTrustDomain.cpp
@@ -214,9 +214,15 @@ AppTrustDomain::CheckRevocation(EndEntit
 }
 
 SECStatus
 AppTrustDomain::IsChainValid(const DERArray& certChain)
 {
   return ConstructCERTCertListFromReversedDERArray(certChain, mCertChain);
 }
 
+SECStatus
+AppTrustDomain::CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+{
+  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+}
+
 } } // namespace mozilla::psm
--- a/security/apps/AppTrustDomain.h
+++ b/security/apps/AppTrustDomain.h
@@ -33,16 +33,17 @@ public:
                             /*optional*/ const SECItem* aiaExtension);
   SECStatus IsChainValid(const mozilla::pkix::DERArray& certChain);
 
   SECStatus VerifySignedData(
               const mozilla::pkix::SignedDataWithSignature& signedData,
               const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
   SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t* digestBuf,
                       size_t digestBufLen) MOZ_OVERRIDE;
+  SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
 
 private:
   /*out*/ ScopedCERTCertList& mCertChain;
   void* mPinArg; // non-owning!
   ScopedCERTCertificate mTrustedRoot;
 };
 
 } } // namespace mozilla::psm
--- a/security/build/nss.def
+++ b/security/build/nss.def
@@ -505,16 +505,17 @@ SECKEY_DestroyPrivateKey
 SECKEY_DestroyPrivateKeyList
 SECKEY_DestroyPublicKey
 SECKEY_DestroySubjectPublicKeyInfo
 SECKEY_ECParamsToBasePointOrderLen
 SECKEY_ECParamsToKeySize
 SECKEY_EncodeDERSubjectPublicKeyInfo
 SECKEY_ExtractPublicKey
 SECKEY_PublicKeyStrength
+SECKEY_PublicKeyStrengthInBits
 SECKEY_RSAPSSParamsTemplate DATA
 SECMIME_DecryptionAllowed
 SECMOD_AddNewModule
 SECMOD_AddNewModuleEx
 SECMOD_CancelWait
 SECMOD_CanDeleteInternalModule
 SECMOD_CloseUserDB
 SECMOD_CreateModule
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -662,16 +662,22 @@ NSSCertDBTrustDomain::IsChainValid(const
 
   if (mBuiltChain) {
     *mBuiltChain = certList.forget();
   }
 
   return SECSuccess;
 }
 
+SECStatus
+NSSCertDBTrustDomain::CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+{
+  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+}
+
 namespace {
 
 static char*
 nss_addEscape(const char* string, char quote)
 {
   char* newString = 0;
   size_t escapes = 0, size = 0;
   const char* src;
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -72,16 +72,18 @@ public:
   virtual SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                     const mozilla::pkix::CertID& certID,
                                     PRTime time,
                        /*optional*/ const SECItem* stapledOCSPResponse,
                        /*optional*/ const SECItem* aiaExtension);
 
   virtual SECStatus IsChainValid(const mozilla::pkix::DERArray& certChain);
 
+  virtual SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo);
+
 private:
   enum EncodedResponseSource {
     ResponseIsFromNetwork = 1,
     ResponseWasStapled = 2
   };
   static const PRTime ServerFailureDelay = 5 * 60 * PR_USEC_PER_SEC;
   SECStatus VerifyAndMaybeCacheEncodedOCSPResponse(
     const mozilla::pkix::CertID& certID, PRTime time,
--- a/security/pkix/include/pkix/pkix.h
+++ b/security/pkix/include/pkix/pkix.h
@@ -134,11 +134,14 @@ SECStatus VerifyEncodedOCSPResponse(Trus
 //
 // TODO(bug 966856): Add SHA-2 support
 // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
 // other, extensive, memory safety efforts in mozilla::pkix, and we should find
 // a way to provide a more-obviously-safe interface.
 SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t* digestBuf,
                     size_t digestBufLen);
 
+// Checks, for RSA keys and DSA keys, that the modulus is at least 1024 bits.
+SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo);
+
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkix_h
--- a/security/pkix/include/pkix/pkixtypes.h
+++ b/security/pkix/include/pkix/pkixtypes.h
@@ -297,16 +297,19 @@ public:
                                     const CertID& certID, PRTime time,
                        /*optional*/ const SECItem* stapledOCSPresponse,
                        /*optional*/ const SECItem* aiaExtension) = 0;
 
   // Verify the given signature using the given public key.
   //
   // Most implementations of this function should probably forward the call
   // directly to mozilla::pkix::VerifySignedData.
+  //
+  // In any case, the implementation must perform checks on the public key
+  // similar to how mozilla::pkix::CheckPublicKey() does.
   virtual SECStatus VerifySignedData(const SignedDataWithSignature& signedData,
                                      const SECItem& subjectPublicKeyInfo) = 0;
 
   // Compute the SHA-1 hash of the data in the current item.
   //
   // item contains the data to hash.
   // digestBuf must point to a buffer to where the SHA-1 hash will be written.
   // digestBufLen must be DIGEST_LENGTH (20, the length of a SHA-1 hash).
@@ -314,16 +317,24 @@ public:
   // TODO(bug 966856): Add SHA-2 support
   // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
   // other, extensive, memory safety efforts in mozilla::pkix, and we should
   // find a way to provide a more-obviously-safe interface.
   static const size_t DIGEST_LENGTH = 20; // length of SHA-1 digest
   virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t* digestBuf,
                               size_t digestBufLen) = 0;
 
+  // Check that the key size, algorithm, and parameters of the given public key
+  // are acceptable.
+  //
+  // VerifySignedData() should do the same checks that this function does, but
+  // mainly for efficiency, some keys are not passed to VerifySignedData().
+  // This function is called instead for those keys.
+  virtual SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo) = 0;
+
 protected:
   TrustDomain() { }
 
 private:
   TrustDomain(const TrustDomain&) /* = delete */;
   void operator=(const TrustDomain&) /* = delete */;
 };
 
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -314,16 +314,23 @@ BuildCertChain(TrustDomain& trustDomain,
   // XXX: Support the legacy use of the subject CN field for indicating the
   // domain name the certificate is valid for.
   BackCert cert(certDER, endEntityOrCA, nullptr);
   Result rv = cert.Init();
   if (rv != Success) {
     return SECFailure;
   }
 
+  // See documentation for CheckPublicKey() in pkixtypes.h for why the public
+  // key also needs to be checked here when trustDomain.VerifySignedData()
+  // should already be doing it.
+  if (trustDomain.CheckPublicKey(cert.GetSubjectPublicKeyInfo()) != SECSuccess) {
+    return SECFailure;
+  }
+
   rv = BuildForward(trustDomain, cert, time, requiredKeyUsageIfPresent,
                     requiredEKUIfPresent, requiredPolicy, stapledOCSPResponse,
                     0/*subCACount*/);
   if (rv != Success) {
     return SECFailure;
   }
 
   return SECSuccess;
--- a/security/pkix/lib/pkixkey.cpp
+++ b/security/pkix/lib/pkixkey.cpp
@@ -31,16 +31,68 @@
 #include "pk11pub.h"
 #include "pkix/pkix.h"
 #include "pkix/ScopedPtr.h"
 #include "prerror.h"
 #include "secerr.h"
 
 namespace mozilla { namespace pkix {
 
+typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> ScopedSECKeyPublicKey;
+
+SECStatus
+CheckPublicKeySize(const SECItem& subjectPublicKeyInfo,
+                   /*out*/ ScopedSECKeyPublicKey& publicKey)
+{
+  ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
+    spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfo));
+  if (!spki) {
+    return SECFailure;
+  }
+  publicKey = SECKEY_ExtractPublicKey(spki.get());
+  if (!publicKey) {
+    return SECFailure;
+  }
+
+  static const unsigned int MINIMUM_NON_ECC_BITS = 1024;
+
+  switch (publicKey.get()->keyType) {
+    case ecKey:
+      // TODO(bug 622859): We should check which curve.
+      return SECSuccess;
+    case dsaKey: // fall through
+    case rsaKey:
+      // TODO(bug 622859): Enforce a minimum of 2048 bits for EV certs.
+      if (SECKEY_PublicKeyStrengthInBits(publicKey.get()) < MINIMUM_NON_ECC_BITS) {
+        // TODO(bug 1031946): Create a new error code.
+        PR_SetError(SEC_ERROR_INVALID_KEY, 0);
+        return SECFailure;
+      }
+      break;
+    case nullKey:
+    case fortezzaKey:
+    case dhKey:
+    case keaKey:
+    case rsaPssKey:
+    case rsaOaepKey:
+    default:
+      PR_SetError(SEC_ERROR_UNSUPPORTED_KEYALG, 0);
+      return SECFailure;
+  }
+
+  return SECSuccess;
+}
+
+SECStatus
+CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+{
+  ScopedSECKeyPublicKey unused;
+  return CheckPublicKeySize(subjectPublicKeyInfo, unused);
+}
+
 SECStatus
 VerifySignedData(const SignedDataWithSignature& sd,
                  const SECItem& subjectPublicKeyInfo, void* pkcs11PinArg)
 {
   if (!sd.data.data || !sd.signature.data) {
     PR_NOT_REACHED("invalid args to VerifySignedData");
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
@@ -96,24 +148,18 @@ VerifySignedData(const SignedDataWithSig
       digestAlg = SEC_OID_SHA1;
       break;
     default:
       PR_NOT_REACHED("unknown signature algorithm");
       PR_SetError(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED, 0);
       return SECFailure;
   }
 
-  ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
-    spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfo));
-  if (!spki) {
-    return SECFailure;
-  }
-  ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
-    pubKey(SECKEY_ExtractPublicKey(spki.get()));
-  if (!pubKey) {
+  ScopedSECKeyPublicKey pubKey;
+  if (CheckPublicKeySize(subjectPublicKeyInfo, pubKey) != SECSuccess) {
     return SECFailure;
   }
 
   // The static_cast is safe according to the check above that references
   // bug 921585.
   return VFY_VerifyDataDirect(sd.data.data, static_cast<int>(sd.data.len),
                               pubKey.get(), &sd.signature, pubKeyAlg,
                               digestAlg, nullptr, pkcs11PinArg);
--- a/security/pkix/test/gtest/pkixbuild_tests.cpp
+++ b/security/pkix/test/gtest/pkixbuild_tests.cpp
@@ -172,16 +172,21 @@ private:
   virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,
                               size_t digestBufLen)
   {
     ADD_FAILURE();
     PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
     return SECFailure;
   }
 
+  SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+  {
+    return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  }
+
   // We hold references to CERTCertificates in the cert chain tail so that we
   // CERT_CreateSubjectCertList can find them.
   ScopedCERTCertificate certChainTail[7];
 
 public:
   ScopedSECKEYPrivateKey leafCAKey;
   CERTCertificate* GetLeafCACert() const
   {
--- a/security/pkix/test/gtest/pkixcert_extension_tests.cpp
+++ b/security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ -110,16 +110,21 @@ private:
   }
 
   SECStatus DigestBuf(const SECItem&, /*out*/ uint8_t *, size_t)
   {
     ADD_FAILURE();
     PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
     return SECFailure;
   }
+
+  SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+  {
+    return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  }
 };
 
 class pkixcert_extension: public NSSTest
 {
 public:
   static void SetUpTestCase()
   {
     NSSTest::SetUpTestCase();
--- a/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
+++ b/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp
@@ -72,16 +72,21 @@ private:
     return SECFailure;
   }
 
   virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,
                               size_t digestBufLen)
   {
     return ::mozilla::pkix::DigestBuf(item, digestBuf, digestBufLen);
   }
+
+  virtual SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+  {
+    return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  }
 };
 
 class pkixocsp_CreateEncodedOCSPRequest : public NSSTest
 {
 protected:
   // These SECItems are allocated in arena, and so will be auto-cleaned.
   SECItem* unsupportedLongSerialNumber;
   SECItem* longestRequiredSerialNumber;
--- a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
+++ b/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ -87,16 +87,21 @@ public:
   }
 
   virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t* digestBuf,
                               size_t digestBufLen)
   {
     return ::mozilla::pkix::DigestBuf(item, digestBuf, digestBufLen);
   }
 
+  virtual SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo)
+  {
+    return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  }
+
 private:
   OCSPTestTrustDomain(const OCSPTestTrustDomain&) /*delete*/;
   void operator=(const OCSPTestTrustDomain&) /*delete*/;
 };
 
 namespace {
 char const* const rootName = "CN=Test CA 1";
 void deleteCertID(CertID* certID) { delete certID; }