From fd860abf57d5742967618798521d945b40095d6b Mon Sep 17 00:00:00 2001 From: David Keeler Date: Thu, 25 Sep 2014 11:18:56 -0700 Subject: [PATCH] bug 1071308 - (2/2) remove libpkix-style chain validation callback from CertVerifier r=cviecco --- security/apps/AppTrustDomain.cpp | 2 +- security/apps/AppTrustDomain.h | 4 +- security/certverifier/CertVerifier.cpp | 148 +++++++----------- security/certverifier/CertVerifier.h | 3 + .../certverifier/NSSCertDBTrustDomain.cpp | 33 ++-- security/certverifier/NSSCertDBTrustDomain.h | 12 +- security/pkix/include/pkix/pkixtypes.h | 2 +- security/pkix/lib/pkixbuild.cpp | 2 +- security/pkix/test/gtest/pkixbuild_tests.cpp | 2 +- .../test/gtest/pkixcert_extension_tests.cpp | 2 +- ...kixocsp_CreateEncodedOCSPRequest_tests.cpp | 2 +- .../pkixocsp_VerifyEncodedOCSPResponse.cpp | 2 +- 12 files changed, 83 insertions(+), 131 deletions(-) diff --git a/security/apps/AppTrustDomain.cpp b/security/apps/AppTrustDomain.cpp index 10e2ec9cae3c..da0361270546 100644 --- a/security/apps/AppTrustDomain.cpp +++ b/security/apps/AppTrustDomain.cpp @@ -238,7 +238,7 @@ AppTrustDomain::CheckRevocation(EndEntityOrCA, const CertID&, Time, } Result -AppTrustDomain::IsChainValid(const DERArray& certChain) +AppTrustDomain::IsChainValid(const DERArray& certChain, Time time) { SECStatus srv = ConstructCERTCertListFromReversedDERArray(certChain, mCertChain); diff --git a/security/apps/AppTrustDomain.h b/security/apps/AppTrustDomain.h index e8abf1531a5b..5bf53bff2a18 100644 --- a/security/apps/AppTrustDomain.h +++ b/security/apps/AppTrustDomain.h @@ -36,8 +36,8 @@ public: mozilla::pkix::Time time, /*optional*/ const mozilla::pkix::Input* stapledOCSPresponse, /*optional*/ const mozilla::pkix::Input* aiaExtension); - virtual Result IsChainValid(const mozilla::pkix::DERArray& certChain) - 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 VerifySignedData( diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index cd47916aca64..869c93f60e56 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -81,78 +81,45 @@ IsCertBuiltInRoot(CERTCertificate* cert, bool& result) { return SECSuccess; } -struct ChainValidationCallbackState +Result +CertListContainsExpectedKeys(const CERTCertList* certList, + const char* hostname, Time time, + CertVerifier::PinningMode pinningMode) { - const char* hostname; - const CertVerifier::PinningMode pinningMode; - const SECCertificateUsage usage; - const Time time; -}; + if (pinningMode == CertVerifier::pinningDisabled) { + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, + ("Pinning is disabled; not checking keys.")); + return Success; + } -SECStatus chainValidationCallback(void* state, const CERTCertList* certList, - PRBool* chainOK) -{ - ChainValidationCallbackState* callbackState = - reinterpret_cast(state); - - *chainOK = PR_FALSE; - - PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, - ("verifycert: Inside the Callback \n")); - - // On sanity failure we fail closed. if (!certList) { - PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, - ("verifycert: Short circuit, callback, sanity check failed \n")); - PR_SetError(PR_INVALID_STATE_ERROR, 0); - return SECFailure; - } - if (!callbackState) { - PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, - ("verifycert: Short circuit, callback, no state! \n")); - PR_SetError(PR_INVALID_STATE_ERROR, 0); - return SECFailure; + return Result::FATAL_ERROR_INVALID_ARGS; } - if (callbackState->usage != certificateUsageSSLServer || - callbackState->pinningMode == CertVerifier::pinningDisabled) { - PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, - ("verifycert: Callback shortcut pel=%d \n", - callbackState->pinningMode)); - *chainOK = PR_TRUE; - return SECSuccess; + CERTCertListNode* rootNode = CERT_LIST_TAIL(certList); + if (CERT_LIST_END(rootNode, certList)) { + return Result::FATAL_ERROR_INVALID_ARGS; } - for (CERTCertListNode* node = CERT_LIST_HEAD(certList); - !CERT_LIST_END(node, certList); - node = CERT_LIST_NEXT(node)) { - CERTCertificate* currentCert = node->cert; - if (CERT_LIST_END(CERT_LIST_NEXT(node), certList)) { - bool isBuiltInRoot = false; - SECStatus srv = IsCertBuiltInRoot(currentCert, isBuiltInRoot); - if (srv != SECSuccess) { - PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("Is BuiltInRoot failure")); - return srv; - } - // If desired, the user can enable "allow user CA MITM mode", in which - // case key pinning is not enforced for certificates that chain to trust - // anchors that are not in Mozilla's root program - if (!isBuiltInRoot && - (callbackState->pinningMode == - CertVerifier::pinningAllowUserCAMITM)) { - *chainOK = PR_TRUE; - return SECSuccess; - } - } + bool isBuiltInRoot = false; + SECStatus srv = IsCertBuiltInRoot(rootNode->cert, isBuiltInRoot); + if (srv != SECSuccess) { + return MapPRErrorCodeToResult(PR_GetError()); + } + // If desired, the user can enable "allow user CA MITM mode", in which + // case key pinning is not enforced for certificates that chain to trust + // anchors that are not in Mozilla's root program + if (!isBuiltInRoot && pinningMode == CertVerifier::pinningAllowUserCAMITM) { + return Success; } - bool enforceTestMode = (callbackState->pinningMode == - CertVerifier::pinningEnforceTestMode); - *chainOK = PublicKeyPinningService:: - ChainHasValidPins(certList, callbackState->hostname, callbackState->time, - enforceTestMode); + bool enforceTestMode = (pinningMode == CertVerifier::pinningEnforceTestMode); + if (PublicKeyPinningService::ChainHasValidPins(certList, hostname, time, + enforceTestMode)) { + return Success; + } - return SECSuccess; + return Result::ERROR_KEY_PINNING_FAILURE; } static Result @@ -216,13 +183,6 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, return SECFailure; } - ChainValidationCallbackState callbackState = { - hostname, mPinningMode, usage, time - }; - CERTChainVerifyCallback callbackContainer; - callbackContainer.isChainValid = chainValidationCallback; - callbackContainer.isChainValidArg = &callbackState; - NSSCertDBTrustDomain::OCSPFetching ocspFetching = !mOCSPDownloadEnabled || (flags & FLAG_LOCAL_ONLY) ? NSSCertDBTrustDomain::NeverFetchOCSP @@ -250,8 +210,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, // XXX: We don't really have a trust bit for SSL client authentication so // just use trustEmail as it is the closest alternative. NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache, - pinArg, ocspGETConfig, nullptr, - builtChain); + pinArg, ocspGETConfig, pinningDisabled, + nullptr, builtChain); rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity, KeyUsage::digitalSignature, @@ -276,8 +236,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, ocspFetching == NSSCertDBTrustDomain::NeverFetchOCSP ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV : NSSCertDBTrustDomain::FetchOCSPForEV, - mOCSPCache, pinArg, ocspGETConfig, - &callbackContainer, builtChain); + mOCSPCache, pinArg, ocspGETConfig, mPinningMode, + hostname, builtChain); rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time, KeyUsage::digitalSignature,// (EC)DHE KeyUsage::keyEncipherment, // RSA @@ -300,8 +260,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, // Now try non-EV. NSSCertDBTrustDomain trustDomain(trustSSL, ocspFetching, mOCSPCache, - pinArg, ocspGETConfig, &callbackContainer, - builtChain); + pinArg, ocspGETConfig, mPinningMode, + hostname, builtChain); rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time, KeyUsage::digitalSignature, // (EC)DHE KeyUsage::keyEncipherment, // RSA @@ -314,8 +274,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, case certificateUsageSSLCA: { NSSCertDBTrustDomain trustDomain(trustSSL, ocspFetching, mOCSPCache, - pinArg, ocspGETConfig, nullptr, - builtChain); + pinArg, ocspGETConfig, pinningDisabled, + nullptr, builtChain); rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeCA, KeyUsage::keyCertSign, KeyPurposeId::id_kp_serverAuth, @@ -325,8 +285,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, case certificateUsageEmailSigner: { NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache, - pinArg, ocspGETConfig, nullptr, - builtChain); + pinArg, ocspGETConfig, pinningDisabled, + nullptr, builtChain); rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity, KeyUsage::digitalSignature, @@ -340,8 +300,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, // usage it is trying to verify for, and base its algorithm choices // based on the result of the verification(s). NSSCertDBTrustDomain trustDomain(trustEmail, ocspFetching, mOCSPCache, - pinArg, ocspGETConfig, nullptr, - builtChain); + pinArg, ocspGETConfig, pinningDisabled, + nullptr, builtChain); rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity, KeyUsage::keyEncipherment, // RSA @@ -360,7 +320,7 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, case certificateUsageObjectSigner: { NSSCertDBTrustDomain trustDomain(trustObjectSigning, ocspFetching, mOCSPCache, pinArg, ocspGETConfig, - nullptr, builtChain); + pinningDisabled, nullptr, builtChain); rv = BuildCertChain(trustDomain, certDER, time, EndEntityOrCA::MustBeEndEntity, KeyUsage::digitalSignature, @@ -388,14 +348,15 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, } NSSCertDBTrustDomain sslTrust(trustSSL, ocspFetching, mOCSPCache, pinArg, - ocspGETConfig, nullptr, builtChain); + ocspGETConfig, pinningDisabled, 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, nullptr, - builtChain); + pinArg, ocspGETConfig, pinningDisabled, + nullptr, builtChain); rv = BuildCertChain(emailTrust, certDER, time, endEntityOrCA, keyUsage, eku, CertPolicyId::anyPolicy, stapledOCSPResponse); @@ -403,7 +364,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, NSSCertDBTrustDomain objectSigningTrust(trustObjectSigning, ocspFetching, mOCSPCache, pinArg, ocspGETConfig, - nullptr, builtChain); + pinningDisabled, nullptr, + builtChain); rv = BuildCertChain(objectSigningTrust, certDER, time, endEntityOrCA, keyUsage, eku, CertPolicyId::anyPolicy, stapledOCSPResponse); @@ -418,7 +380,8 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, } if (rv != Success) { - if (rv != Result::ERROR_KEY_PINNING_FAILURE) { + if (rv != Result::ERROR_KEY_PINNING_FAILURE && + usage == certificateUsageSSLServer) { ScopedCERTCertificate certCopy(CERT_DupCertificate(cert)); if (!certCopy) { return SECFailure; @@ -432,13 +395,10 @@ CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage, return SECFailure; } certCopy.forget(); // now owned by certList - PRBool chainOK = false; - srv = chainValidationCallback(&callbackState, certList, &chainOK); - if (srv != SECSuccess) { - return SECFailure; - } - if (!chainOK) { - rv = Result::ERROR_KEY_PINNING_FAILURE; + Result pinningResult = CertListContainsExpectedKeys(certList, hostname, + time, mPinningMode); + if (pinningResult != Success) { + rv = pinningResult; } } PR_SetError(MapResultToPRErrorCode(rv), 0); diff --git a/security/certverifier/CertVerifier.h b/security/certverifier/CertVerifier.h index 8ce278ae11e8..62ce8af87f17 100644 --- a/security/certverifier/CertVerifier.h +++ b/security/certverifier/CertVerifier.h @@ -79,6 +79,9 @@ private: void InitCertVerifierLog(); SECStatus IsCertBuiltInRoot(CERTCertificate* cert, bool& result); +mozilla::pkix::Result CertListContainsExpectedKeys( + const CERTCertList* certList, const char* hostname, mozilla::pkix::Time time, + CertVerifier::PinningMode pinningMode); } } // namespace mozilla::psm diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp index 85fdd40332cf..bea082f41d79 100644 --- a/security/certverifier/NSSCertDBTrustDomain.cpp +++ b/security/certverifier/NSSCertDBTrustDomain.cpp @@ -52,14 +52,16 @@ NSSCertDBTrustDomain::NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPCache& ocspCache, /*optional but shouldn't be*/ void* pinArg, CertVerifier::ocsp_get_config ocspGETConfig, - /*optional*/ CERTChainVerifyCallback* checkChainCallback, + CertVerifier::PinningMode pinningMode, + /*optional*/ const char* hostname, /*optional*/ ScopedCERTCertList* builtChain) : mCertDBTrustType(certDBTrustType) , mOCSPFetching(ocspFetching) , mOCSPCache(ocspCache) , mPinArg(pinArg) , mOCSPGetConfig(ocspGETConfig) - , mCheckChainCallback(checkChainCallback) + , mPinningMode(pinningMode) + , mHostname(hostname) , mBuiltChain(builtChain) { } @@ -633,16 +635,10 @@ NSSCertDBTrustDomain::VerifyAndMaybeCacheEncodedOCSPResponse( } Result -NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray) +NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time) { PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, - ("NSSCertDBTrustDomain: Top of IsChainValid mCheckChainCallback=%p", - mCheckChainCallback)); - - if (!mBuiltChain && !mCheckChainCallback) { - // No need to create a CERTCertList, and nothing else to do. - return Success; - } + ("NSSCertDBTrustDomain: IsChainValid")); ScopedCERTCertList certList; SECStatus srv = ConstructCERTCertListFromReversedDERArray(certArray, @@ -651,19 +647,10 @@ NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray) return MapPRErrorCodeToResult(PR_GetError()); } - if (mCheckChainCallback) { - if (!mCheckChainCallback->isChainValid) { - return Result::FATAL_ERROR_INVALID_ARGS; - } - PRBool chainOK; - srv = (mCheckChainCallback->isChainValid)( - mCheckChainCallback->isChainValidArg, certList.get(), &chainOK); - if (srv != SECSuccess) { - return MapPRErrorCodeToResult(PR_GetError()); - } - if (!chainOK) { - return Result::ERROR_KEY_PINNING_FAILURE; - } + Result result = CertListContainsExpectedKeys(certList, mHostname, time, + mPinningMode); + if (result != Success) { + return result; } if (mBuiltChain) { diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h index 769b47200913..fa66fef4d801 100644 --- a/security/certverifier/NSSCertDBTrustDomain.h +++ b/security/certverifier/NSSCertDBTrustDomain.h @@ -53,8 +53,9 @@ public: NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, OCSPCache& ocspCache, void* pinArg, CertVerifier::ocsp_get_config ocspGETConfig, - /*optional*/ CERTChainVerifyCallback* checkChainCallback = nullptr, - /*optional*/ ScopedCERTCertList* builtChain = nullptr); + CertVerifier::PinningMode pinningMode, + /*optional*/ const char* hostname = nullptr, + /*optional out*/ ScopedCERTCertList* builtChain = nullptr); virtual Result FindIssuer(mozilla::pkix::Input encodedIssuerName, IssuerChecker& checker, @@ -86,8 +87,8 @@ public: /*optional*/ const mozilla::pkix::Input* aiaExtension) MOZ_OVERRIDE; - virtual Result IsChainValid(const mozilla::pkix::DERArray& certChain) - MOZ_OVERRIDE; + virtual Result IsChainValid(const mozilla::pkix::DERArray& certChain, + mozilla::pkix::Time time) MOZ_OVERRIDE; private: enum EncodedResponseSource { @@ -104,7 +105,8 @@ private: OCSPCache& mOCSPCache; // non-owning! void* mPinArg; // non-owning! const CertVerifier::ocsp_get_config mOCSPGetConfig; - CERTChainVerifyCallback* mCheckChainCallback; // non-owning! + CertVerifier::PinningMode mPinningMode; + const char* mHostname; // non-owning - only used for pinning checks ScopedCERTCertList* mBuiltChain; // non-owning }; diff --git a/security/pkix/include/pkix/pkixtypes.h b/security/pkix/include/pkix/pkixtypes.h index 09825812b0d4..b80c0faa6314 100644 --- a/security/pkix/include/pkix/pkixtypes.h +++ b/security/pkix/include/pkix/pkixtypes.h @@ -282,7 +282,7 @@ public: // very wrong to assume that the certificate chain is valid. // // certChain.GetDER(0) is the trust anchor. - virtual Result IsChainValid(const DERArray& certChain) = 0; + virtual Result IsChainValid(const DERArray& certChain, Time time) = 0; // issuerCertToDup is only non-const so CERT_DupCertificate can be called on // it. diff --git a/security/pkix/lib/pkixbuild.cpp b/security/pkix/lib/pkixbuild.cpp index 0a786c8790f3..a4fbd66dceb4 100644 --- a/security/pkix/lib/pkixbuild.cpp +++ b/security/pkix/lib/pkixbuild.cpp @@ -246,7 +246,7 @@ BuildForward(TrustDomain& trustDomain, // This must be done here, after the chain is built but before any // revocation checks have been done. - return trustDomain.IsChainValid(chain); + return trustDomain.IsChainValid(chain, time); } if (subject.endEntityOrCA == EndEntityOrCA::MustBeCA) { diff --git a/security/pkix/test/gtest/pkixbuild_tests.cpp b/security/pkix/test/gtest/pkixbuild_tests.cpp index cb468d2ab7de..f37fe3eeda9e 100644 --- a/security/pkix/test/gtest/pkixbuild_tests.cpp +++ b/security/pkix/test/gtest/pkixbuild_tests.cpp @@ -165,7 +165,7 @@ private: return Success; } - virtual Result IsChainValid(const DERArray&) + virtual Result IsChainValid(const DERArray&, Time) { return Success; } diff --git a/security/pkix/test/gtest/pkixcert_extension_tests.cpp b/security/pkix/test/gtest/pkixcert_extension_tests.cpp index 284d9b6b0594..3a2d7d67e274 100644 --- a/security/pkix/test/gtest/pkixcert_extension_tests.cpp +++ b/security/pkix/test/gtest/pkixcert_extension_tests.cpp @@ -87,7 +87,7 @@ private: return Success; } - virtual Result IsChainValid(const DERArray&) + virtual Result IsChainValid(const DERArray&, Time) { return Success; } diff --git a/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp b/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp index 4a159ebf3799..c8e93b6bb3ba 100644 --- a/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp +++ b/security/pkix/test/gtest/pkixocsp_CreateEncodedOCSPRequest_tests.cpp @@ -53,7 +53,7 @@ private: return Result::FATAL_ERROR_LIBRARY_FAILURE; } - virtual Result IsChainValid(const DERArray&) + virtual Result IsChainValid(const DERArray&, Time) { ADD_FAILURE(); return Result::FATAL_ERROR_LIBRARY_FAILURE; diff --git a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp b/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp index 10be77a32688..75320ecce931 100644 --- a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp +++ b/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp @@ -63,7 +63,7 @@ public: return Result::FATAL_ERROR_LIBRARY_FAILURE; } - virtual Result IsChainValid(const DERArray&) + virtual Result IsChainValid(const DERArray&, Time) { ADD_FAILURE(); return Result::FATAL_ERROR_LIBRARY_FAILURE;