Put CT policy check after certificate verification in QUIC ProofVerifier

Change-Id: I87ccef2bb33ee0651a254ac539b8ad21872df6b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437475Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Nick Harper <nharper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813347}

net/quic/crypto/proof_verifier_chromium.cc

@@ -237,13 +237,6 @@ quic::QuicAsyncStatus ProofVerifierChromium::Job::VerifyProof(
   if (!GetX509Certificate(certs, error_details, verify_details))
     return quic::QUIC_FAILURE;
 
-  // Note that this is a completely synchronous operation: The CT Log Verifier
-  // gets all the data it needs for SCT verification and does not do any
-  // external communication.
-  cert_transparency_verifier_->Verify(
-      hostname, cert_.get(), std::string(), cert_sct,
-      &verify_details_->ct_verify_result.scts, net_log_);
-
   // We call VerifySignature first to avoid copying of server_config and
   // signature.
   if (!VerifySignature(server_config, quic_version, chlo_hash, signature,
@@ -286,13 +279,6 @@ quic::QuicAsyncStatus ProofVerifierChromium::Job::VerifyCertChain(
   if (!GetX509Certificate(certs, error_details, verify_details))
     return quic::QUIC_FAILURE;
 
-  // Note that this is a completely synchronous operation: The CT Log Verifier
-  // gets all the data it needs for SCT verification and does not do any
-  // external communication.
-  cert_transparency_verifier_->Verify(
-      hostname, cert_.get(), std::string(), cert_sct,
-      &verify_details_->ct_verify_result.scts, net_log_);
-
   return VerifyCert(hostname, port, ocsp_response, cert_sct, error_details,
                     verify_details, std::move(callback));
 }
@@ -421,6 +407,13 @@ int ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
   // If the connection was good, check HPKP and CT status simultaneously,
   // but prefer to treat the HPKP error as more serious, if there was one.
   if (result == OK) {
+    // Note that this is a completely synchronous operation: The CT Log Verifier
+    // gets all the data it needs for SCT verification and does not do any
+    // external communication.
+    cert_transparency_verifier_->Verify(
+        hostname_, cert_.get(), std::string(), cert_sct_,
+        &verify_details_->ct_verify_result.scts, net_log_);
+
     ct::SCTList verified_scts = ct::SCTsMatchingStatus(
         verify_details_->ct_verify_result.scts, ct::SCT_STATUS_OK);
 

net/quic/crypto/proof_verifier_chromium_test.cc

@@ -199,7 +199,7 @@ class ProofVerifierChromiumTest : public ::testing::Test {
           ct_verify_result.scts,
           ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION));
     } else {
-      EXPECT_EQ(1U, ct_verify_result.scts.size());
+      ASSERT_EQ(1U, ct_verify_result.scts.size());
       EXPECT_EQ(ct::SCT_STATUS_LOG_UNKNOWN, ct_verify_result.scts[0].status);
     }
   }
@@ -275,61 +275,61 @@ TEST_F(ProofVerifierChromiumTest, FailsIfCertFails) {
   ASSERT_EQ(quic::QUIC_FAILURE, status);
 }
 
-// Valid SCT, but invalid signature.
+// Valid SCT and cert
 TEST_F(ProofVerifierChromiumTest, ValidSCTList) {
   // Use different certificates for SCT tests.
   ASSERT_NO_FATAL_FAILURE(GetSCTTestCertificates(&certs_));
 
-  MockCertVerifier cert_verifier;
+  std::string der_test_cert(ct::GetDerEncodedX509Cert());
+  scoped_refptr<X509Certificate> test_cert = X509Certificate::CreateFromBytes(
+      der_test_cert.data(), der_test_cert.length());
+  ASSERT_TRUE(test_cert);
+  CertVerifyResult dummy_result;
+  dummy_result.verified_cert = test_cert;
+  dummy_result.is_issued_by_known_root = true;
+  MockCertVerifier dummy_verifier;
+  dummy_verifier.AddResultForCert(test_cert.get(), dummy_result, OK);
 
   ProofVerifierChromium proof_verifier(
-      &cert_verifier, &ct_policy_enforcer_, &transport_security_state_,
+      &dummy_verifier, &ct_policy_enforcer_, &transport_security_state_,
       ct_verifier_.get(), nullptr, {}, NetworkIsolationKey());
 
   std::unique_ptr<DummyProofVerifierCallback> callback(
       new DummyProofVerifierCallback);
-  quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
-      kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
-      kTestChloHash, certs_, ct::GetSCTListForTesting(), kTestEmptySignature,
-      verify_context_.get(), &error_details_, &details_, std::move(callback));
-  ASSERT_EQ(quic::QUIC_FAILURE, status);
-  CheckSCT(/*sct_expected_ok=*/true);
-
-  callback = std::make_unique<DummyProofVerifierCallback>();
-  status = proof_verifier.VerifyCertChain(
+  quic::QuicAsyncStatus status = proof_verifier.VerifyCertChain(
       kTestHostname, kTestPort, certs_, kTestEmptyOCSPResponse,
       ct::GetSCTListForTesting(), verify_context_.get(), &error_details_,
       &details_, std::move(callback));
-  ASSERT_EQ(quic::QUIC_FAILURE, status);
+  ASSERT_EQ(quic::QUIC_SUCCESS, status);
   CheckSCT(/*sct_expected_ok=*/true);
 }
 
-// Invalid SCT and signature.
+// Invalid SCT, but valid cert
 TEST_F(ProofVerifierChromiumTest, InvalidSCTList) {
   // Use different certificates for SCT tests.
   ASSERT_NO_FATAL_FAILURE(GetSCTTestCertificates(&certs_));
 
-  MockCertVerifier cert_verifier;
+  std::string der_test_cert(ct::GetDerEncodedX509Cert());
+  scoped_refptr<X509Certificate> test_cert = X509Certificate::CreateFromBytes(
+      der_test_cert.data(), der_test_cert.length());
+  ASSERT_TRUE(test_cert);
+  CertVerifyResult dummy_result;
+  dummy_result.verified_cert = test_cert;
+  dummy_result.is_issued_by_known_root = true;
+  MockCertVerifier dummy_verifier;
+  dummy_verifier.AddResultForCert(test_cert.get(), dummy_result, OK);
+
   ProofVerifierChromium proof_verifier(
-      &cert_verifier, &ct_policy_enforcer_, &transport_security_state_,
+      &dummy_verifier, &ct_policy_enforcer_, &transport_security_state_,
       ct_verifier_.get(), nullptr, {}, NetworkIsolationKey());
 
   std::unique_ptr<DummyProofVerifierCallback> callback(
       new DummyProofVerifierCallback);
-  quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
-      kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
-      kTestChloHash, certs_, ct::GetSCTListWithInvalidSCT(),
-      kTestEmptySignature, verify_context_.get(), &error_details_, &details_,
-      std::move(callback));
-  ASSERT_EQ(quic::QUIC_FAILURE, status);
-  CheckSCT(/*sct_expected_ok=*/false);
-
-  callback = std::make_unique<DummyProofVerifierCallback>();
-  status = proof_verifier.VerifyCertChain(
+  quic::QuicAsyncStatus status = proof_verifier.VerifyCertChain(
       kTestHostname, kTestPort, certs_, kTestEmptyOCSPResponse,
       ct::GetSCTListWithInvalidSCT(), verify_context_.get(), &error_details_,
       &details_, std::move(callback));
-  ASSERT_EQ(quic::QUIC_FAILURE, status);
+  ASSERT_EQ(quic::QUIC_SUCCESS, status);
   CheckSCT(/*sct_expected_ok=*/false);
 }