Bug 622859 - Reject EV certificates with key sizes below RSA 2048. r=briansmith

security/apps/AppTrustDomain.cpp

@@ -30,6 +30,8 @@ using namespace mozilla::pkix;
 extern PRLogModuleInfo* gPIPNSSLog;
 #endif
 
+static const unsigned int MINIMUM_NON_ECC_BITS = 2048;
+
 namespace mozilla { namespace psm {
 
 AppTrustDomain::AppTrustDomain(ScopedCERTCertList& certChain, void* pinArg)
@@ -213,7 +215,7 @@ AppTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData,
                                  Input subjectPublicKeyInfo)
 {
   return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
-                                           mPinArg);
+                                           MINIMUM_NON_ECC_BITS, mPinArg);
 }
 
 Result
@@ -247,7 +249,8 @@ AppTrustDomain::IsChainValid(const DERArray& certChain, Time time)
 Result
 AppTrustDomain::CheckPublicKey(Input subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo,
+                                         MINIMUM_NON_ECC_BITS);
 }
 
 } } // namespace mozilla::psm

security/certverifier/CertVerifier.cpp

@@ -211,7 +211,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
       // just use trustEmail as it is the closest alternative.
       NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache,
                                        pinArg, ocspGETConfig, pinningDisabled,
-                                       nullptr, builtChain);
+                                       false, nullptr, builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
@@ -236,7 +236,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
                       ocspFetching == NSSCertDBTrustDomain::NeverFetchOCSP
                         ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV
                         : NSSCertDBTrustDomain::FetchOCSPForEV,
-                      mOCSPCache, pinArg, ocspGETConfig, mPinningMode,
+                      mOCSPCache, pinArg, ocspGETConfig, mPinningMode, true,
                       hostname, builtChain);
         rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time,
                                           KeyUsage::digitalSignature,// (EC)DHE
@@ -261,7 +261,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
       // Now try non-EV.
       NSSCertDBTrustDomain trustDomain(trustSSL, ocspFetching, mOCSPCache,
                                        pinArg, ocspGETConfig, mPinningMode,
-                                       hostname, builtChain);
+                                       false, hostname, builtChain);
       rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time,
                                         KeyUsage::digitalSignature, // (EC)DHE
                                         KeyUsage::keyEncipherment, // RSA
@@ -275,7 +275,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
     case certificateUsageSSLCA: {
       NSSCertDBTrustDomain trustDomain(trustSSL, ocspFetching, mOCSPCache,
                                        pinArg, ocspGETConfig, pinningDisabled,
-                                       nullptr, builtChain);
+                                       false, nullptr, builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeCA, KeyUsage::keyCertSign,
                           KeyPurposeId::id_kp_serverAuth,
@@ -286,7 +286,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
     case certificateUsageEmailSigner: {
       NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache,
                                        pinArg, ocspGETConfig, pinningDisabled,
-                                       nullptr, builtChain);
+                                       false, nullptr, builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
@@ -301,7 +301,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
       // based on the result of the verification(s).
       NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache,
                                        pinArg, ocspGETConfig, pinningDisabled,
-                                       nullptr, builtChain);
+                                       false, nullptr, builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::keyEncipherment, // RSA
@@ -320,7 +320,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
     case certificateUsageObjectSigner: {
       NSSCertDBTrustDomain trustDomain(trustObjectSigning, ocspFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
-                                       pinningDisabled, nullptr, builtChain);
+                                       pinningDisabled, false, nullptr,
+                                       builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
@@ -348,15 +349,15 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
       }
 
       NSSCertDBTrustDomain sslTrust(trustSSL, ocspFetching, mOCSPCache, pinArg,
-                                    ocspGETConfig, pinningDisabled, nullptr,
+                                    ocspGETConfig, pinningDisabled, false,
-                                    builtChain);
+                                    nullptr, builtChain);
       rv = BuildCertChain(sslTrust, certDER, time, endEntityOrCA,
                           keyUsage, eku, CertPolicyId::anyPolicy,
                           stapledOCSPResponse);
       if (rv == Result::ERROR_UNKNOWN_ISSUER) {
         NSSCertDBTrustDomain emailTrust(trustEmail, ocspFetching, mOCSPCache,
                                         pinArg, ocspGETConfig, pinningDisabled,
-                                        nullptr, builtChain);
+                                        false, nullptr, builtChain);
         rv = BuildCertChain(emailTrust, certDER, time, endEntityOrCA,
                             keyUsage, eku, CertPolicyId::anyPolicy,
                             stapledOCSPResponse);
@@ -364,8 +365,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
           NSSCertDBTrustDomain objectSigningTrust(trustObjectSigning,
                                                   ocspFetching, mOCSPCache,
                                                   pinArg, ocspGETConfig,
-                                                  pinningDisabled, nullptr,
+                                                  pinningDisabled, false,
-                                                  builtChain);
+                                                  nullptr, builtChain);
           rv = BuildCertChain(objectSigningTrust, certDER, time,
                               endEntityOrCA, keyUsage, eku,
                               CertPolicyId::anyPolicy, stapledOCSPResponse);

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -35,6 +35,9 @@ extern PRLogModuleInfo* gCertVerifierLog;
 
 static const uint64_t ServerFailureDelaySeconds = 5 * 60;
 
+static const unsigned int MINIMUM_NON_ECC_BITS_DV = 1024;
+static const unsigned int MINIMUM_NON_ECC_BITS_EV = 2048;
+
 namespace mozilla { namespace psm {
 
 const char BUILTIN_ROOTS_MODULE_DEFAULT_NAME[] = "Builtin Roots Module";
@@ -53,6 +56,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(SECTrustType certDBTrustType,
              /*optional but shouldn't be*/ void* pinArg,
                                            CertVerifier::ocsp_get_config ocspGETConfig,
                                            CertVerifier::PinningMode pinningMode,
+                                           bool forEV,
                               /*optional*/ const char* hostname,
                               /*optional*/ ScopedCERTCertList* builtChain)
   : mCertDBTrustType(certDBTrustType)
@@ -61,6 +65,7 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(SECTrustType certDBTrustType,
   , mPinArg(pinArg)
   , mOCSPGetConfig(ocspGETConfig)
   , mPinningMode(pinningMode)
+  , mMinimumNonECCBits(forEV ? MINIMUM_NON_ECC_BITS_EV : MINIMUM_NON_ECC_BITS_DV)
   , mHostname(hostname)
   , mBuiltChain(builtChain)
 {
@@ -226,7 +231,7 @@ NSSCertDBTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData
                                        Input subjectPublicKeyInfo)
 {
   return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
-                                           mPinArg);
+                                           mMinimumNonECCBits, mPinArg);
 }
 
 Result
@@ -663,7 +668,8 @@ NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time)
 Result
 NSSCertDBTrustDomain::CheckPublicKey(Input subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo);
+  return ::mozilla::pkix::CheckPublicKey(subjectPublicKeyInfo,
+                                         mMinimumNonECCBits);
 }
 
 namespace {

security/certverifier/NSSCertDBTrustDomain.h

@@ -54,6 +54,7 @@ public:
                        OCSPCache& ocspCache, void* pinArg,
                        CertVerifier::ocsp_get_config ocspGETConfig,
                        CertVerifier::PinningMode pinningMode,
+                       bool forEV,
           /*optional*/ const char* hostname = nullptr,
       /*optional out*/ ScopedCERTCertList* builtChain = nullptr);
 
@@ -106,6 +107,7 @@ private:
   void* mPinArg; // non-owning!
   const CertVerifier::ocsp_get_config mOCSPGetConfig;
   CertVerifier::PinningMode mPinningMode;
+  const unsigned int mMinimumNonECCBits;
   const char* mHostname; // non-owning - only used for pinning checks
   ScopedCERTCertList* mBuiltChain; // non-owning
 };

security/pkix/include/pkix/pkixnss.h

@@ -34,6 +34,7 @@ namespace mozilla { namespace pkix {
 // Verify the given signed data using the given public key.
 Result VerifySignedData(const SignedDataWithSignature& sd,
                         Input subjectPublicKeyInfo,
+                        unsigned int minimumNonECCBits,
                         void* pkcs11PinArg);
 
 // Computes the SHA-1 hash of the data in the current item.
@@ -50,8 +51,10 @@ Result VerifySignedData(const SignedDataWithSignature& sd,
 Result DigestBuf(Input item, /*out*/ uint8_t* digestBuf,
                  size_t digestBufLen);
 
-// Checks, for RSA keys and DSA keys, that the modulus is at least 1024 bits.
+// Checks, for RSA keys and DSA keys, that the modulus is at least the given
-Result CheckPublicKey(Input subjectPublicKeyInfo);
+// number of bits.
+Result CheckPublicKey(Input subjectPublicKeyInfo,
+                      unsigned int minimumNonECCBits);
 
 Result MapPRErrorCodeToResult(PRErrorCode errorCode);
 PRErrorCode MapResultToPRErrorCode(Result result);

security/pkix/lib/pkixnss.cpp

@@ -39,7 +39,7 @@ namespace mozilla { namespace pkix {
 typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> ScopedSECKeyPublicKey;
 
 Result
-CheckPublicKeySize(Input subjectPublicKeyInfo,
+CheckPublicKeySize(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits,
                    /*out*/ ScopedSECKeyPublicKey& publicKey)
 {
   SECItem subjectPublicKeyInfoSECItem =
@@ -54,16 +54,13 @@ CheckPublicKeySize(Input subjectPublicKeyInfo,
     return MapPRErrorCodeToResult(PR_GetError());
   }
 
-  static const unsigned int MINIMUM_NON_ECC_BITS = 1024;
-
   switch (publicKey.get()->keyType) {
     case ecKey:
-      // TODO(bug 622859): We should check which curve.
+      // TODO(bug 1077790): We should check which curve.
       return Success;
     case dsaKey: // fall through
     case rsaKey:
-      // TODO(bug 622859): Enforce a minimum of 2048 bits for EV certs.
+      if (SECKEY_PublicKeyStrengthInBits(publicKey.get()) < minimumNonECCBits) {
-      if (SECKEY_PublicKeyStrengthInBits(publicKey.get()) < MINIMUM_NON_ECC_BITS) {
         return Result::ERROR_INADEQUATE_KEY_SIZE;
       }
       break;
@@ -81,15 +78,16 @@ CheckPublicKeySize(Input subjectPublicKeyInfo,
 }
 
 Result
-CheckPublicKey(Input subjectPublicKeyInfo)
+CheckPublicKey(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits)
 {
   ScopedSECKeyPublicKey unused;
-  return CheckPublicKeySize(subjectPublicKeyInfo, unused);
+  return CheckPublicKeySize(subjectPublicKeyInfo, minimumNonECCBits, unused);
 }
 
 Result
 VerifySignedData(const SignedDataWithSignature& sd,
-                 Input subjectPublicKeyInfo, void* pkcs11PinArg)
+                 Input subjectPublicKeyInfo, unsigned int minimumNonECCBits,
+                 void* pkcs11PinArg)
 {
   SECOidTag pubKeyAlg;
   SECOidTag digestAlg;
@@ -142,7 +140,7 @@ VerifySignedData(const SignedDataWithSignature& sd,
 
   Result rv;
   ScopedSECKeyPublicKey pubKey;
-  rv = CheckPublicKeySize(subjectPublicKeyInfo, pubKey);
+  rv = CheckPublicKeySize(subjectPublicKeyInfo, minimumNonECCBits, pubKey);
   if (rv != Success) {
     return rv;
   }

security/pkix/test/gtest/pkixbuild_tests.cpp

@@ -169,7 +169,7 @@ private:
                                   Input subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
-                                             nullptr);
+                                             MINIMUM_TEST_KEY_BITS, nullptr);
   }
 
   virtual Result DigestBuf(Input item, /*out*/ uint8_t *digestBuf,
@@ -352,7 +352,7 @@ public:
                                   Input subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
-                                             nullptr);
+                                             MINIMUM_TEST_KEY_BITS, nullptr);
   }
 
   virtual Result DigestBuf(Input, /*out*/uint8_t*, size_t)

security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp

@@ -111,7 +111,7 @@ private:
   {
     EXPECT_NE(SignatureAlgorithm::unsupported_algorithm, signedData.algorithm);
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
-                                             nullptr);
+                                             MINIMUM_TEST_KEY_BITS, nullptr);
   }
 
   virtual Result DigestBuf(Input, uint8_t*, size_t)

security/pkix/test/lib/pkixtestnss.cpp

@@ -259,7 +259,7 @@ Result
 TestCheckPublicKey(Input subjectPublicKeyInfo)
 {
   InitNSSIfNeeded();
-  return CheckPublicKey(subjectPublicKeyInfo);
+  return CheckPublicKey(subjectPublicKeyInfo, MINIMUM_TEST_KEY_BITS);
 }
 
 Result
@@ -267,7 +267,8 @@ TestVerifySignedData(const SignedDataWithSignature& signedData,
                      Input subjectPublicKeyInfo)
 {
   InitNSSIfNeeded();
-  return VerifySignedData(signedData, subjectPublicKeyInfo, nullptr);
+  return VerifySignedData(signedData, subjectPublicKeyInfo,
+                          MINIMUM_TEST_KEY_BITS, nullptr);
 }
 
 Result

security/pkix/test/lib/pkixtestutil.h

@@ -33,6 +33,8 @@
 #include "pkix/pkixtypes.h"
 #include "pkix/ScopedPtr.h"
 
+static const unsigned int MINIMUM_TEST_KEY_BITS = 1024;
+
 namespace mozilla { namespace pkix { namespace test {
 
 typedef std::basic_string<uint8_t> ByteString;