From 01941f880c9f00073a71fd1d7d4cd55c1ced52a1 Mon Sep 17 00:00:00 2001 From: Cykesiopka Date: Thu, 16 Oct 2014 05:13:00 +0200 Subject: [PATCH] Bug 622859 - Reject EV certificates with key sizes below RSA 2048. r=briansmith --- security/apps/AppTrustDomain.cpp | 7 ++++-- security/certverifier/CertVerifier.cpp | 25 ++++++++++--------- .../certverifier/NSSCertDBTrustDomain.cpp | 10 ++++++-- security/certverifier/NSSCertDBTrustDomain.h | 2 ++ security/pkix/include/pkix/pkixnss.h | 7 ++++-- security/pkix/lib/pkixnss.cpp | 18 ++++++------- security/pkix/test/gtest/pkixbuild_tests.cpp | 4 +-- .../pkixcert_signature_algorithm_tests.cpp | 2 +- security/pkix/test/lib/pkixtestnss.cpp | 5 ++-- security/pkix/test/lib/pkixtestutil.h | 2 ++ 10 files changed, 49 insertions(+), 33 deletions(-) diff --git a/security/apps/AppTrustDomain.cpp b/security/apps/AppTrustDomain.cpp index 0723aca1c6f5..b6eb67dc2b3a 100644 --- a/security/apps/AppTrustDomain.cpp +++ b/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 diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index 869c93f60e56..59dba7a914d0 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/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, - builtChain); + ocspGETConfig, pinningDisabled, false, + 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, - builtChain); + pinningDisabled, false, + nullptr, builtChain); rv = BuildCertChain(objectSigningTrust, certDER, time, endEntityOrCA, keyUsage, eku, CertPolicyId::anyPolicy, stapledOCSPResponse); diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index bea082f41d79..efef4f215b16 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/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 { diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h index fa66fef4d801..15de082b303b 100644 --- a/security/certverifier/NSSCertDBTrustDomain.h +++ b/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 }; diff --git a/security/pkix/include/pkix/pkixnss.h b/security/pkix/include/pkix/pkixnss.h index 26ffcf014c4f..17af33a540da 100644 --- a/security/pkix/include/pkix/pkixnss.h +++ b/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. -Result CheckPublicKey(Input subjectPublicKeyInfo); +// Checks, for RSA keys and DSA keys, that the modulus is at least the given +// number of bits. +Result CheckPublicKey(Input subjectPublicKeyInfo, + unsigned int minimumNonECCBits); Result MapPRErrorCodeToResult(PRErrorCode errorCode); PRErrorCode MapResultToPRErrorCode(Result result); diff --git a/security/pkix/lib/pkixnss.cpp b/security/pkix/lib/pkixnss.cpp index deec8260ebf3..1aee8af40b11 100644 --- a/security/pkix/lib/pkixnss.cpp +++ b/security/pkix/lib/pkixnss.cpp @@ -39,7 +39,7 @@ namespace mozilla { namespace pkix { typedef ScopedPtr 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()) < MINIMUM_NON_ECC_BITS) { + if (SECKEY_PublicKeyStrengthInBits(publicKey.get()) < minimumNonECCBits) { 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; } diff --git a/security/pkix/test/gtest/pkixbuild_tests.cpp b/security/pkix/test/gtest/pkixbuild_tests.cpp index 089f141f20e2..aad84fbe9369 100644 --- a/security/pkix/test/gtest/pkixbuild_tests.cpp +++ b/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) diff --git a/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp b/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp index ee9cf47327e8..fce353568b8c 100644 --- a/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp +++ b/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) diff --git a/security/pkix/test/lib/pkixtestnss.cpp b/security/pkix/test/lib/pkixtestnss.cpp index aea3b8677a55..f763837f0165 100644 --- a/security/pkix/test/lib/pkixtestnss.cpp +++ b/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 diff --git a/security/pkix/test/lib/pkixtestutil.h b/security/pkix/test/lib/pkixtestutil.h index c09f8bc18d4b..80bb480d8770 100644 --- a/security/pkix/test/lib/pkixtestutil.h +++ b/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 ByteString;