Bug 1122841, Part 2: Centralize checking of public key, r=keeler

--HG--
extra : rebase_source : 6b41ad2d3f37bead8d3ac8b48c5ee0b8063c795b
extra : source : d470b5a68bf915cfb12f0e948e1492463092883c

security/apps/AppTrustDomain.cpp

@@ -217,15 +217,14 @@ Result
 AppTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData,
                                  Input subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::VerifySignedDataNSS(signedData, subjectPublicKeyInfo,
-                                              mMinimumNonECCBits, mPinArg);
+  return VerifySignedDataNSS(signedData, subjectPublicKeyInfo, mPinArg);
 }
 
 Result
 AppTrustDomain::DigestBuf(Input item, /*out*/ uint8_t* digestBuf,
                           size_t digestBufLen)
 {
-  return ::mozilla::pkix::DigestBufNSS(item, digestBuf, digestBufLen);
+  return DigestBufNSS(item, digestBuf, digestBufLen);
 }
 
 Result
@@ -250,10 +249,27 @@ AppTrustDomain::IsChainValid(const DERArray& certChain, Time time)
 }
 
 Result
-AppTrustDomain::CheckPublicKey(Input subjectPublicKeyInfo)
+AppTrustDomain::CheckRSAPublicKeyModulusSizeInBits(
+  EndEntityOrCA /*endEntityOrCA*/, unsigned int modulusSizeInBits)
 {
-  return ::mozilla::pkix::CheckPublicKeyNSS(subjectPublicKeyInfo,
-                                            mMinimumNonECCBits);
+  if (modulusSizeInBits < mMinimumNonECCBits) {
+    return Result::ERROR_INADEQUATE_KEY_SIZE;
+  }
+  return Success;
+}
+
+Result
+AppTrustDomain::CheckECDSACurveIsAcceptable(EndEntityOrCA /*endEntityOrCA*/,
+                                            NamedCurve curve)
+{
+  switch (curve) {
+    case NamedCurve::secp256r1: // fall through
+    case NamedCurve::secp384r1: // fall through
+    case NamedCurve::secp521r1:
+      return Success;
+  }
+
+  return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
 }
 
 } } // namespace mozilla::psm

security/apps/AppTrustDomain.h

@@ -38,8 +38,12 @@ 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 CheckPublicKey(mozilla::pkix::Input subjectPublicKeyInfo)
-                                MOZ_OVERRIDE;
+  virtual Result CheckRSAPublicKeyModulusSizeInBits(
+                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
+                   unsigned int modulusSizeInBits) MOZ_OVERRIDE;
+  virtual Result CheckECDSACurveIsAcceptable(
+                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
+                   mozilla::pkix::NamedCurve curve) MOZ_OVERRIDE;
   virtual Result VerifySignedData(
            const mozilla::pkix::SignedDataWithSignature& signedData,
            mozilla::pkix::Input subjectPublicKeyInfo) MOZ_OVERRIDE;

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -260,15 +260,14 @@ Result
 NSSCertDBTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData,
                                        Input subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::VerifySignedDataNSS(signedData, subjectPublicKeyInfo,
-                                              mMinimumNonECCBits, mPinArg);
+  return VerifySignedDataNSS(signedData, subjectPublicKeyInfo, mPinArg);
 }
 
 Result
 NSSCertDBTrustDomain::DigestBuf(Input item,
                                 /*out*/ uint8_t* digestBuf, size_t digestBufLen)
 {
-  return ::mozilla::pkix::DigestBufNSS(item, digestBuf, digestBufLen);
+  return DigestBufNSS(item, digestBuf, digestBufLen);
 }
 
 
@@ -721,10 +720,27 @@ NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time)
 }
 
 Result
-NSSCertDBTrustDomain::CheckPublicKey(Input subjectPublicKeyInfo)
+NSSCertDBTrustDomain::CheckRSAPublicKeyModulusSizeInBits(
+  EndEntityOrCA /*endEntityOrCA*/, unsigned int modulusSizeInBits)
 {
-  return ::mozilla::pkix::CheckPublicKeyNSS(subjectPublicKeyInfo,
-                                            mMinimumNonECCBits);
+  if (modulusSizeInBits < mMinimumNonECCBits) {
+    return Result::ERROR_INADEQUATE_KEY_SIZE;
+  }
+  return Success;
+}
+
+Result
+NSSCertDBTrustDomain::CheckECDSACurveIsAcceptable(
+  EndEntityOrCA /*endEntityOrCA*/, NamedCurve curve)
+{
+  switch (curve) {
+    case NamedCurve::secp256r1: // fall through
+    case NamedCurve::secp384r1: // fall through
+    case NamedCurve::secp521r1:
+      return Success;
+  }
+
+  return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
 }
 
 namespace {

security/certverifier/NSSCertDBTrustDomain.h

@@ -70,8 +70,13 @@ public:
                               /*out*/ mozilla::pkix::TrustLevel& trustLevel)
                               MOZ_OVERRIDE;
 
-  virtual Result CheckPublicKey(mozilla::pkix::Input subjectPublicKeyInfo)
-                                MOZ_OVERRIDE;
+  virtual Result CheckRSAPublicKeyModulusSizeInBits(
+                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
+                   unsigned int modulusSizeInBits) MOZ_OVERRIDE;
+
+  virtual Result CheckECDSACurveIsAcceptable(
+                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
+                   mozilla::pkix::NamedCurve curve) MOZ_OVERRIDE;
 
   virtual Result VerifySignedData(
                    const mozilla::pkix::SignedDataWithSignature& signedData,

security/manager/ssl/tests/unit/test_keysize_ev.js

@@ -127,9 +127,7 @@ function checkRSAChains(inadequateKeySize, adequateKeySize) {
   // adequate size for DV
   intFullName = intNotOKName + "-" + rootOKName;
   eeFullName = eeOKName + "-" + intNotOKName + "-" + rootOKName;
-  expectedNamesForOCSP = gEVExpected
-                       ? [ intFullName ]
-                       : [ eeFullName ];
+  expectedNamesForOCSP = [ eeFullName ];
   addKeySizeTestForEV(expectedNamesForOCSP, rootOKCertFileName,
                       [ intFullName ], eeFullName, false);
 
@@ -137,7 +135,10 @@ function checkRSAChains(inadequateKeySize, adequateKeySize) {
   // adequate size for DV
   intFullName = intOKName + "-" + rootOKName;
   eeFullName = eeNotOKName + "-" + intOKName + "-" + rootOKName;
-  expectedNamesForOCSP = [ eeFullName ];
+  expectedNamesForOCSP = gEVExpected
+                       ? [ intFullName,
+                           eeFullName ]
+                       : [ eeFullName ];
   addKeySizeTestForEV(expectedNamesForOCSP, rootOKCertFileName,
                       [ intFullName ], eeFullName, false);
 }

security/manager/ssl/tests/unit/test_ocsp_stapling.js

@@ -141,7 +141,7 @@ function add_tests(certDB, otherTestCA) {
   // Check that OCSP responder certificates with key sizes below 1024 bits are
   // rejected, even when the main certificate chain keys are at least 1024 bits.
   add_ocsp_test("keysize-ocsp-delegated.example.com",
-                getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE),
+                getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT),
                 true);
 
   add_ocsp_test("revoked-ca-cert-used-as-end-entity.example.com",

security/pkix/include/pkix/Result.h

@@ -177,6 +177,8 @@ static const unsigned int FATAL_ERROR_FLAG = 0x800;
                      MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE) \
     MOZILLA_PKIX_MAP(ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE, 46, \
                      MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE) \
+    MOZILLA_PKIX_MAP(ERROR_UNSUPPORTED_EC_POINT_FORM, 47, \
+                     SEC_ERROR_UNSUPPORTED_EC_POINT_FORM) \
     MOZILLA_PKIX_MAP(FATAL_ERROR_INVALID_ARGS, FATAL_ERROR_FLAG | 1, \
                      SEC_ERROR_INVALID_ARGS) \
     MOZILLA_PKIX_MAP(FATAL_ERROR_INVALID_STATE, FATAL_ERROR_FLAG | 2, \

security/pkix/include/pkix/pkixnss.h

@@ -34,7 +34,6 @@ namespace mozilla { namespace pkix {
 // Verify the given signed data using the given public key.
 Result VerifySignedDataNSS(const SignedDataWithSignature& sd,
                            Input subjectPublicKeyInfo,
-                           unsigned int minimumNonECCBits,
                            void* pkcs11PinArg);
 
 // Computes the SHA-1 hash of the data in the current item.
@@ -51,12 +50,6 @@ Result VerifySignedDataNSS(const SignedDataWithSignature& sd,
 Result DigestBufNSS(Input item, /*out*/ uint8_t* digestBuf,
                     size_t digestBufLen);
 
-// Checks, for RSA keys, that the modulus is at least the given number of bits.
-// Checks, for ECC keys, that the curve used is one of the NIST P-256, P-384,
-// or P-521 curves.
-Result CheckPublicKeyNSS(Input subjectPublicKeyInfo,
-                         unsigned int minimumNonECCBits);
-
 Result MapPRErrorCodeToResult(PRErrorCode errorCode);
 PRErrorCode MapResultToPRErrorCode(Result result);
 

security/pkix/include/pkix/pkixtypes.h

@@ -302,21 +302,33 @@ public:
                     /*optional*/ const Input* stapledOCSPresponse,
                     /*optional*/ const Input* aiaExtension) = 0;
 
-  // Check that the key size, algorithm, elliptic curve used (if applicable),
-  // and parameters of the given public key are acceptable.
+  // Check that the RSA public key size is 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 Result CheckPublicKey(Input subjectPublicKeyInfo) = 0;
+  // Return Success if the key size is acceptable,
+  // Result::ERROR_INADEQUATE_KEY_SIZE if the key size is not acceptable,
+  // or another error code if another error occurred.
+  virtual Result CheckRSAPublicKeyModulusSizeInBits(
+                   EndEntityOrCA endEntityOrCA,
+                   unsigned int modulusSizeInBits) = 0;
+
+  // Check that the given named ECC curve is acceptable for ECDSA signatures.
+  //
+  // Return Success if the curve is acceptable,
+  // Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE if the curve is not acceptable,
+  // or another error code if another error occurred.
+  virtual Result CheckECDSACurveIsAcceptable(EndEntityOrCA endEntityOrCA,
+                                             NamedCurve curve) = 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.
+  // CheckRSAPublicKeyModulusSizeInBits or CheckECDSACurveIsAcceptable will
+  // be called before calling this function, so it is not necessary to repeat
+  // those checks in VerifySignedData. However, VerifySignedData *is*
+  // responsible for doing the mathematical verification of the public key
+  // validity as specified in NIST SP 800-56A.
   virtual Result VerifySignedData(const SignedDataWithSignature& signedData,
                                   Input subjectPublicKeyInfo) = 0;
 

security/pkix/lib/pkixbuild.cpp

@@ -327,14 +327,6 @@ BuildCertChain(TrustDomain& trustDomain, Input certDER,
     return rv;
   }
 
-  // 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.
-  rv = trustDomain.CheckPublicKey(cert.GetSubjectPublicKeyInfo());
-  if (rv != Success) {
-    return rv;
-  }
-
   return BuildForward(trustDomain, cert, time, requiredKeyUsageIfPresent,
                       requiredEKUIfPresent, requiredPolicy, stapledOCSPResponse,
                       0/*subCACount*/);

security/pkix/lib/pkixcert.cpp

@@ -103,13 +103,6 @@ BackCert::Init()
   if (rv != Success) {
     return rv;
   }
-  // TODO(bug XXXXXXX): We defer parsing/validating subjectPublicKeyInfo to
-  // the point where the public key is needed. For end-entity certificates, we
-  // assume that the caller will extract the public key and use it somehow; if
-  // they don't do that then we'll never know whether the key is invalid. On
-  // the other hand, if the caller never uses the key then in some ways it
-  // doesn't matter. Regardless, we should parse and validate
-  // subjectPublicKeyKeyInfo internally.
   rv = der::ExpectTagAndGetTLV(tbsCertificate, der::SEQUENCE,
                                subjectPublicKeyInfo);
   if (rv != Success) {

security/pkix/lib/pkixcheck.cpp

@@ -29,6 +29,8 @@
 
 namespace mozilla { namespace pkix {
 
+// 4.1.2.5 Validity
+
 Result
 CheckValidity(Input encodedValidity, Time time)
 {
@@ -52,6 +54,187 @@ CheckValidity(Input encodedValidity, Time time)
   return der::End(validity);
 }
 
+// 4.1.2.7 Subject Public Key Info
+
+Result
+CheckSubjectPublicKeyInfo(Reader& input, TrustDomain& trustDomain,
+                          EndEntityOrCA endEntityOrCA)
+{
+  // Here, we validate the syntax and do very basic semantic validation of the
+  // public key of the certificate. The intention here is to filter out the
+  // types of bad inputs that are most likely to trigger non-mathematical
+  // security vulnerabilities in the TrustDomain, like buffer overflows or the
+  // use of unsafe elliptic curves.
+  //
+  // We don't check (all of) the mathematical properties of the public key here
+  // because it is more efficient for the TrustDomain to do it during signature
+  // verification and/or other use of the public key. In particular, we
+  // delegate the arithmetic validation of the public key, as specified in
+  // NIST SP800-56A section 5.6.2, to the TrustDomain, at least for now.
+
+  Reader algorithm;
+  Input subjectPublicKey;
+  Result rv = der::ExpectTagAndGetValue(input, der::SEQUENCE, algorithm);
+  if (rv != Success) {
+    return rv;
+  }
+  rv = der::BitStringWithNoUnusedBits(input, subjectPublicKey);
+  if (rv != Success) {
+    return rv;
+  }
+  rv = der::End(input);
+  if (rv != Success) {
+    return rv;
+  }
+
+  Reader subjectPublicKeyReader(subjectPublicKey);
+
+  Reader algorithmOID;
+  rv = der::ExpectTagAndGetValue(algorithm, der::OIDTag, algorithmOID);
+  if (rv != Success) {
+    return rv;
+  }
+
+  // RFC 3279 Section 2.3.1
+  // python DottedOIDToCode.py rsaEncryption 1.2.840.113549.1.1.1
+  static const uint8_t rsaEncryption[] = {
+    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01
+  };
+
+  // RFC 3279 Section 2.3.5 and RFC 5480 Section 2.1.1
+  // python DottedOIDToCode.py id-ecPublicKey 1.2.840.10045.2.1
+  static const uint8_t id_ecPublicKey[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01
+  };
+
+  if (algorithmOID.MatchRest(id_ecPublicKey)) {
+    // An id-ecPublicKey AlgorithmIdentifier has a parameter that identifes
+    // the curve being used. Although RFC 5480 specifies multiple forms, we
+    // only supported the NamedCurve form, where the curve is identified by an
+    // OID.
+
+    Reader namedCurveOIDValue;
+    rv = der::ExpectTagAndGetValue(algorithm, der::OIDTag,
+                                   namedCurveOIDValue);
+    if (rv != Success) {
+      return rv;
+    }
+
+    // RFC 5480
+    // python DottedOIDToCode.py secp256r1 1.2.840.10045.3.1.7
+    static const uint8_t secp256r1[] = {
+      0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07
+    };
+
+    // RFC 5480
+    // python DottedOIDToCode.py secp384r1 1.3.132.0.34
+    static const uint8_t secp384r1[] = {
+      0x2b, 0x81, 0x04, 0x00, 0x22
+    };
+
+    // RFC 5480
+    // python DottedOIDToCode.py secp521r1 1.3.132.0.35
+    static const uint8_t secp521r1[] = {
+      0x2b, 0x81, 0x04, 0x00, 0x23
+    };
+
+    // Matching is attempted based on a rough estimate of the commonality of the
+    // elliptic curve, to minimize the number of MatchRest calls.
+    NamedCurve curve;
+    unsigned int bits;
+    if (namedCurveOIDValue.MatchRest(secp256r1)) {
+      curve = NamedCurve::secp256r1;
+      bits = 256;
+    } else if (namedCurveOIDValue.MatchRest(secp384r1)) {
+      curve = NamedCurve::secp384r1;
+      bits = 384;
+    } else if (namedCurveOIDValue.MatchRest(secp521r1)) {
+      curve = NamedCurve::secp521r1;
+      bits = 521;
+    } else {
+      return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
+    }
+
+    rv = trustDomain.CheckECDSACurveIsAcceptable(endEntityOrCA, curve);
+    if (rv != Success) {
+      return rv;
+    }
+
+    // RFC 5480 Section 2.2 says that the first octet will be 0x04 to indicate
+    // an uncompressed point, which is the only encoding we support.
+    uint8_t compressedOrUncompressed;
+    rv = subjectPublicKeyReader.Read(compressedOrUncompressed);
+    if (rv != Success) {
+      return rv;
+    }
+    if (compressedOrUncompressed != 0x04) {
+      return Result::ERROR_UNSUPPORTED_EC_POINT_FORM;
+    }
+
+    // The point is encoded as two raw (not DER-encoded) integers, each padded
+    // to the bit length (rounded up to the nearest byte).
+    Input point;
+    rv = subjectPublicKeyReader.SkipToEnd(point);
+    if (rv != Success) {
+      return rv;
+    }
+    if (point.GetLength() != ((bits + 7) / 8u) * 2u) {
+      return Result::ERROR_BAD_DER;
+    }
+
+    // XXX: We defer the mathematical verification of the validity of the point
+    // until signature verification. This means that if we never verify a
+    // signature, we'll never fully check whether the public key is valid.
+  } else if (algorithmOID.MatchRest(rsaEncryption)) {
+    // RFC 3279 Section 2.3.1 says "The parameters field MUST have ASN.1 type
+    // NULL for this algorithm identifier."
+    rv = der::ExpectTagAndEmptyValue(algorithm, der::NULLTag);
+    if (rv != Success) {
+      return rv;
+    }
+
+    // RSAPublicKey :: = SEQUENCE{
+    //    modulus            INTEGER,    --n
+    //    publicExponent     INTEGER  }  --e
+    rv = der::Nested(subjectPublicKeyReader, der::SEQUENCE,
+                     [&trustDomain, endEntityOrCA](Reader& r) {
+      Input modulus;
+      Input::size_type modulusSignificantBytes;
+      Result rv = der::PositiveInteger(r, modulus, &modulusSignificantBytes);
+      if (rv != Success) {
+        return rv;
+      }
+      // XXX: Should we do additional checks of the modulus?
+      rv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
+             endEntityOrCA, modulusSignificantBytes * 8u);
+      if (rv != Success) {
+        return rv;
+      }
+
+      // XXX: We don't allow the TrustDomain to validate the exponent.
+      // XXX: We don't do our own sanity checking of the exponent.
+      Input exponent;
+      return der::PositiveInteger(r, exponent);
+    });
+    if (rv != Success) {
+      return rv;
+    }
+  } else {
+    return Result::ERROR_UNSUPPORTED_KEYALG;
+  }
+
+  rv = der::End(algorithm);
+  if (rv != Success) {
+    return rv;
+  }
+  rv = der::End(subjectPublicKeyReader);
+  if (rv != Success) {
+    return rv;
+  }
+
+  return Success;
+}
+
 // 4.2.1.3. Key Usage (id-ce-keyUsage)
 
 // As explained in the comment in CheckKeyUsage, bit 0 is the most significant
@@ -556,6 +739,21 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
     return Result::FATAL_ERROR_INVALID_STATE;
   }
 
+  // Check the SPKI first, because it is one of the most selective properties
+  // of the certificate due to SHA-1 deprecation and the deprecation of
+  // certificates with keys weaker than RSA 2048.
+  Reader spki(cert.GetSubjectPublicKeyInfo());
+  rv = der::Nested(spki, der::SEQUENCE, [&](Reader& r) {
+    return CheckSubjectPublicKeyInfo(r, trustDomain, endEntityOrCA);
+  });
+  if (rv != Success) {
+    return rv;
+  }
+  rv = der::End(spki);
+  if (rv != Success) {
+    return rv;
+  }
+
   // 4.2.1.1. Authority Key Identifier is ignored (see bug 965136).
 
   // 4.2.1.2. Subject Key Identifier is ignored (see bug 965136).

security/pkix/lib/pkixder.cpp

@@ -223,42 +223,6 @@ SignatureAlgorithmOIDValue(Reader& algorithmID,
   return Success;
 }
 
-static Result
-NamedCurveOIDValue(Reader& namedCurveID, /*out*/ NamedCurve& namedCurve)
-{
-  // RFC 5480
-  // python DottedOIDToCode.py secp256r1 1.2.840.10045.3.1.7
-  static const uint8_t secp256r1[] = {
-    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07
-  };
-
-  // RFC 5480
-  // python DottedOIDToCode.py id-secp384r1 1.3.132.0.34
-  static const uint8_t secp384r1[] = {
-    0x2b, 0x81, 0x04, 0x00, 0x22
-  };
-
-  // RFC 5480
-  // python DottedOIDToCode.py id-secp521r1 1.3.132.0.35
-  static const uint8_t secp521r1[] = {
-    0x2b, 0x81, 0x04, 0x00, 0x23
-  };
-
-  // Matching is attempted based on a rough estimate of the commonality of the
-  // named curve, to minimize the number of MatchRest calls.
-  if (namedCurveID.MatchRest(secp256r1)) {
-    namedCurve = NamedCurve::secp256r1;
-  } else if (namedCurveID.MatchRest(secp384r1)) {
-    namedCurve = NamedCurve::secp384r1;
-  } else if (namedCurveID.MatchRest(secp521r1)) {
-    namedCurve = NamedCurve::secp521r1;
-  } else {
-    return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
-  }
-
-  return Success;
-}
-
 template <typename OidValueParser, typename Algorithm>
 Result
 AlgorithmIdentifier(OidValueParser oidValueParser, Reader& input,
@@ -303,23 +267,6 @@ DigestAlgorithmIdentifier(Reader& input, /*out*/ DigestAlgorithm& algorithm)
   return AlgorithmIdentifier(DigestAlgorithmOIDValue, input, algorithm);
 }
 
-Result
-NamedCurveOID(Reader& input, /*out*/ NamedCurve& namedCurve)
-{
-  Reader namedCurveID;
-  Result rv = ExpectTagAndGetValue(input, der::OIDTag, namedCurveID);
-  if (rv != Success) {
-    return rv;
-  }
-
-  rv = NamedCurveOIDValue(namedCurveID, namedCurve);
-  if (rv != Success) {
-    return rv;
-  }
-
-  return Success;
-}
-
 Result
 SignedData(Reader& input, /*out*/ Reader& tbs,
            /*out*/ SignedDataWithSignature& signedData)

security/pkix/lib/pkixder.h

@@ -603,8 +603,6 @@ Result DigestAlgorithmIdentifier(Reader& input,
 Result SignatureAlgorithmIdentifier(Reader& input,
                                     /*out*/ SignatureAlgorithm& algorithm);
 
-Result NamedCurveOID(Reader& input, /*out*/ NamedCurve& namedCurve);
-
 // Parses a SEQUENCE into tbs and then parses an AlgorithmIdentifier followed
 // by a BIT STRING into signedData. This handles the commonality between
 // parsing the signed/signature fields of certificates and OCSP responses. In

security/pkix/lib/pkixnss.cpp

@@ -26,114 +26,20 @@
 
 #include <limits>
 
-#include "cert.h"
 #include "cryptohi.h"
 #include "keyhi.h"
 #include "pk11pub.h"
 #include "pkix/pkix.h"
 #include "pkix/ScopedPtr.h"
-#include "pkixder.h"
 #include "pkixutil.h"
 #include "secerr.h"
 #include "sslerr.h"
 
 namespace mozilla { namespace pkix {
 
-typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> ScopedSECKeyPublicKey;
-
-static Result
-CheckPublicKeySize(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits,
-                   /*out*/ ScopedSECKeyPublicKey& publicKey)
-{
-  SECItem subjectPublicKeyInfoSECItem =
-    UnsafeMapInputToSECItem(subjectPublicKeyInfo);
-  ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
-    spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfoSECItem));
-  if (!spki) {
-    return MapPRErrorCodeToResult(PR_GetError());
-  }
-  publicKey = SECKEY_ExtractPublicKey(spki.get());
-  if (!publicKey) {
-    return MapPRErrorCodeToResult(PR_GetError());
-  }
-
-  // Some compilers complain if if we don't explicitly list every case. That is
-  // usually what we want, but in this case we really want to support an
-  // open-ended set of key types that might be expanded by future NSS versions.
-#if defined(__clang__)
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wswitch-enum"
-#elif defined(_MSC_VER)
-#pragma warning(push)
-#pragma warning(disable: 4061)
-#endif
-
-  switch (publicKey.get()->keyType) {
-    case ecKey:
-    {
-      SECKEYECParams* encodedParams = &publicKey.get()->u.ec.DEREncodedParams;
-      if (!encodedParams) {
-        return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
-      }
-
-      Input input;
-      Result rv = input.Init(encodedParams->data, encodedParams->len);
-      if (rv != Success) {
-        return rv;
-      }
-
-      Reader reader(input);
-      NamedCurve namedCurve;
-      rv = der::NamedCurveOID(reader, namedCurve);
-      if (rv != Success) {
-        return rv;
-      }
-
-      rv = der::End(reader);
-      if (rv != Success) {
-        return rv;
-      }
-
-      switch (namedCurve) {
-        case NamedCurve::secp256r1: // fall through
-        case NamedCurve::secp384r1: // fall through
-        case NamedCurve::secp521r1:
-          break;
-      }
-
-      return Success;
-    }
-
-    case rsaKey:
-      if (SECKEY_PublicKeyStrengthInBits(publicKey.get()) < minimumNonECCBits) {
-        return Result::ERROR_INADEQUATE_KEY_SIZE;
-      }
-      break;
-
-    default:
-      return Result::ERROR_UNSUPPORTED_KEYALG;
-  }
-
-#if defined(__clang__)
-#pragma clang diagnostic pop
-#elif defined(_MSC_VER)
-#pragma warning(pop)
-#endif
-
-  return Success;
-}
-
-Result
-CheckPublicKeyNSS(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits)
-{
-  ScopedSECKeyPublicKey unused;
-  return CheckPublicKeySize(subjectPublicKeyInfo, minimumNonECCBits, unused);
-}
-
 Result
 VerifySignedDataNSS(const SignedDataWithSignature& sd,
-                    Input subjectPublicKeyInfo, unsigned int minimumNonECCBits,
-                    void* pkcs11PinArg)
+                    Input subjectPublicKeyInfo, void* pkcs11PinArg)
 {
   SECOidTag pubKeyAlg;
   SECOidTag digestAlg;
@@ -176,11 +82,17 @@ VerifySignedDataNSS(const SignedDataWithSignature& sd,
     MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM
   }
 
-  Result rv;
-  ScopedSECKeyPublicKey pubKey;
-  rv = CheckPublicKeySize(subjectPublicKeyInfo, minimumNonECCBits, pubKey);
-  if (rv != Success) {
-    return rv;
+  SECItem subjectPublicKeyInfoSECItem =
+    UnsafeMapInputToSECItem(subjectPublicKeyInfo);
+  ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
+    spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfoSECItem));
+  if (!spki) {
+    return MapPRErrorCodeToResult(PR_GetError());
+  }
+  ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
+    pubKey(SECKEY_ExtractPublicKey(spki.get()));
+  if (!pubKey) {
+    return MapPRErrorCodeToResult(PR_GetError());
   }
 
   // The static_cast is safe as long as the length of the data in sd.data can

security/pkix/test/gtest/pkixbuild_tests.cpp

@@ -165,11 +165,18 @@ private:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
   }
 
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
+  }
+
+
   std::map<ByteString, ByteString> subjectDERToCertDER;
   ByteString leafCACertDER;
   ByteString rootCACertDER;
@@ -327,9 +334,15 @@ public:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 
 private:
@@ -410,9 +423,15 @@ public:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 };
 
@@ -507,9 +526,15 @@ public:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 
 private:

security/pkix/test/gtest/pkixcert_extension_tests.cpp

@@ -101,9 +101,15 @@ private:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 };
 

security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp

@@ -116,9 +116,15 @@ private:
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 
   ByteString rootDER;

security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp

@@ -71,10 +71,15 @@ private:
     return TestDigestBuf(item, digestBuf, digestBufLen);
   }
 
-  Result CheckPublicKey(Input) override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            override
   {
-    ADD_FAILURE();
-    return Result::FATAL_ERROR_LIBRARY_FAILURE;
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override
+  {
+    return Success;
   }
 };
 

security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp

@@ -82,9 +82,15 @@ public:
     return TestDigestBuf(item, digestBuf, digestBufLen);
   }
 
-  Result CheckPublicKey(Input subjectPublicKeyInfo) final override
+  Result CheckRSAPublicKeyModulusSizeInBits(EndEntityOrCA, unsigned int)
+                                            final override
   {
-    return TestCheckPublicKey(subjectPublicKeyInfo);
+    return Success;
+  }
+
+  Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) final override
+  {
+    return Success;
   }
 
   OCSPTestTrustDomain(const OCSPTestTrustDomain&) = delete;

security/pkix/test/lib/pkixtestnss.cpp

@@ -384,20 +384,12 @@ SHA1(const ByteString& toHash)
   return ByteString(digestBuf, sizeof(digestBuf));
 }
 
-Result
-TestCheckPublicKey(Input subjectPublicKeyInfo)
-{
-  InitNSSIfNeeded();
-  return CheckPublicKeyNSS(subjectPublicKeyInfo, MINIMUM_TEST_KEY_BITS);
-}
-
 Result
 TestVerifySignedData(const SignedDataWithSignature& signedData,
                      Input subjectPublicKeyInfo)
 {
   InitNSSIfNeeded();
-  return VerifySignedDataNSS(signedData, subjectPublicKeyInfo,
-                             MINIMUM_TEST_KEY_BITS, nullptr);
+  return VerifySignedDataNSS(signedData, subjectPublicKeyInfo, nullptr);
 }
 
 Result

security/pkix/test/lib/pkixtestutil.h

@@ -32,8 +32,6 @@
 #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;