Take delegate parameter in CheckCRL and CheckOCSP Bug: 504234693 Change-Id: If789c68422d340b8fbc8dd255c80c467eaf8592a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/103227 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Matt Mueller <mattm@google.com>
diff --git a/pki/crl.cc b/pki/crl.cc index 3144531..e143689 100644 --- a/pki/crl.cc +++ b/pki/crl.cc
@@ -28,6 +28,7 @@ #include "signature_algorithm.h" #include "verify_name_match.h" #include "verify_signed_data.h" +#include "verify_certificate_chain.h" BSSL_NAMESPACE_BEGIN @@ -425,7 +426,8 @@ size_t target_cert_index, const ParsedDistributionPoint &cert_dp, int64_t verify_time_epoch_seconds, - std::optional<int64_t> max_age_seconds) { + std::optional<int64_t> max_age_seconds, + VerifyCertificateChainDelegate *delegate) { BSSL_CHECK(target_cert_index < valid_chain.size()); if (cert_dp.reasons) { @@ -461,14 +463,17 @@ } // 5.1.1.2 signatureAlgorithm - // - // TODO(https://crbug.com/749276): Check the signature algorithm against - // policy. std::optional<SignatureAlgorithm> signature_algorithm = ParseSignatureAlgorithm(signature_algorithm_tlv); if (!signature_algorithm) { return CRLRevocationStatus::UNKNOWN; } + // Check the signature algorithm against policy. + CertErrors unused_errors; + if (delegate && !delegate->IsSignatureAlgorithmAcceptable( + *signature_algorithm, &unused_errors)) { + return CRLRevocationStatus::UNKNOWN; + } // This field MUST contain the same algorithm identifier as the // signature field in the sequence tbsCertList (Section 5.1.2.2). @@ -662,7 +667,7 @@ // key validated in step (f). if (!VerifySignedData(*signature_algorithm, tbs_cert_list_tlv, signature_value, issuer_cert->tbs().spki_tlv, - /*cache=*/nullptr)) { + delegate ? delegate->GetVerifyCache() : nullptr)) { continue; } @@ -687,4 +692,14 @@ return CRLRevocationStatus::UNKNOWN; } +CRLRevocationStatus CheckCRL(std::string_view raw_crl, + const ParsedCertificateList &valid_chain, + size_t target_cert_index, + const ParsedDistributionPoint &cert_dp, + int64_t verify_time_epoch_seconds, + std::optional<int64_t> max_age_seconds) { + return CheckCRL(raw_crl, valid_chain, target_cert_index, cert_dp, + verify_time_epoch_seconds, max_age_seconds, nullptr); +} + BSSL_NAMESPACE_END
diff --git a/pki/crl.h b/pki/crl.h index 4d59720..01639cc 100644 --- a/pki/crl.h +++ b/pki/crl.h
@@ -28,6 +28,7 @@ struct ParsedCrlTbsCertList; struct ParsedDistributionPoint; +class VerifyCertificateChainDelegate; // TODO(https://crbug.com/749276): This is the same enum with the same meaning // as OCSPRevocationStatus, maybe they should be merged? @@ -220,6 +221,16 @@ // implemented as time since the `thisUpdate` field in the CRL // TBSCertList. Responses older than `max_age_seconds` will be // considered invalid. +// * `delegate`: If non-null, used to check the signature algorithm. +// +// TODO(mattm): Make the `delegate` non-optional. The other wrapper could +// configure a default one itself instead of passing in null. +[[nodiscard]] OPENSSL_EXPORT CRLRevocationStatus CheckCRL( + std::string_view raw_crl, const ParsedCertificateList &valid_chain, + size_t target_cert_index, const ParsedDistributionPoint &cert_dp, + int64_t verify_time_epoch_seconds, std::optional<int64_t> max_age_seconds, + VerifyCertificateChainDelegate* delegate); + [[nodiscard]] OPENSSL_EXPORT CRLRevocationStatus CheckCRL( std::string_view raw_crl, const ParsedCertificateList &valid_chain, size_t target_cert_index, const ParsedDistributionPoint &cert_dp,
diff --git a/pki/crl_unittest.cc b/pki/crl_unittest.cc index 0ccca3d..0e6b3c0 100644 --- a/pki/crl_unittest.cc +++ b/pki/crl_unittest.cc
@@ -20,6 +20,7 @@ #include <openssl/pool.h> #include "cert_errors.h" #include "parsed_certificate.h" +#include "simple_path_builder_delegate.h" #include "string_util.h" #include "test_helpers.h" @@ -43,6 +44,37 @@ {}, &errors); } +// A delegate that optionally allows SHA-256 based signatures. No other +// algorithms are allowed. +// +// Derived from SimplePathBuilderDelegate just so that the test doesn't need to +// add no-op implementations of all the other methods on the +// VerifyCertificateChainDelegate interface. The actual +// IsSignatureAlgorithmAcceptable method is overridden so the parameters given +// to SimplePathBuilderDelegate don't actually matter. +class Sha256CheckerDelegate : public SimplePathBuilderDelegate { + public: + Sha256CheckerDelegate(bool allow_sha256) + : SimplePathBuilderDelegate( + 1024, SimplePathBuilderDelegate::DigestPolicy::kStrong), + allow_sha256_(allow_sha256) {} + + bool IsSignatureAlgorithmAcceptable(SignatureAlgorithm signature_algorithm, + CertErrors *errors) override { + switch (signature_algorithm) { + case SignatureAlgorithm::kRsaPkcs1Sha256: + case SignatureAlgorithm::kEcdsaSha256: + case SignatureAlgorithm::kRsaPssSha256: + return allow_sha256_; + default: + return false; + } + } + + private: + bool allow_sha256_; +}; + class CheckCRLTest : public ::testing::TestWithParam<const char *> {}; // Test prefix naming scheme: @@ -211,6 +243,22 @@ revocation_status = CheckCRL(crl_data, certs, /*target_cert_index=*/1, *cert_dp, kVerifyTime, kAgeOneWeek); EXPECT_EQ(expected_revocation_status, revocation_status); + + // Check that the delegate IsSignatureAlgorithmAcceptable is used. All the + // test CRLs use SHA-256, so test with a delegate which either allows or + // denies SHA-256. If SHA-256 is allowed, this should have the same result as + // the tests that don't pass the delegate. + Sha256CheckerDelegate allow_sha256_delegate(true); + revocation_status = + CheckCRL(crl_data, certs, /*target_cert_index=*/1, *cert_dp, kVerifyTime, + kAgeOneWeek, &allow_sha256_delegate); + EXPECT_EQ(expected_revocation_status, revocation_status); + + Sha256CheckerDelegate disallow_sha256_delegate(false); + revocation_status = + CheckCRL(crl_data, certs, /*target_cert_index=*/1, *cert_dp, kVerifyTime, + kAgeOneWeek, &disallow_sha256_delegate); + EXPECT_EQ(CRLRevocationStatus::UNKNOWN, revocation_status); } } // namespace
diff --git a/pki/ocsp.cc b/pki/ocsp.cc index f576c6e..8d8efe1 100644 --- a/pki/ocsp.cc +++ b/pki/ocsp.cc
@@ -661,15 +661,18 @@ [[nodiscard]] bool VerifyAuthorizedResponderCert( const std::shared_ptr<const ParsedCertificate> &responder_certificate, const std::shared_ptr<const ParsedCertificate> &issuer_certificate, - int64_t verify_time_epoch_seconds) { - // Use a default delegate that matches the previous behavior of allowing - // SHA-1. (This doesn't actually need a PathBuilderDelegate, but there isn't - // a SimpleVerifyCertificateChainDelegate.) - // TODO(mattm): Allow caller to pass in a delegate or otherwise configure - // this. - SimplePathBuilderDelegate verify_delegate( + int64_t verify_time_epoch_seconds, + VerifyCertificateChainDelegate *delegate) { + // If a delegate is not supplied, use a default delegate that matches the + // previous behavior of allowing SHA-1. (This doesn't actually need a + // PathBuilderDelegate, but there isn't a + // SimpleVerifyCertificateChainDelegate.) + SimplePathBuilderDelegate default_verify_delegate( /*min_rsa_modulus_length_bits=*/1024, SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1); + if (!delegate) { + delegate = &default_verify_delegate; + } CertPathErrors errors; TrustAnchor issuer_trust(CertificateTrust::ForTrustAnchor()); der::GeneralizedTime verify_time; @@ -681,10 +684,10 @@ // certificate being checked. Use the RFC 5280 chain verifier to ensure that // all relevant verification checks are done. VerifyCertificateChain( - {responder_certificate, issuer_certificate}, issuer_trust, - &verify_delegate, verify_time, KeyPurpose::ANY_EKU, - InitialExplicitPolicy::kFalse, /*user_initial_policy_set=*/{}, - InitialPolicyMappingInhibit::kFalse, InitialAnyPolicyInhibit::kFalse, + {responder_certificate, issuer_certificate}, issuer_trust, delegate, + verify_time, KeyPurpose::ANY_EKU, InitialExplicitPolicy::kFalse, + /*user_initial_policy_set=*/{}, InitialPolicyMappingInhibit::kFalse, + InitialAnyPolicyInhibit::kFalse, /*user_constrained_policy_set=*/nullptr, &errors); if (errors.ContainsHighSeverityErrors()) { return false; @@ -709,11 +712,18 @@ } [[nodiscard]] bool VerifyOCSPResponseSignatureGivenCert( - const OCSPResponse &response, const ParsedCertificate *cert) { - // TODO(eroman): Must check the signature algorithm against policy. + const OCSPResponse &response, const ParsedCertificate *cert, + VerifyCertificateChainDelegate *delegate) { + // Check the signature algorithm against policy. + CertErrors unused_errors; + if (delegate && !delegate->IsSignatureAlgorithmAcceptable( + response.signature_algorithm, &unused_errors)) { + return false; + } + return VerifySignedData(response.signature_algorithm, response.data, response.signature, cert->tbs().spki_tlv, - /*cache=*/nullptr); + delegate ? delegate->GetVerifyCache() : nullptr); } // Verifies that the OCSP response has a valid signature using @@ -722,14 +732,16 @@ [[nodiscard]] bool VerifyOCSPResponseSignature( const OCSPResponse &response, const OCSPResponseData &response_data, const std::shared_ptr<const ParsedCertificate> &issuer_certificate, - int64_t verify_time_epoch_seconds) { + int64_t verify_time_epoch_seconds, + VerifyCertificateChainDelegate *delegate) { // In order to verify the OCSP signature, a valid responder matching the OCSP // Responder ID must be located (RFC 6960, 4.2.2.2). The responder is allowed // to be either the certificate issuer or a delegated authority directly // signed by the issuer. if (CheckResponderIDMatchesCertificate(response_data.responder_id, issuer_certificate.get()) && - VerifyOCSPResponseSignatureGivenCert(response, issuer_certificate.get())) { + VerifyOCSPResponseSignatureGivenCert(response, issuer_certificate.get(), + delegate)) { return true; } @@ -757,14 +769,14 @@ // looking. if (!VerifyAuthorizedResponderCert(cur_responder_certificate, issuer_certificate, - verify_time_epoch_seconds)) { + verify_time_epoch_seconds, delegate)) { continue; } // If the certificate signed this OCSP response, have found a match. // Otherwise keep looking. - if (VerifyOCSPResponseSignatureGivenCert(response, - cur_responder_certificate.get())) { + if (VerifyOCSPResponseSignatureGivenCert( + response, cur_responder_certificate.get(), delegate)) { return true; } } @@ -904,6 +916,7 @@ std::string_view raw_response, const ParsedCertificate *certificate, const std::shared_ptr<const ParsedCertificate> &issuer_certificate, int64_t verify_time_epoch_seconds, std::optional<int64_t> max_age_seconds, + VerifyCertificateChainDelegate *delegate, OCSPVerifyResult::ResponseStatus *response_details) { *response_details = OCSPVerifyResult::NOT_CHECKED; @@ -979,7 +992,7 @@ // signed directly by the issuing certificate, or a valid authorized // responder. if (!VerifyOCSPResponseSignature(response, response_data, issuer_certificate, - verify_time_epoch_seconds)) { + verify_time_epoch_seconds, delegate)) { return OCSPRevocationStatus::UNKNOWN; } @@ -1000,7 +1013,7 @@ return CheckOCSP(raw_response, parsed_certificate.get(), parsed_issuer_certificate, verify_time_epoch_seconds, - max_age_seconds, response_details); + max_age_seconds, nullptr, response_details); } OCSPRevocationStatus CheckOCSP( @@ -1008,12 +1021,22 @@ const ParsedCertificate *issuer_certificate, int64_t verify_time_epoch_seconds, std::optional<int64_t> max_age_seconds, OCSPVerifyResult::ResponseStatus *response_details) { - // TODO(mattm): change the function signature to take shared_ptr for - // issuer_certificate so that we don't have to re-parse it. std::shared_ptr<const ParsedCertificate> parsed_issuer_certificate = OCSPParseCertificate(issuer_certificate->der_cert()); return CheckOCSP(raw_response, certificate, parsed_issuer_certificate, verify_time_epoch_seconds, max_age_seconds, + nullptr, response_details); +} + +OCSPRevocationStatus CheckOCSP( + std::string_view raw_response, + const std::shared_ptr<const ParsedCertificate> &certificate, + const std::shared_ptr<const ParsedCertificate> &issuer_certificate, + int64_t verify_time_epoch_seconds, std::optional<int64_t> max_age_seconds, + VerifyCertificateChainDelegate *delegate, + OCSPVerifyResult::ResponseStatus *response_details) { + return CheckOCSP(raw_response, certificate.get(), issuer_certificate, + verify_time_epoch_seconds, max_age_seconds, delegate, response_details); }
diff --git a/pki/ocsp.h b/pki/ocsp.h index cf09357..4df1525 100644 --- a/pki/ocsp.h +++ b/pki/ocsp.h
@@ -31,6 +31,7 @@ BSSL_NAMESPACE_BEGIN class ParsedCertificate; +class VerifyCertificateChainDelegate; // OCSPCertID contains a representation of a DER-encoded RFC 6960 "CertID". // @@ -273,8 +274,22 @@ // Checks the revocation status of `certificate` by using the DER-encoded // `raw_response`. // -// Arguments are the same as above, except that it takes already parsed -// instances of the certificate and issuer certificate. +// Arguments are the same as the CheckOCSP from the public ocsp.h, except that +// it takes already parsed instances of the certificate and issuer certificate +// and takes a optional `delegate` for signature and certificate verification +// policy. +// +// TODO(mattm): Make the `delegate` non-optional. The other wrapper could +// configure a default one itself instead of passing in null. +[[nodiscard]] OPENSSL_EXPORT OCSPRevocationStatus CheckOCSP( + std::string_view raw_response, + const std::shared_ptr<const ParsedCertificate> &certificate, + const std::shared_ptr<const ParsedCertificate> &issuer_certificate, + int64_t verify_time_epoch_seconds, std::optional<int64_t> max_age_seconds, + VerifyCertificateChainDelegate *delegate, + OCSPVerifyResult::ResponseStatus *response_details); + +// TODO(mattm): remove this after Chrome is updated to use the new signature. [[nodiscard]] OPENSSL_EXPORT OCSPRevocationStatus CheckOCSP( std::string_view raw_response, const ParsedCertificate *certificate, const ParsedCertificate *issuer_certificate,
diff --git a/pki/ocsp_unittest.cc b/pki/ocsp_unittest.cc index 9ccf9a7..4989323 100644 --- a/pki/ocsp_unittest.cc +++ b/pki/ocsp_unittest.cc
@@ -22,6 +22,7 @@ #include "encode_values.h" #include "parsed_certificate.h" +#include "signature_algorithm.h" #include "string_util.h" #include "test_helpers.h" @@ -145,7 +146,7 @@ OCSPVerifyResult::NO_MATCHING_RESPONSE}, }; -// Parameterised test name generator for tests depending on RenderTextBackend. +// Parameterised test name generator for tests depending on TestParams. struct PrintTestName { std::string operator()(const testing::TestParamInfo<TestParams> &info) const { std::string_view name(info.param.file_name); @@ -200,6 +201,136 @@ der::Input(StringAsBytes(request_data))); } +struct TestDelegateParams { + const char *file_name; + std::set<SignatureAlgorithm> allowed_sig_algs; + OCSPRevocationStatus expected_revocation_status; + OCSPVerifyResult::ResponseStatus expected_response_status; +}; + +class CheckOCSPDelegateTest + : public ::testing::TestWithParam<TestDelegateParams> {}; + +const TestDelegateParams kTestDelegateParams[] = { + // Tests that the delegate is used for the policy on the OCSP response + // signature algorithm. + {"good_response.pem", + {SignatureAlgorithm::kRsaPkcs1Sha1}, + OCSPRevocationStatus::GOOD, + OCSPVerifyResult::PROVIDED}, + {"good_response.pem", + {SignatureAlgorithm::kRsaPkcs1Sha256}, + OCSPRevocationStatus::UNKNOWN, + OCSPVerifyResult::PROVIDED}, + {"good_response_sha256.pem", + {SignatureAlgorithm::kRsaPkcs1Sha1}, + OCSPRevocationStatus::UNKNOWN, + OCSPVerifyResult::PROVIDED}, + {"good_response_sha256.pem", + {SignatureAlgorithm::kRsaPkcs1Sha256}, + OCSPRevocationStatus::GOOD, + OCSPVerifyResult::PROVIDED}, + + // Tests that the delegate is used for the policy on the authorized + // responder verification. + // + // The ocsp_sign_indirect.pem uses SHA-1 for the OCSP response signature, + // and SHA-256 for the authorized responder certificate's signature. If + // both algorithms are allowed, it should verify successfully. If SHA-256 + // is not allowed, the authorized responder certificate should be rejected + // during the certificate verification. + {"ocsp_sign_indirect.pem", + {SignatureAlgorithm::kRsaPkcs1Sha1, SignatureAlgorithm::kRsaPkcs1Sha256}, + OCSPRevocationStatus::GOOD, + OCSPVerifyResult::PROVIDED}, + {"ocsp_sign_indirect.pem", + {SignatureAlgorithm::kRsaPkcs1Sha1}, + OCSPRevocationStatus::UNKNOWN, + OCSPVerifyResult::PROVIDED}, +}; + +// Parameterised test name generator for tests depending on TestDelegateParams. +struct PrintTestDelegateName { + std::string operator()( + const testing::TestParamInfo<TestDelegateParams> &info) const { + std::string_view file_name(info.param.file_name); + // Strip ".pem" from the end as GTest names cannot contain period. + file_name.remove_suffix(4); + std::string name = std::string(file_name); + for (SignatureAlgorithm sig_alg : info.param.allowed_sig_algs) { + name += "SigAlg"; + name += std::to_string(static_cast<int>(sig_alg)); + } + return name; + } +}; + +INSTANTIATE_TEST_SUITE_P(All, CheckOCSPDelegateTest, + ::testing::ValuesIn(kTestDelegateParams), + PrintTestDelegateName()); + +// A delegate that only allows a specified set of signature algorithms. +// +// Derived from SimplePathBuilderDelegate just so that the test doesn't need to +// add no-op implementations of all the other methods on the +// VerifyCertificateChainDelegate interface. The actual +// IsSignatureAlgorithmAcceptable method is overridden so the DigestPolicy given +// to SimplePathBuilderDelegate doesn't actually matter. +class SigAlgCheckerDelegate : public SimplePathBuilderDelegate { + public: + SigAlgCheckerDelegate(std::set<SignatureAlgorithm> allowed_sig_algs) + : SimplePathBuilderDelegate( + 1024, SimplePathBuilderDelegate::DigestPolicy::kStrong), + allowed_sig_algs_(std::move(allowed_sig_algs)) {} + + bool IsSignatureAlgorithmAcceptable(SignatureAlgorithm signature_algorithm, + CertErrors *errors) override { + return allowed_sig_algs_.find(signature_algorithm) != + allowed_sig_algs_.end(); + } + + private: + std::set<SignatureAlgorithm> allowed_sig_algs_; +}; + + +TEST_P(CheckOCSPDelegateTest, FromFile) { + const TestDelegateParams ¶ms = GetParam(); + + SigAlgCheckerDelegate delegate(params.allowed_sig_algs); + + std::string ocsp_data; + std::string ca_data; + std::string cert_data; + std::string request_data; + const PemBlockMapping mappings[] = { + {"OCSP RESPONSE", &ocsp_data}, + {"CA CERTIFICATE", &ca_data}, + {"CERTIFICATE", &cert_data}, + {"OCSP REQUEST", &request_data}, + }; + + ASSERT_TRUE(ReadTestDataFromPemFile(GetFilePath(params.file_name), mappings)); + + // Mar 5 00:00:00 2017 GMT + int64_t kVerifyTime = 1488672000; + + std::shared_ptr<const ParsedCertificate> cert = ParseCertificate(cert_data); + ASSERT_TRUE(cert); + std::shared_ptr<const ParsedCertificate> issuer = ParseCertificate(ca_data); + ASSERT_TRUE(issuer); + + + // Test that CheckOCSP() works. + OCSPVerifyResult::ResponseStatus response_status; + OCSPRevocationStatus revocation_status = + CheckOCSP(ocsp_data, cert, issuer, kVerifyTime, kOCSPAgeOneWeek, + &delegate, &response_status); + + EXPECT_EQ(params.expected_revocation_status, revocation_status); + EXPECT_EQ(params.expected_response_status, response_status); +} + std::string_view kGetURLTestParams[] = { "http://www.example.com/", "http://www.example.com/path/",