[SCT Auditing] Only audit SCTs for certs issued from known roots

This is an initial solution for crbug.com/1129197 -- the full solution
is implemented in crrev.com/c/2422435 but requires changes to allow
mocking CT results in order to land.

This prevents logging private certificates, and adds a test explicitly
exercising this case. It also makes the browser tests a bit more robust
by tracking the last report seen by the test server and adding a step to
flush a new report through for negative tests.

Bug: 1129197
Change-Id: I7e1d4010b2666db7f98194aa1e3ba80df1e0a493
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2453777Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: David Schinazi <dschinazi@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Auto-Submit: Chris Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815086}

chrome/browser/ssl/sct_reporting_service_browsertest.cc

@@ -6,6 +6,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ssl/cert_verifier_browser_test.h"
 #include "chrome/browser/ssl/sct_reporting_service.h"
 #include "chrome/browser/ssl/sct_reporting_service_factory.h"
 #include "chrome/browser/ui/browser.h"
@@ -19,61 +20,19 @@
 #include "content/public/common/network_service_util.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_mock_cert_verifier.h"
 #include "content/public/test/network_service_test_helper.h"
+#include "net/cert/cert_verify_result.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
-#include "net/test/embedded_test_server/embedded_test_server_connection_listener.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/proto/sct_audit_report.pb.h"
 #include "services/network/test/test_url_loader_factory.h"
 
-namespace {
-
-class ConnectionListener
-    : public net::test_server::EmbeddedTestServerConnectionListener {
- public:
-  ConnectionListener() = default;
-  ~ConnectionListener() override = default;
-
-  ConnectionListener(const ConnectionListener&) = delete;
-  ConnectionListener& operator=(const ConnectionListener&) = delete;
-
-  // EmbeddedTestServerConnectionListener overrides:
-  std::unique_ptr<net::StreamSocket> AcceptedSocket(
-      std::unique_ptr<net::StreamSocket> socket) override {
-    ++connections_seen_;
-    if (!quit_closure_.is_null() &&
-        connections_seen_ >= connections_expected_) {
-      std::move(quit_closure_).Run();
-    }
-    return socket;
-  }
-  void ReadFromSocket(const net::StreamSocket& socket, int rv) override {}
-  void OnResponseCompletedSuccessfully(
-      std::unique_ptr<net::StreamSocket> socket) override {}
-
-  void WaitForConnections(size_t num_connections) {
-    if (connections_seen_ >= num_connections)
-      return;
-
-    connections_expected_ = num_connections;
-
-    base::RunLoop run_loop;
-    quit_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
-  }
-
-  size_t connections_seen() const { return connections_seen_; }
-
- private:
-  size_t connections_seen_ = 0;
-  size_t connections_expected_ = 0;
-  base::OnceClosure quit_closure_;
-};
-
-}  // namespace
-
-class SCTReportingServiceBrowserTest : public InProcessBrowserTest {
+class SCTReportingServiceBrowserTest : public CertVerifierBrowserTest {
  public:
   SCTReportingServiceBrowserTest() {
     // Set sampling rate to 1.0 to ensure deterministic behavior.
@@ -83,11 +42,10 @@ class SCTReportingServiceBrowserTest : public InProcessBrowserTest {
         {});
     SystemNetworkContextManager::SetEnableCertificateTransparencyForTesting(
         true);
-    // We have to initialize the report_server here so we can set the reporting
-    // URL before the network service is initialized.
+    // The report server must be initialized here so the reporting URL can be
+    // set before the network service is initialized.
     ignore_result(report_server()->InitializeAndListen());
-    SCTReportingService::GetReportURLInstance() =
-        report_server()->GetURL("/echo?status=200");
+    SCTReportingService::GetReportURLInstance() = report_server()->GetURL("/");
   }
   ~SCTReportingServiceBrowserTest() override {
     SystemNetworkContextManager::SetEnableCertificateTransparencyForTesting(
@@ -102,16 +60,30 @@ class SCTReportingServiceBrowserTest : public InProcessBrowserTest {
   void SetUpOnMainThread() override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     host_resolver()->AddRule("*", "127.0.0.1");
-    https_server_.AddDefaultHandlers(GetChromeTestDataDir());
-    report_server_.AddDefaultHandlers(GetChromeTestDataDir());
-
-    connection_listener_ = std::make_unique<ConnectionListener>();
-    report_server()->SetConnectionListener(connection_listener_.get());
+    https_server()->AddDefaultHandlers(GetChromeTestDataDir());
+    report_server()->RegisterRequestHandler(base::BindRepeating(
+        &SCTReportingServiceBrowserTest::HandleReportRequest,
+        base::Unretained(this)));
     report_server()->StartAcceptingConnections();
-
-    ASSERT_TRUE(https_server_.Start());
-
-    InProcessBrowserTest::SetUpOnMainThread();
+    ASSERT_TRUE(https_server()->Start());
+
+    // Set up two test hosts as using publicly-issued certificates for testing.
+    net::CertVerifyResult verify_result;
+    verify_result.verified_cert = https_server_.GetCertificate().get();
+    verify_result.is_issued_by_known_root = true;
+    mock_cert_verifier()->AddResultForCertAndHost(
+        https_server()->GetCertificate().get(), "a.test", verify_result,
+        net::OK);
+    mock_cert_verifier()->AddResultForCertAndHost(
+        https_server()->GetCertificate().get(), "b.test", verify_result,
+        net::OK);
+
+    // Set up a third (internal) test host for FlushAndCheckZeroReports().
+    mock_cert_verifier()->AddResultForCertAndHost(
+        https_server()->GetCertificate().get(),
+        "flush-and-check-zero-reports.test", verify_result, net::OK);
+
+    CertVerifierBrowserTest::SetUpOnMainThread();
   }
 
  protected:
@@ -124,22 +96,68 @@ class SCTReportingServiceBrowserTest : public InProcessBrowserTest {
                                                  enabled);
   }
 
-  SCTReportingService* service() const {
-    return SCTReportingServiceFactory::GetForBrowserContext(
-        browser()->profile());
-  }
-
   net::EmbeddedTestServer* https_server() { return &https_server_; }
   net::EmbeddedTestServer* report_server() { return &report_server_; }
-  ConnectionListener* connection_listener() {
-    return connection_listener_.get();
+
+  void WaitForRequests(size_t num_requests) {
+    if (requests_seen_ >= num_requests)
+      return;
+
+    requests_expected_ = num_requests;
+
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+  size_t requests_seen() { return requests_seen_; }
+
+  std::string GetLastSeenHostname() {
+    if (!last_seen_request_.has_content)
+      return std::string();
+    sct_auditing::TLSConnectionReport auditing_report;
+    auditing_report.ParseFromString(last_seen_request_.content);
+    return auditing_report.context().origin().hostname();
+  }
+
+  // Checks that no reports have been sent. To do this, opt-in the profile,
+  // make a new navigation, and check that there is only a single report and it
+  // was for this new navigation specifically. This should be used at the end of
+  // any negative tests to reduce the chance of false successes.
+  bool FlushAndCheckZeroReports() {
+    SetSafeBrowsingEnabled(true);
+    SetExtendedReportingEnabled(true);
+    ui_test_utils::NavigateToURL(
+        browser(),
+        https_server()->GetURL("flush-and-check-zero-reports.test", "/"));
+    WaitForRequests(1);
+    return (1u == requests_seen() &&
+            "flush-and-check-zero-reports.test" == GetLastSeenHostname());
   }
 
  private:
+  std::unique_ptr<net::test_server::HttpResponse> HandleReportRequest(
+      const net::test_server::HttpRequest& request) {
+    last_seen_request_ = request;
+    ++requests_seen_;
+    if (!quit_closure_.is_null() && requests_seen_ >= requests_expected_) {
+      std::move(quit_closure_).Run();
+    }
+
+    auto http_response =
+        std::make_unique<net::test_server::BasicHttpResponse>();
+    http_response->set_code(net::HTTP_OK);
+    return http_response;
+  }
+
   net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS};
   net::EmbeddedTestServer report_server_;
   base::test::ScopedFeatureList scoped_feature_list_;
-  std::unique_ptr<ConnectionListener> connection_listener_;
+
+  net::test_server::HttpRequest last_seen_request_;
+  size_t requests_seen_ = 0;
+  size_t requests_expected_ = 0;
+  base::OnceClosure quit_closure_;
 };
 
 // Tests that reports should not be sent when extended reporting is not opted
@@ -149,10 +167,12 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   SetExtendedReportingEnabled(false);
 
   // Visit an HTTPS page.
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
 
   // Check that no reports are sent.
-  EXPECT_EQ(0u, connection_listener()->connections_seen());
+  EXPECT_EQ(0u, requests_seen());
+  EXPECT_TRUE(FlushAndCheckZeroReports());
 }
 
 // Tests that reports should be sent when extended reporting is opted in.
@@ -161,21 +181,23 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   SetExtendedReportingEnabled(true);
 
   // Visit an HTTPS page and wait for the report to be sent.
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
-  connection_listener()->WaitForConnections(1);
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
+  WaitForRequests(1);
 
-  // Check that one report was sent.
-  EXPECT_EQ(1u, connection_listener()->connections_seen());
+  // Check that one report was sent and contains the expected details.
+  EXPECT_EQ(1u, requests_seen());
+  EXPECT_EQ("a.test", GetLastSeenHostname());
 }
 
 // Tests that disabling Safe Browsing entirely should cause reports to not get
 // sent.
 IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest, DisableSafebrowsing) {
   SetSafeBrowsingEnabled(false);
-
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
-
-  EXPECT_EQ(0u, connection_listener()->connections_seen());
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
+  EXPECT_EQ(0u, requests_seen());
+  EXPECT_TRUE(FlushAndCheckZeroReports());
 }
 
 // Tests that we don't send a report for a navigation with a cert error.
@@ -187,7 +209,8 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   ui_test_utils::NavigateToURL(browser(),
                                https_server()->GetURL("invalid.test", "/"));
 
-  EXPECT_EQ(0u, connection_listener()->connections_seen());
+  EXPECT_EQ(0u, requests_seen());
+  EXPECT_TRUE(FlushAndCheckZeroReports());
 }
 
 // Tests that reports aren't sent for Incognito windows.
@@ -201,7 +224,8 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
 
   ui_test_utils::NavigateToURL(incognito, https_server()->GetURL("/"));
 
-  EXPECT_EQ(0u, connection_listener()->connections_seen());
+  EXPECT_EQ(0u, requests_seen());
+  EXPECT_TRUE(FlushAndCheckZeroReports());
 }
 
 // Tests that disabling Extended Reporting causes the cache to be cleared.
@@ -211,11 +235,13 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   SetExtendedReportingEnabled(true);
 
   // Visit an HTTPS page and wait for a report to be sent.
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
-  connection_listener()->WaitForConnections(1);
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
+  WaitForRequests(1);
 
   // Check that one report was sent.
-  EXPECT_EQ(1u, connection_listener()->connections_seen());
+  EXPECT_EQ(1u, requests_seen());
+  EXPECT_EQ("a.test", GetLastSeenHostname());
 
   // Disable Extended Reporting which should clear the underlying cache.
   SetExtendedReportingEnabled(false);
@@ -223,9 +249,11 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   // We can check that the same report gets cached again instead of being
   // deduplicated (i.e., another report should be sent).
   SetExtendedReportingEnabled(true);
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
-  connection_listener()->WaitForConnections(2);
-  EXPECT_EQ(2u, connection_listener()->connections_seen());
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
+  WaitForRequests(2);
+  EXPECT_EQ(2u, requests_seen());
+  EXPECT_EQ("a.test", GetLastSeenHostname());
 }
 
 // Tests that reports are still sent for opted-in profiles after the network
@@ -245,11 +273,33 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
   SimulateNetworkServiceCrash();
 
   // Visit an HTTPS page and wait for the report to be sent.
-  ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
-  connection_listener()->WaitForConnections(1);
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("a.test", "/"));
+  WaitForRequests(1);
 
   // Check that one report was enqueued.
-  EXPECT_EQ(1u, connection_listener()->connections_seen());
+  EXPECT_EQ(1u, requests_seen());
+  EXPECT_EQ("a.test", GetLastSeenHostname());
+}
+
+// Tests that certificates that aren't issued from publicly known roots don't
+// get reported.
+IN_PROC_BROWSER_TEST_F(SCTReportingServiceBrowserTest,
+                       PrivateCertsNotReported) {
+  // Set up a hostname that uses a non-publicly issued cert.
+  net::CertVerifyResult verify_result;
+  verify_result.verified_cert = https_server()->GetCertificate().get();
+  verify_result.is_issued_by_known_root = false;
+  mock_cert_verifier()->AddResultForCertAndHost(
+      https_server()->GetCertificate().get(), "private.test", verify_result,
+      net::OK);
+
+  SetExtendedReportingEnabled(true);
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server()->GetURL("private.test", "/"));
+
+  EXPECT_EQ(0u, requests_seen());
+  EXPECT_TRUE(FlushAndCheckZeroReports());
 }
 
 // TODO(crbug.com/1107975): Add test for "invalid SCTs should not get reported".
@@ -290,5 +340,5 @@ IN_PROC_BROWSER_TEST_F(SCTReportingServiceZeroSamplingRateBrowserTest,
   ui_test_utils::NavigateToURL(browser(), https_server()->GetURL("/"));
 
   // Check that no reports are observed.
-  EXPECT_EQ(0u, connection_listener()->connections_seen());
+  EXPECT_EQ(0u, requests_seen());
 }

content/browser/web_package/signed_exchange_handler_unittest.cc

@@ -1078,6 +1078,44 @@ TEST_P(SignedExchangeHandlerTest, SCTAuditingReportEnqueued) {
   EXPECT_EQ(static_cast<int>(expected_payload.size()), rv);
 }
 
+// Test that SignedExchangeHandler does not enqueue SCT auditing reports if the
+// certificate is not issued by a known root. Mirrors the
+// `SCTAuditingReportEnqueued` test above, except that `is_issued_by_known_root`
+// is set to false.
+TEST_P(SignedExchangeHandlerTest, SCTAuditingReportNonPublicCertsNotReported) {
+  mock_cert_fetcher_factory_->ExpectFetch(
+      GURL("https://cert.example.org/cert.msg"),
+      GetTestFileContents("test.example.org.public.pem.cbor"));
+
+  net::CertVerifyResult cert_result = CreateCertVerifyResult();
+  cert_result.is_issued_by_known_root = false;
+  SetupMockCertVerifier("prime256v1-sha256.public.pem", cert_result);
+
+  // The mock CT policy enforcer will return CT_POLICY_COMPLIES_VIA_SCTS, as
+  // configured in SetUp().
+
+  // Add SCTAuditingDelegate mock results.
+  EXPECT_CALL(*mock_sct_auditing_delegate_, IsSCTAuditingEnabled())
+      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*mock_sct_auditing_delegate_, MaybeEnqueueReport(_, _, _))
+      .Times(0);
+
+  SetSourceStreamContents("test.example.org_test.sxg");
+
+  CreateSignedExchangeHandler(CreateTestURLRequestContext());
+  WaitForHeader();
+
+  ASSERT_TRUE(read_header());
+  EXPECT_EQ(SignedExchangeLoadResult::kSuccess, result());
+  EXPECT_EQ(net::OK, error());
+
+  std::string payload;
+  int rv = ReadPayloadStream(&payload);
+  std::string expected_payload = GetTestFileContents("test.html");
+  EXPECT_EQ(expected_payload, payload);
+  EXPECT_EQ(static_cast<int>(expected_payload.size()), rv);
+}
+
 INSTANTIATE_TEST_SUITE_P(SignedExchangeHandlerTests,
                          SignedExchangeHandlerTest,
                          ::testing::Values(net::MockSourceStream::SYNC,

net/quic/crypto/proof_verifier_chromium.cc

@@ -477,7 +477,8 @@ int ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
     }
 
     if (sct_auditing_delegate_ &&
-        sct_auditing_delegate_->IsSCTAuditingEnabled()) {
+        sct_auditing_delegate_->IsSCTAuditingEnabled() &&
+        cert_verify_result.is_issued_by_known_root) {
       sct_auditing_delegate_->MaybeEnqueueReport(
           HostPortPair(hostname_, port_),
           cert_verify_result.verified_cert.get(), cert_verify_result.scts);

net/quic/crypto/proof_verifier_chromium_test.cc

@@ -1206,5 +1206,41 @@ TEST_F(ProofVerifierChromiumTest, SCTAuditingReportCollected) {
   ASSERT_EQ(quic::QUIC_SUCCESS, status);
 }
 
+// Tests that the SCTAuditingDelegate is not called when a cert isn't issued
+// from a known root. Mirrors `SCTAuditingReportCollected` test above, but with
+// `is_issued_by_known_root` set to false. Note that QUIC fails for certs that
+// aren't issued from known roots.
+TEST_F(ProofVerifierChromiumTest, SCTAuditingNonPublicCertsNotReported) {
+  MockCertVerifier cert_verifier;
+  dummy_result_.is_issued_by_known_root = false;
+  cert_verifier.AddResultForCert(test_cert_.get(), dummy_result_, OK);
+  EXPECT_CALL(ct_policy_enforcer_, CheckCompliance(_, _, _))
+      .WillRepeatedly(
+          Return(ct::CTPolicyCompliance::CT_POLICY_COMPLIES_VIA_SCTS));
+  MockSCTAuditingDelegate sct_auditing_delegate;
+  EXPECT_CALL(sct_auditing_delegate, IsSCTAuditingEnabled())
+      .WillRepeatedly(Return(true));
+  HostPortPair host_port_pair(kTestHostname, kTestPort);
+  EXPECT_CALL(sct_auditing_delegate, MaybeEnqueueReport(host_port_pair, _, _))
+      .Times(0);
+
+  ProofVerifierChromium proof_verifier(
+      &cert_verifier, &ct_policy_enforcer_, &transport_security_state_,
+      ct_verifier_.get(), &sct_auditing_delegate, {}, NetworkIsolationKey());
+
+  auto callback = std::make_unique<DummyProofVerifierCallback>();
+  quic::QuicAsyncStatus status = proof_verifier.VerifyProof(
+      kTestHostname, kTestPort, kTestConfig, kTestTransportVersion,
+      kTestChloHash, certs_, kTestEmptySCT, GetTestSignature(),
+      verify_context_.get(), &error_details_, &details_, std::move(callback));
+  ASSERT_EQ(quic::QUIC_FAILURE, status);
+
+  callback = std::make_unique<DummyProofVerifierCallback>();
+  status = proof_verifier.VerifyCertChain(
+      kTestHostname, kTestPort, certs_, kTestEmptyOCSPResponse, kTestEmptySCT,
+      verify_context_.get(), &error_details_, &details_, std::move(callback));
+  ASSERT_EQ(quic::QUIC_FAILURE, status);
+}
+
 }  // namespace test
 }  // namespace net

net/socket/ssl_client_socket_impl.cc

@@ -1607,7 +1607,8 @@ int SSLClientSocketImpl::VerifyCT() {
   }
 
   if (context_->sct_auditing_delegate() &&
-      context_->sct_auditing_delegate()->IsSCTAuditingEnabled()) {
+      context_->sct_auditing_delegate()->IsSCTAuditingEnabled() &&
+      server_cert_verify_result_.is_issued_by_known_root) {
     context_->sct_auditing_delegate()->MaybeEnqueueReport(
         host_and_port_, server_cert_verify_result_.verified_cert.get(),
         server_cert_verify_result_.scts);

services/network/network_context.cc

@@ -2403,7 +2403,8 @@ void NetworkContext::OnCertVerifyForSignedExchangeComplete(int cert_verify_id,
             net::NetworkIsolationKey::Todo());
 
     if (url_request_context_->sct_auditing_delegate() &&
-        url_request_context_->sct_auditing_delegate()->IsSCTAuditingEnabled()) {
+        url_request_context_->sct_auditing_delegate()->IsSCTAuditingEnabled() &&
+        pending_cert_verify->result->is_issued_by_known_root) {
       url_request_context_->sct_auditing_delegate()->MaybeEnqueueReport(
           net::HostPortPair::FromURL(pending_cert_verify->url), verified_cert,
           pending_cert_verify->result->scts);