diff --git a/security/apps/AppTrustDomain.cpp b/security/apps/AppTrustDomain.cpp index f9f01d7cd16..d921e7491b0 100644 --- a/security/apps/AppTrustDomain.cpp +++ b/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 diff --git a/security/apps/AppTrustDomain.h b/security/apps/AppTrustDomain.h index 2c79098e19d..2d0810df788 100644 --- a/security/apps/AppTrustDomain.h +++ b/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; diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index 35fef53cde3..f5b874b9683 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/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 { diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h index d5d689fea2f..f6bfb0292fb 100644 --- a/security/certverifier/NSSCertDBTrustDomain.h +++ b/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, diff --git a/security/manager/ssl/tests/unit/test_keysize_ev.js b/security/manager/ssl/tests/unit/test_keysize_ev.js index dc689ab8e25..c7039a12640 100644 --- a/security/manager/ssl/tests/unit/test_keysize_ev.js +++ b/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); } diff --git a/security/manager/ssl/tests/unit/test_ocsp_stapling.js b/security/manager/ssl/tests/unit/test_ocsp_stapling.js index 50b9468aed8..7b1d99c7be1 100644 --- a/security/manager/ssl/tests/unit/test_ocsp_stapling.js +++ b/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", diff --git a/security/pkix/include/pkix/Result.h b/security/pkix/include/pkix/Result.h index 97f5b3f121f..f78169bf423 100644 --- a/security/pkix/include/pkix/Result.h +++ b/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, \ diff --git a/security/pkix/include/pkix/pkixnss.h b/security/pkix/include/pkix/pkixnss.h index 33d5fe022cb..5ef30132831 100644 --- a/security/pkix/include/pkix/pkixnss.h +++ b/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); diff --git a/security/pkix/include/pkix/pkixtypes.h b/security/pkix/include/pkix/pkixtypes.h index d70d414f884..f0b50a8cab1 100644 --- a/security/pkix/include/pkix/pkixtypes.h +++ b/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; diff --git a/security/pkix/lib/pkixbuild.cpp b/security/pkix/lib/pkixbuild.cpp index 1bb7f1c1899..02009a9441f 100644 --- a/security/pkix/lib/pkixbuild.cpp +++ b/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*/); diff --git a/security/pkix/lib/pkixcert.cpp b/security/pkix/lib/pkixcert.cpp index 6371f5d30b5..9639b723edb 100644 --- a/security/pkix/lib/pkixcert.cpp +++ b/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) { diff --git a/security/pkix/lib/pkixcheck.cpp b/security/pkix/lib/pkixcheck.cpp index 3524b2b14df..609487120af 100644 --- a/security/pkix/lib/pkixcheck.cpp +++ b/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). diff --git a/security/pkix/lib/pkixder.cpp b/security/pkix/lib/pkixder.cpp index 9154cf85f58..fff9abcf9d0 100644 --- a/security/pkix/lib/pkixder.cpp +++ b/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 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) diff --git a/security/pkix/lib/pkixder.h b/security/pkix/lib/pkixder.h index efa599f8d64..036049013cc 100644 --- a/security/pkix/lib/pkixder.h +++ b/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 diff --git a/security/pkix/lib/pkixnss.cpp b/security/pkix/lib/pkixnss.cpp index 72187cf692d..9471e2ffeb2 100644 --- a/security/pkix/lib/pkixnss.cpp +++ b/security/pkix/lib/pkixnss.cpp @@ -26,114 +26,20 @@ #include -#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 ScopedSECKeyPublicKey; - -static Result -CheckPublicKeySize(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits, - /*out*/ ScopedSECKeyPublicKey& publicKey) -{ - SECItem subjectPublicKeyInfoSECItem = - UnsafeMapInputToSECItem(subjectPublicKeyInfo); - ScopedPtr - 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 + spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfoSECItem)); + if (!spki) { + return MapPRErrorCodeToResult(PR_GetError()); + } + ScopedPtr + 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 diff --git a/security/pkix/test/gtest/pkixbuild_tests.cpp b/security/pkix/test/gtest/pkixbuild_tests.cpp index 6bf6e9b8487..efabe691491 100644 --- a/security/pkix/test/gtest/pkixbuild_tests.cpp +++ b/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 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: diff --git a/security/pkix/test/gtest/pkixcert_extension_tests.cpp b/security/pkix/test/gtest/pkixcert_extension_tests.cpp index 1b01f7e4574..4d7bc559bf0 100644 --- a/security/pkix/test/gtest/pkixcert_extension_tests.cpp +++ b/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; } }; diff --git a/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp b/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp index 4807ab99fc8..cac334849ea 100644 --- a/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp +++ b/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; diff --git a/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp b/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp index 624995031d2..365a1e10a23 100644 --- a/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp +++ b/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; } }; diff --git a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp b/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp index 97aaa2d19b8..d18b721e987 100644 --- a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp +++ b/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; diff --git a/security/pkix/test/lib/pkixtestnss.cpp b/security/pkix/test/lib/pkixtestnss.cpp index 752ea46e44c..2c4453ce134 100644 --- a/security/pkix/test/lib/pkixtestnss.cpp +++ b/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 diff --git a/security/pkix/test/lib/pkixtestutil.h b/security/pkix/test/lib/pkixtestutil.h index 414163cf147..13ed4396859 100644 --- a/security/pkix/test/lib/pkixtestutil.h +++ b/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 ByteString;