Commit ee104c44 authored by Chris Thompson's avatar Chris Thompson Committed by Chromium LUCI CQ

Address CertAndCTVerifier crash

This enforces the invariant that CertVerifyResult::verified_cert should
always be non-null for the case when a MojoCertVerifier is disconnected,
and also adds a check to CertAndCTVerifier to only perform CT
verification if the underlying CertVerifier succeeded. This fixes a
crash where the CT verification step would assume there was a cert in
the CertVerifyResult, but MojoCertVerifier had been disconnected and
reset the CertVerifyResult in its disconnect handler.

Bug: 1153484
Change-Id: I6039ea4e28715a5c6d5e9a2878c538ddf4d5aa8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2565537Reviewed-by: default avatarRyan Sleevi <rsleevi@chromium.org>
Commit-Queue: Chris Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832084}
parent e3f530d8
...@@ -35,10 +35,13 @@ int CertAndCTVerifier::Verify(const RequestParams& params, ...@@ -35,10 +35,13 @@ int CertAndCTVerifier::Verify(const RequestParams& params,
int result = cert_verifier_->Verify(params, verify_result, int result = cert_verifier_->Verify(params, verify_result,
std::move(ct_callback), out_req, net_log); std::move(ct_callback), out_req, net_log);
if (result != ERR_IO_PENDING) { // If the certificate verification completed synchronously and successfully,
// Synchronous completion; directly perform CT verification which is always // then directly perform CT verification (which is always synchronous as it
// synchronous as it has all the data it needs for SCT verificiation and // has all the data it needs for SCT verification and does not do any external
// does not do any external communication. // communication).
if (result != ERR_IO_PENDING &&
(result == OK || IsCertificateError(result))) {
DCHECK(verify_result->verified_cert);
ct_verifier_->Verify(params.hostname(), verify_result->verified_cert.get(), ct_verifier_->Verify(params.hostname(), verify_result->verified_cert.get(),
params.ocsp_response(), params.sct_list(), params.ocsp_response(), params.sct_list(),
&verify_result->scts, net_log); &verify_result->scts, net_log);
...@@ -55,13 +58,18 @@ void CertAndCTVerifier::OnCertVerifyComplete(const RequestParams& params, ...@@ -55,13 +58,18 @@ void CertAndCTVerifier::OnCertVerifyComplete(const RequestParams& params,
CompletionOnceCallback callback, CompletionOnceCallback callback,
CertVerifyResult* verify_result, CertVerifyResult* verify_result,
const NetLogWithSource& net_log, const NetLogWithSource& net_log,
int error) { int result) {
ct_verifier_->Verify(params.hostname(), verify_result->verified_cert.get(), // Only perform CT verification if the certificate verification completed
params.ocsp_response(), params.sct_list(), // successfully.
&verify_result->scts, net_log); if (result == OK || IsCertificateError(result)) {
DCHECK(verify_result->verified_cert);
ct_verifier_->Verify(params.hostname(), verify_result->verified_cert.get(),
params.ocsp_response(), params.sct_list(),
&verify_result->scts, net_log);
}
// Now chain to the user's callback, which may delete |this|. // Now chain to the user's callback, which may delete |this|.
std::move(callback).Run(error); std::move(callback).Run(result);
} }
} // namespace net } // namespace net
...@@ -43,7 +43,7 @@ class NET_EXPORT CertAndCTVerifier : public CertVerifier { ...@@ -43,7 +43,7 @@ class NET_EXPORT CertAndCTVerifier : public CertVerifier {
CompletionOnceCallback callback, CompletionOnceCallback callback,
CertVerifyResult* verify_result, CertVerifyResult* verify_result,
const NetLogWithSource& net_log, const NetLogWithSource& net_log,
int error); int result);
std::unique_ptr<CertVerifier> cert_verifier_; std::unique_ptr<CertVerifier> cert_verifier_;
std::unique_ptr<CTVerifier> ct_verifier_; std::unique_ptr<CTVerifier> ct_verifier_;
......
...@@ -299,4 +299,47 @@ TEST_F(CertAndCTVerifierTest, CancelRequestThenQuit) { ...@@ -299,4 +299,47 @@ TEST_F(CertAndCTVerifierTest, CancelRequestThenQuit) {
// Destroy |cert_and_ct_verifier| by going out of scope. // Destroy |cert_and_ct_verifier| by going out of scope.
} }
// Regression test for crbug.com/1153484: If the CertVerifier aborts, the
// CT verification step should be skipped.
TEST_F(CertAndCTVerifierTest, CertVerifierErrorShouldSkipCT) {
base::FilePath certs_dir = GetTestCertsDirectory();
scoped_refptr<X509Certificate> test_cert(
ImportCertFromFile(certs_dir, "ok_cert.pem"));
ASSERT_TRUE(test_cert.get());
// Mock the cert verification result as aborted (like what happens if the
// MojoCertVerifier gets disconnected).
CertVerifyResult mock_result;
mock_result.cert_status = CERT_STATUS_INVALID;
mock_result.verified_cert = test_cert;
auto cert_verifier = std::make_unique<MockCertVerifier>();
cert_verifier->AddResultForCert(test_cert, mock_result, ERR_ABORTED);
cert_verifier->set_async(true);
// Mock valid SCTs.
scoped_refptr<ct::SignedCertificateTimestamp> sct;
ct::GetX509CertSCT(&sct);
SignedCertificateTimestampAndStatus sct_and_status(sct, ct::SCT_STATUS_OK);
SignedCertificateTimestampAndStatusList sct_list{sct_and_status};
auto ct_verifier = std::make_unique<FakeCTVerifier>();
ct_verifier->set_scts(sct_list);
CertAndCTVerifier cert_and_ct_verifier(std::move(cert_verifier),
std::move(ct_verifier));
CertVerifyResult verify_result;
TestCompletionCallback callback;
std::unique_ptr<CertVerifier::Request> request;
int result = callback.GetResult(cert_and_ct_verifier.Verify(
CertVerifier::RequestParams(test_cert, "www.example.com", 0,
/*ocsp_response=*/std::string(),
/*sct_list=*/std::string()),
&verify_result, callback.callback(), &request, NetLogWithSource()));
EXPECT_THAT(result, IsError(ERR_ABORTED));
// SCTs should not be filled in because CTVerifier::Verify() should not be
// called.
ASSERT_EQ(0u, verify_result.scts.size());
}
} // namespace net } // namespace net
...@@ -160,10 +160,10 @@ class NET_EXPORT CertVerifier { ...@@ -160,10 +160,10 @@ class NET_EXPORT CertVerifier {
// Returns OK if successful or an error code upon failure. // Returns OK if successful or an error code upon failure.
// //
// The |*verify_result| structure, including the |verify_result->cert_status| // The |*verify_result| structure, including the |verify_result->cert_status|
// bitmask, is always filled out regardless of the return value. If the // bitmask and |verify_result->verified_cert|, is always filled out regardless
// certificate has multiple errors, the corresponding status flags are set in // of the return value. If the certificate has multiple errors, the
// |verify_result->cert_status|, and the error code for the most serious // corresponding status flags are set in |verify_result->cert_status|, and the
// error is returned. // error code for the most serious error is returned.
// //
// |callback| must not be null. ERR_IO_PENDING is returned if the operation // |callback| must not be null. ERR_IO_PENDING is returned if the operation
// could not be completed synchronously, in which case the result code will // could not be completed synchronously, in which case the result code will
......
...@@ -18,10 +18,12 @@ class CertVerifierRequestImpl : public mojom::CertVerifierRequest, ...@@ -18,10 +18,12 @@ class CertVerifierRequestImpl : public mojom::CertVerifierRequest,
public: public:
CertVerifierRequestImpl( CertVerifierRequestImpl(
mojo::PendingReceiver<mojom::CertVerifierRequest> receiver, mojo::PendingReceiver<mojom::CertVerifierRequest> receiver,
scoped_refptr<net::X509Certificate> cert,
net::CertVerifyResult* verify_result, net::CertVerifyResult* verify_result,
net::CompletionOnceCallback callback, net::CompletionOnceCallback callback,
const net::NetLogWithSource& net_log) const net::NetLogWithSource& net_log)
: receiver_(this, std::move(receiver)), : receiver_(this, std::move(receiver)),
cert_(cert),
cert_verify_result_(verify_result), cert_verify_result_(verify_result),
completion_callback_(std::move(callback)), completion_callback_(std::move(callback)),
net_log_(net_log) { net_log_(net_log) {
...@@ -50,12 +52,15 @@ class CertVerifierRequestImpl : public mojom::CertVerifierRequest, ...@@ -50,12 +52,15 @@ class CertVerifierRequestImpl : public mojom::CertVerifierRequest,
// The CertVerifierRequest disconnected. // The CertVerifierRequest disconnected.
DCHECK(completion_callback_); DCHECK(completion_callback_);
*cert_verify_result_ = net::CertVerifyResult(); *cert_verify_result_ = net::CertVerifyResult();
cert_verify_result_->verified_cert = cert_;
cert_verify_result_->cert_status = net::CERT_STATUS_INVALID; cert_verify_result_->cert_status = net::CERT_STATUS_INVALID;
std::move(completion_callback_).Run(net::ERR_ABORTED); std::move(completion_callback_).Run(net::ERR_ABORTED);
} }
private: private:
mojo::Receiver<mojom::CertVerifierRequest> receiver_; mojo::Receiver<mojom::CertVerifierRequest> receiver_;
// Certificate being verified.
scoped_refptr<net::X509Certificate> cert_;
// Out parameter for the result. // Out parameter for the result.
net::CertVerifyResult* cert_verify_result_; net::CertVerifyResult* cert_verify_result_;
// Callback to call once the result is available. // Callback to call once the result is available.
...@@ -119,8 +124,8 @@ int MojoCertVerifier::Verify( ...@@ -119,8 +124,8 @@ int MojoCertVerifier::Verify(
cert_verifier_request.InitWithNewPipeAndPassReceiver(); cert_verifier_request.InitWithNewPipeAndPassReceiver();
mojo_cert_verifier_->Verify(params, std::move(cert_verifier_request)); mojo_cert_verifier_->Verify(params, std::move(cert_verifier_request));
*out_req = std::make_unique<CertVerifierRequestImpl>( *out_req = std::make_unique<CertVerifierRequestImpl>(
std::move(cert_verifier_receiver), verify_result, std::move(callback), std::move(cert_verifier_receiver), params.certificate(), verify_result,
net_log); std::move(callback), net_log);
return net::ERR_IO_PENDING; return net::ERR_IO_PENDING;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment