Bug 1131767: Prune away paths using unacceptable algorithms earlier, r=keeler

--HG--
extra : rebase_source : 79efad2c5f60120ff1022547ce7efa628a7acd0f

security/apps/AppTrustDomain.cpp

@@ -243,6 +243,13 @@ AppTrustDomain::IsChainValid(const DERArray& certChain, Time time)
   return Success;
 }
 
+Result
+AppTrustDomain::CheckSignatureDigestAlgorithm(DigestAlgorithm)
+{
+  // TODO: We should restrict signatures to SHA-256 or better.
+  return Success;
+}
+
 Result
 AppTrustDomain::CheckRSAPublicKeyModulusSizeInBits(
   EndEntityOrCA /*endEntityOrCA*/, unsigned int modulusSizeInBits)
@@ -257,6 +264,7 @@ Result
 AppTrustDomain::VerifyRSAPKCS1SignedDigest(const SignedDigest& signedDigest,
                                            Input subjectPublicKeyInfo)
 {
+  // TODO: We should restrict signatures to SHA-256 or better.
   return VerifyRSAPKCS1SignedDigestNSS(signedDigest, subjectPublicKeyInfo,
                                        mPinArg);
 }

security/apps/AppTrustDomain.h

@@ -38,6 +38,8 @@ public:
                     /*optional*/ const mozilla::pkix::Input* aiaExtension) MOZ_OVERRIDE;
   virtual Result IsChainValid(const mozilla::pkix::DERArray& certChain,
                               mozilla::pkix::Time time) MOZ_OVERRIDE;
+  virtual Result CheckSignatureDigestAlgorithm(
+                   mozilla::pkix::DigestAlgorithm digestAlg) MOZ_OVERRIDE;
   virtual Result CheckRSAPublicKeyModulusSizeInBits(
                    mozilla::pkix::EndEntityOrCA endEntityOrCA,
                    unsigned int modulusSizeInBits) MOZ_OVERRIDE;

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -712,6 +712,12 @@ NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time)
   return Success;
 }
 
+Result
+NSSCertDBTrustDomain::CheckSignatureDigestAlgorithm(DigestAlgorithm)
+{
+  return Success;
+}
+
 Result
 NSSCertDBTrustDomain::CheckRSAPublicKeyModulusSizeInBits(
   EndEntityOrCA /*endEntityOrCA*/, unsigned int modulusSizeInBits)

security/certverifier/NSSCertDBTrustDomain.h

@@ -70,6 +70,9 @@ public:
                               /*out*/ mozilla::pkix::TrustLevel& trustLevel)
                               MOZ_OVERRIDE;
 
+  virtual Result CheckSignatureDigestAlgorithm(
+                   mozilla::pkix::DigestAlgorithm digestAlg) MOZ_OVERRIDE;
+
   virtual Result CheckRSAPublicKeyModulusSizeInBits(
                    mozilla::pkix::EndEntityOrCA endEntityOrCA,
                    unsigned int modulusSizeInBits) MOZ_OVERRIDE;

security/pkix/include/pkix/pkixtypes.h

@@ -271,6 +271,13 @@ public:
                     /*optional*/ const Input* stapledOCSPresponse,
                     /*optional*/ const Input* aiaExtension) = 0;
 
+  // Check that the given digest algorithm is acceptable for use in signatures.
+  //
+  // Return Success if the algorithm is acceptable,
+  // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
+  // acceptable, or another error code if another error occurred.
+  virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg) = 0;
+
   // Check that the RSA public key size is acceptable.
   //
   // Return Success if the key size is acceptable,

security/pkix/lib/pkixcheck.cpp

@@ -33,12 +33,15 @@ namespace mozilla { namespace pkix {
 // 4.1.2.3 signature
 
 Result
-CheckSignatureAlgorithm(Input signatureAlgorithmValue, Input signatureValue)
+CheckSignatureAlgorithm(TrustDomain& trustDomain,
+                        EndEntityOrCA endEntityOrCA,
+                        const der::SignedDataWithSignature& signedData,
+                        Input signatureValue)
 {
   // 4.1.1.2. signatureAlgorithm
   der::PublicKeyAlgorithm publicKeyAlg;
   DigestAlgorithm digestAlg;
-  Reader signatureAlgorithmReader(signatureAlgorithmValue);
+  Reader signatureAlgorithmReader(signedData.algorithm);
   Result rv = der::SignatureAlgorithmIdentifierValue(signatureAlgorithmReader,
                                                      publicKeyAlg, digestAlg);
   if (rv != Success) {
@@ -78,6 +81,44 @@ CheckSignatureAlgorithm(Input signatureAlgorithmValue, Input signatureValue)
     return Result::ERROR_SIGNATURE_ALGORITHM_MISMATCH;
   }
 
+  // During the time of the deprecation of SHA-1 and the deprecation of RSA
+  // keys of less than 2048 bits, we will encounter many certs signed using
+  // SHA-1 and/or too-small RSA keys. With this in mind, we ask the trust
+  // domain early on if it knows it will reject the signature purely based on
+  // the digest algorithm and/or the RSA key size (if an RSA signature). This
+  // is a good optimization because it completely avoids calling
+  // trustDomain.FindIssuers (which may be slow) for such rejected certs, and
+  // more generally it short-circuits any path building with them (which, of
+  // course, is even slower).
+
+  rv = trustDomain.CheckSignatureDigestAlgorithm(digestAlg);
+  if (rv != Success) {
+    return rv;
+  }
+
+  switch (publicKeyAlg) {
+    case der::PublicKeyAlgorithm::RSA_PKCS1:
+    {
+      // The RSA computation may give a result that requires fewer bytes to
+      // encode than the public key (since it is modular arithmetic). However,
+      // the last step of generating a PKCS#1.5 signature is the I2OSP
+      // procedure, which pads any such shorter result with zeros so that it
+      // is exactly the same length as the public key.
+      unsigned int signatureSizeInBits = signedData.signature.GetLength() * 8u;
+      return trustDomain.CheckRSAPublicKeyModulusSizeInBits(
+               endEntityOrCA, signatureSizeInBits);
+    }
+
+    case der::PublicKeyAlgorithm::ECDSA:
+      // In theory, we could implement a similar early-pruning optimization for
+      // ECDSA curves. However, since there has been no similar deprecation for
+      // for any curve that we support, the chances of us encountering a curve
+      // during path building is too low to be worth bothering with.
+      break;
+
+    MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM
+  }
+
   return Success;
 }
 
@@ -808,8 +849,8 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
 
   switch (trustLevel) {
     case TrustLevel::InheritsTrust:
-      rv = CheckSignatureAlgorithm(cert.GetSignedData().algorithm,
-                                   cert.GetSignature());
+      rv = CheckSignatureAlgorithm(trustDomain, endEntityOrCA,
+                                   cert.GetSignedData(), cert.GetSignature());
       if (rv != Success) {
         return rv;
       }

security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp

@@ -30,8 +30,10 @@ using namespace mozilla::pkix::test;
 
 namespace mozilla { namespace pkix {
 
-extern Result CheckSignatureAlgorithm(Input signatureAlgorithmValue,
-                                      Input signatureValue);
+extern Result CheckSignatureAlgorithm(
+                TrustDomain& trustDomain, EndEntityOrCA endEntityOrCA,
+                const der::SignedDataWithSignature& signedData,
+                Input signatureValue);
 
 } } // namespace mozilla::pkix
 
@@ -39,6 +41,7 @@ struct CheckSignatureAlgorithmTestParams
 {
   ByteString signatureAlgorithmValue;
   ByteString signatureValue;
+  unsigned int signatureLengthInBytes;
   Result expectedResult;
 };
 
@@ -76,86 +79,110 @@ static const CheckSignatureAlgorithmTestParams
   { // Both algorithm IDs are empty
     ByteString(),
     ByteString(),
+    2048 / 8,
     Result::ERROR_BAD_DER,
   },
   { // signatureAlgorithm is empty, signature is supported.
     ByteString(),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_BAD_DER,
   },
   { // signatureAlgorithm is supported, signature is empty.
     BS(tlv_sha256WithRSAEncryption),
     ByteString(),
+    2048 / 8,
     Result::ERROR_BAD_DER,
   },
   { // Algorithms match, both are supported.
     BS(tlv_sha256WithRSAEncryption),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Success
   },
   { // Algorithms do not match because signatureAlgorithm is truncated.
     BS(tlv_sha256WithRSAEncryption_truncated),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
   },
   { // Algorithms do not match because signature is truncated.
     BS(tlv_sha256WithRSAEncryption),
     BS(tlv_sha256WithRSAEncryption_truncated),
+    2048 / 8,
     Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
   },
   { // Algorithms do not match, both are supported.
     BS(tlv_sha_1WithRSAEncryption),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_SIGNATURE_ALGORITHM_MISMATCH,
   },
   { // Algorithms do not match, both are supported.
     BS(tlv_sha256WithRSAEncryption),
     BS(tlv_sha_1WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_SIGNATURE_ALGORITHM_MISMATCH,
   },
   { // Algorithms match, both are unsupported.
     BS(tlv_md5WithRSAEncryption),
     BS(tlv_md5WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
   },
   { // signatureAlgorithm is unsupported, signature is supported.
     BS(tlv_md5WithRSAEncryption),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
   },
   { // signatureAlgorithm is supported, signature is unsupported.
     BS(tlv_sha256WithRSAEncryption),
     BS(tlv_md5WithRSAEncryption),
+    2048 / 8,
     Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
   },
   { // Both have the optional NULL parameter.
     BS(tlv_sha256WithRSAEncryption) + TLV(der::NULLTag, ByteString()),
     BS(tlv_sha256WithRSAEncryption) + TLV(der::NULLTag, ByteString()),
+    2048 / 8,
     Success
   },
   { // signatureAlgorithm has the optional NULL parameter, signature doesn't.
     BS(tlv_sha256WithRSAEncryption) + TLV(der::NULLTag, ByteString()),
     BS(tlv_sha256WithRSAEncryption),
+    2048 / 8,
     Success
   },
   { // signatureAlgorithm does not have the optional NULL parameter, signature
     // does.
     BS(tlv_sha256WithRSAEncryption),
     BS(tlv_sha256WithRSAEncryption) + TLV(der::NULLTag, ByteString()),
+    2048 / 8,
     Success
   },
   { // The different OIDs for RSA-with-SHA1 we support are semantically
     // equivalent.
     BS(tlv_sha1WithRSASignature),
     BS(tlv_sha_1WithRSAEncryption),
+    2048 / 8,
     Success,
   },
   { // The different OIDs for RSA-with-SHA1 we support are semantically
     // equivalent (opposite order).
     BS(tlv_sha_1WithRSAEncryption),
     BS(tlv_sha1WithRSASignature),
+    2048 / 8,
     Success,
   },
+  { // Algorithms match, both are supported, key size is not a multile of 128
+    // bits. This test verifies that we're not wrongly rounding up the
+    // signature size like we did in the original patch for bug 1131767.
+    BS(tlv_sha256WithRSAEncryption),
+    BS(tlv_sha256WithRSAEncryption),
+    (2048 / 8) - 1,
+    Success
+  },
 };
 
 class pkixcheck_CheckSignatureAlgorithm
@@ -164,6 +191,39 @@ class pkixcheck_CheckSignatureAlgorithm
 {
 };
 
+class pkixcheck_CheckSignatureAlgorithm_TrustDomain final
+  : public EverythingFailsByDefaultTrustDomain
+{
+public:
+  explicit pkixcheck_CheckSignatureAlgorithm_TrustDomain(
+             unsigned int publicKeySizeInBits)
+    : publicKeySizeInBits(publicKeySizeInBits)
+    , checkedDigestAlgorithm(false)
+    , checkedModulusSizeInBits(false)
+  {
+  }
+
+  Result CheckSignatureDigestAlgorithm(DigestAlgorithm) override
+  {
+    checkedDigestAlgorithm = true;
+    return Success;
+  }
+
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA endEntityOrCA,
+                                            unsigned int modulusSizeInBits)
+    override
+  {
+    EXPECT_EQ(EndEntityOrCA::MustBeEndEntity, endEntityOrCA);
+    EXPECT_EQ(publicKeySizeInBits, modulusSizeInBits);
+    checkedModulusSizeInBits = true;
+    return Success;
+  }
+
+  const unsigned int publicKeySizeInBits;
+  bool checkedDigestAlgorithm;
+  bool checkedModulusSizeInBits;
+};
+
 TEST_P(pkixcheck_CheckSignatureAlgorithm, CheckSignatureAlgorithm)
 {
   const CheckSignatureAlgorithmTestParams& params(GetParam());
@@ -173,26 +233,37 @@ TEST_P(pkixcheck_CheckSignatureAlgorithm, CheckSignatureAlgorithm)
             signatureValueInput.Init(params.signatureValue.data(),
                                      params.signatureValue.length()));
 
-  Input signatureAlgorithmValueInput;
+  pkixcheck_CheckSignatureAlgorithm_TrustDomain
+    trustDomain(params.signatureLengthInBytes * 8);
+
+  der::SignedDataWithSignature signedData;
   ASSERT_EQ(Success,
-            signatureAlgorithmValueInput.Init(
-              params.signatureAlgorithmValue.data(),
-              params.signatureAlgorithmValue.length()));
+            signedData.algorithm.Init(params.signatureAlgorithmValue.data(),
+                                      params.signatureAlgorithmValue.length()));
+
+  ByteString dummySignature(params.signatureLengthInBytes, 0xDE);
+  ASSERT_EQ(Success,
+            signedData.signature.Init(dummySignature.data(),
+                                      dummySignature.length()));
 
   ASSERT_EQ(params.expectedResult,
-            CheckSignatureAlgorithm(signatureAlgorithmValueInput,
-                                    signatureValueInput));
+            CheckSignatureAlgorithm(trustDomain, EndEntityOrCA::MustBeEndEntity,
+                                    signedData, signatureValueInput));
+  ASSERT_EQ(params.expectedResult == Success,
+            trustDomain.checkedDigestAlgorithm);
+  ASSERT_EQ(params.expectedResult == Success,
+            trustDomain.checkedModulusSizeInBits);
 }
 
 INSTANTIATE_TEST_CASE_P(
   pkixcheck_CheckSignatureAlgorithm, pkixcheck_CheckSignatureAlgorithm,
   testing::ValuesIn(CHECKSIGNATUREALGORITHM_TEST_PARAMS));
 
-class pkixcheck_CheckSignatureAlgorithmTrustDomain
+class pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain
   : public DefaultCryptoTrustDomain
 {
 public:
-  explicit pkixcheck_CheckSignatureAlgorithmTrustDomain(
+  explicit pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain(
              const ByteString& issuer)
     : issuer(issuer)
   {
@@ -275,7 +346,8 @@ TEST_F(pkixcheck_CheckSignatureAlgorithm, BuildCertChain)
 
   Input subjectInput;
   ASSERT_EQ(Success, subjectInput.Init(subject.data(), subject.length()));
-  pkixcheck_CheckSignatureAlgorithmTrustDomain trustDomain(issuer);
+  pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain
+    trustDomain(issuer);
   Result rv = BuildCertChain(trustDomain, subjectInput, Now(),
                              EndEntityOrCA::MustBeEndEntity,
                              KeyUsage::noParticularKeyUsageRequired,

security/pkix/test/gtest/pkixgtest.h

@@ -125,6 +125,13 @@ public:
                       Result::FATAL_ERROR_LIBRARY_FAILURE);
   }
 
+  Result CheckSignatureDigestAlgorithm(DigestAlgorithm) override
+  {
+    ADD_FAILURE();
+    return NotReached("CheckSignatureDigestAlgorithm should not be called",
+                      Result::FATAL_ERROR_LIBRARY_FAILURE);
+  }
+
   Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
   {
     ADD_FAILURE();
@@ -163,6 +170,11 @@ class DefaultCryptoTrustDomain : public EverythingFailsByDefaultTrustDomain
     return TestDigestBuf(item, digestAlg, digestBuf, digestBufLen);
   }
 
+  Result CheckSignatureDigestAlgorithm(DigestAlgorithm) override
+  {
+    return Success;
+  }
+
   Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
   {
     return Success;