ProofVerifierChromium rejects certs that don't chain to a known root

Bug: 980654
Change-Id: I7030873c38173296fa998def363d7498607afec1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1725233
Commit-Queue: Nick Harper <nharper@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686308}

chrome/browser/policy/policy_network_browsertest.cc

@@ -34,8 +34,9 @@
 #include "content/public/common/network_service_util.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
-#include "net/cert/test_root_certs.h"
+#include "content/public/test/content_mock_cert_verifier.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/cert_test_util.h"
 #include "net/test/quic_simple_test_server.h"
 #include "net/test/test_data_directory.h"
 #include "services/network/public/cpp/features.h"
@@ -90,15 +91,37 @@ class QuicTestBase : public InProcessBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitchASCII(switches::kOriginToForceQuicOn, "*");
+    mock_cert_verifier_.SetUpCommandLine(command_line);
   }
 
   void SetUpOnMainThread() override {
-    net::TestRootCerts* root_certs = net::TestRootCerts::GetInstance();
-    root_certs->AddFromFile(
-        net::GetTestCertsDirectory().AppendASCII("quic-root.pem"));
+    ConfigureMockCertVerifier();
     net::QuicSimpleTestServer::Start();
     host_resolver()->AddRule("*", "127.0.0.1");
   }
+
+ protected:
+  void ConfigureMockCertVerifier() {
+    auto test_cert =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "quic-chain.pem");
+    net::CertVerifyResult verify_result;
+    verify_result.verified_cert = test_cert;
+    verify_result.is_issued_by_known_root = true;
+    mock_cert_verifier_.mock_cert_verifier()->AddResultForCert(
+        test_cert, verify_result, net::OK);
+    mock_cert_verifier_.mock_cert_verifier()->set_default_result(net::OK);
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    mock_cert_verifier_.SetUpInProcessBrowserTestFixture();
+  }
+
+  void TearDownInProcessBrowserTestFixture() override {
+    mock_cert_verifier_.TearDownInProcessBrowserTestFixture();
+  }
+
+ private:
+  content::ContentMockCertVerifier mock_cert_verifier_;
 };
 
 // The tests are based on the assumption that command line flag kEnableQuic
@@ -110,6 +133,7 @@ class QuicAllowedPolicyTestBase : public QuicTestBase {
 
  protected:
   void SetUpInProcessBrowserTestFixture() override {
+    QuicTestBase::SetUpInProcessBrowserTestFixture();
     base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kEnableQuic);
     EXPECT_CALL(provider_, IsInitializationComplete(testing::_))
         .WillRepeatedly(testing::Return(true));
@@ -132,6 +156,7 @@ class QuicAllowedPolicyTestBase : public QuicTestBase {
       net::QuicSimpleTestServer::Shutdown();
     }
     SimulateNetworkServiceCrash();
+    ConfigureMockCertVerifier();
     ASSERT_TRUE(net::QuicSimpleTestServer::Start());
   }
 

components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java

@@ -9,6 +9,8 @@
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import static org.chromium.net.CronetTestRule.SERVER_CERT_PEM;
+import static org.chromium.net.CronetTestRule.SERVER_KEY_PKCS8_PEM;
 import static org.chromium.net.CronetTestRule.getContext;
 import static org.chromium.net.CronetTestRule.getTestStorage;
 
@@ -40,8 +42,6 @@
  */
 @RunWith(AndroidJUnit4.class)
 public class PkpTest {
-    private static final String CERT_USED = "quic-chain.pem";
-    private static final String[] CERTS_USED = {CERT_USED};
     private static final int DISTANT_FUTURE = Integer.MAX_VALUE;
     private static final boolean INCLUDE_SUBDOMAINS = true;
     private static final boolean EXCLUDE_SUBDOMAINS = false;
@@ -56,7 +56,7 @@ public class PkpTest {
     private CronetEngine mCronetEngine;
     private ExperimentalCronetEngine.Builder mBuilder;
     private TestUrlRequestCallback mListener;
-    private String mServerUrl; // https://test.example.com:6121
+    private String mServerUrl; // https://test.example.com:8443
     private String mServerHost; // test.example.com
     private String mDomain; // example.com
 
@@ -65,17 +65,18 @@ public void setUp() throws Exception {
         if (mTestRule.testingJavaImpl()) {
             return;
         }
-        // Start QUIC Test Server
+        // Start HTTP2 Test Server
         System.loadLibrary("cronet_tests");
-        QuicTestServer.startQuicTestServer(getContext());
-        mServerUrl = QuicTestServer.getServerURL();
-        mServerHost = QuicTestServer.getServerHost();
+        assertTrue(Http2TestServer.startHttp2TestServer(
+                getContext(), SERVER_CERT_PEM, SERVER_KEY_PKCS8_PEM));
+        mServerHost = "test.example.com";
+        mServerUrl = "https://" + mServerHost + ":" + Http2TestServer.getServerPort();
         mDomain = mServerHost.substring(mServerHost.indexOf('.') + 1, mServerHost.length());
     }
 
     @After
     public void tearDown() throws Exception {
-        QuicTestServer.shutdownQuicTestServer();
+        Http2TestServer.shutdownHttp2TestServer();
         shutdownCronetEngine();
     }
 
@@ -112,7 +113,7 @@ public void testErrorCodeIfPinDoesNotMatch() throws Exception {
     public void testSuccessIfPinMatches() throws Exception {
         createCronetEngineBuilder(ENABLE_PINNING_BYPASS_FOR_LOCAL_ANCHORS, KNOWN_ROOT);
         // Get PKP hash of the real certificate
-        X509Certificate cert = readCertFromFileInPemFormat(CERT_USED);
+        X509Certificate cert = readCertFromFileInPemFormat(SERVER_CERT_PEM);
         byte[] matchingHash = CertTestUtil.getPublicKeySha256(cert);
 
         addPkpSha256(mServerHost, matchingHash, EXCLUDE_SUBDOMAINS, DISTANT_FUTURE);
@@ -392,17 +393,11 @@ public void testIllegalArgumentExceptionWhenPinValueIsSHA1() throws Exception {
 
     /**
      * Asserts that the response from the server contains an PKP error.
-     * TODO(kapishnikov): currently QUIC returns ERR_QUIC_PROTOCOL_ERROR instead of expected
-     * ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN error code when the pin doesn't match.
-     * This method should be changed when the bug is resolved.
-     * See http://crbug.com/548378
-     * See http://crbug.com/568669
      */
     private void assertErrorResponse() {
         assertNotNull("Expected an error", mListener.mError);
         int errorCode = ((NetworkException) mListener.mError).getCronetInternalErrorCode();
         Set<Integer> expectedErrors = new HashSet<>();
-        expectedErrors.add(NetError.ERR_QUIC_PROTOCOL_ERROR);
         expectedErrors.add(NetError.ERR_CONNECTION_REFUSED);
         expectedErrors.add(NetError.ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN);
         assertTrue(String.format("Incorrect error code. Expected one of %s but received %s",
@@ -427,17 +422,15 @@ private void createCronetEngineBuilder(boolean bypassPinningForLocalAnchors, boo
         // Set common CronetEngine parameters
         mBuilder = new ExperimentalCronetEngine.Builder(getContext());
         mBuilder.enablePublicKeyPinningBypassForLocalTrustAnchors(bypassPinningForLocalAnchors);
-        mBuilder.enableQuic(true);
-        mBuilder.addQuicHint(QuicTestServer.getServerHost(), QuicTestServer.getServerPort(),
-                QuicTestServer.getServerPort());
         JSONObject hostResolverParams = CronetTestUtil.generateHostResolverRules();
         JSONObject experimentalOptions = new JSONObject()
                                                  .put("HostResolverRules", hostResolverParams);
         mBuilder.setExperimentalOptions(experimentalOptions.toString());
         mBuilder.setStoragePath(getTestStorage(getContext()));
         mBuilder.enableHttpCache(CronetEngine.Builder.HTTP_CACHE_DISK_NO_HTTP, 1000 * 1024);
+        final String[] server_certs = {SERVER_CERT_PEM};
         CronetTestUtil.setMockCertVerifierForTesting(
-                mBuilder, MockCertVerifier.createMockCertVerifier(CERTS_USED, knownRoot));
+                mBuilder, MockCertVerifier.createMockCertVerifier(server_certs, knownRoot));
     }
 
     private void startCronetEngine() {
@@ -467,9 +460,9 @@ private void addPkpSha256(
     private void sendRequestAndWaitForResult() {
         mListener = new TestUrlRequestCallback();
 
-        String quicURL = mServerUrl + "/simple.txt";
+        String httpURL = mServerUrl + "/simple.txt";
         UrlRequest.Builder requestBuilder =
-                mCronetEngine.newUrlRequestBuilder(quicURL, mListener, mListener.getExecutor());
+                mCronetEngine.newUrlRequestBuilder(httpURL, mListener, mListener.getExecutor());
         requestBuilder.build().start();
         mListener.blockForDone();
     }

components/cronet/android/test/src/org/chromium/net/Http2TestHandler.java

@@ -101,7 +101,7 @@ private class RequestResponder {
         void onHeadersRead(ChannelHandlerContext ctx, int streamId, boolean endOfStream,
                 Http2Headers headers) {
             encoder().writeHeaders(ctx, streamId, createResponseHeadersFromRequestHeaders(headers),
-                    0, false, ctx.newPromise());
+                    0, endOfStream, ctx.newPromise());
             ctx.flush();
         }
 

components/cronet/ios/Cronet.mm

@@ -83,6 +83,7 @@ int Verify(const RequestParams& params,
     net::Error result = net::OK;
     verify_result->verified_cert = params.certificate();
     verify_result->cert_status = net::MapNetErrorToCertStatus(result);
+    verify_result->is_issued_by_known_root = true;
     return result;
   }
   void SetConfig(const Config& config) override {}

components/cronet/native/test/test_util.cc

@@ -39,6 +39,7 @@ class TestCertVerifier : public net::MockCertVerifier {
     if (params.hostname() == "test.example.com") {
       verify_result->verified_cert = params.certificate();
       verify_result->cert_status = MapNetErrorToCertStatus(net::OK);
+      verify_result->is_issued_by_known_root = true;
       return net::OK;
     }
     return net::MockCertVerifier::Verify(params, verify_result,

components/grpc_support/test/get_stream_engine.cc

@@ -21,7 +21,9 @@
 #include "net/dns/mapped_host_resolver.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_server_properties.h"
+#include "net/test/cert_test_util.h"
 #include "net/test/quic_simple_test_server.h"
+#include "net/test/test_data_directory.h"
 #include "net/url_request/url_request_test_util.h"
 
 namespace grpc_support {
@@ -47,8 +49,13 @@ class BidirectionalStreamTestURLRequestContextGetter
       host_resolver_.reset(
           new net::MappedHostResolver(std::move(mock_host_resolver)));
       UpdateHostResolverRules();
+      auto test_cert = net::ImportCertFromFile(net::GetTestCertsDirectory(),
+                                               "quic-chain.pem");
       mock_cert_verifier_.reset(new net::MockCertVerifier());
-      mock_cert_verifier_->set_default_result(net::OK);
+      net::CertVerifyResult verify_result;
+      verify_result.verified_cert = test_cert;
+      verify_result.is_issued_by_known_root = true;
+      mock_cert_verifier_->AddResultForCert(test_cert, verify_result, net::OK);
 
       auto params = std::make_unique<net::HttpNetworkSession::Params>();
       params->enable_quic = true;

net/base/net_error_list.h

@@ -536,13 +536,18 @@ NET_ERROR(CERTIFICATE_TRANSPARENCY_REQUIRED, -214)
 // https://g.co/chrome/symantecpkicerts
 NET_ERROR(CERT_SYMANTEC_LEGACY, -215)
 
+// The certificate presented on a QUIC connection does not chain to a known root
+// and the origin connected to is not on a list of domains where unknown roots
+// are allowed.
+NET_ERROR(QUIC_CERT_ROOT_NOT_KNOWN, -216)
+
 // Add new certificate error codes here.
 //
 // Update the value of CERT_END whenever you add a new certificate error
 // code.
 
 // The value immediately past the last certificate error code.
-NET_ERROR(CERT_END, -216)
+NET_ERROR(CERT_END, -217)
 
 // The URL is invalid.
 NET_ERROR(INVALID_URL, -300)

net/quic/crypto/proof_verifier_chromium.cc

@@ -504,6 +504,13 @@ int ProofVerifierChromium::Job::DoVerifyCertComplete(int result) {
       result = ct_result;
   }
 
+  if (result == OK &&
+      !verify_details_->cert_verify_result.is_issued_by_known_root &&
+      !base::Contains(proof_verifier_->hostnames_to_allow_unknown_roots_,
+                      hostname_)) {
+    result = ERR_QUIC_CERT_ROOT_NOT_KNOWN;
+  }
+
   verify_details_->is_fatal_cert_error =
       IsCertStatusError(cert_status) && !IsCertStatusMinorError(cert_status) &&
       transport_security_state_->ShouldSSLErrorsBeFatal(hostname_);
@@ -570,11 +577,13 @@ ProofVerifierChromium::ProofVerifierChromium(
     CertVerifier* cert_verifier,
     CTPolicyEnforcer* ct_policy_enforcer,
     TransportSecurityState* transport_security_state,
-    CTVerifier* cert_transparency_verifier)
+    CTVerifier* cert_transparency_verifier,
+    std::set<std::string> hostnames_to_allow_unknown_roots)
     : cert_verifier_(cert_verifier),
       ct_policy_enforcer_(ct_policy_enforcer),
       transport_security_state_(transport_security_state),
-      cert_transparency_verifier_(cert_transparency_verifier) {
+      cert_transparency_verifier_(cert_transparency_verifier),
+      hostnames_to_allow_unknown_roots_(hostnames_to_allow_unknown_roots) {
   DCHECK(cert_verifier_);
   DCHECK(ct_policy_enforcer_);
   DCHECK(transport_security_state_);

net/quic/crypto/proof_verifier_chromium.h

@@ -73,7 +73,8 @@ class NET_EXPORT_PRIVATE ProofVerifierChromium : public quic::ProofVerifier {
   ProofVerifierChromium(CertVerifier* cert_verifier,
                         CTPolicyEnforcer* ct_policy_enforcer,
                         TransportSecurityState* transport_security_state,
-                        CTVerifier* cert_transparency_verifier);
+                        CTVerifier* cert_transparency_verifier,
+                        std::set<std::string> hostnames_to_allow_unknown_roots);
   ~ProofVerifierChromium() override;
 
   // quic::ProofVerifier interface
@@ -116,6 +117,8 @@ class NET_EXPORT_PRIVATE ProofVerifierChromium : public quic::ProofVerifier {
   TransportSecurityState* const transport_security_state_;
   CTVerifier* const cert_transparency_verifier_;
 
+  std::set<std::string> hostnames_to_allow_unknown_roots_;
+
   DISALLOW_COPY_AND_ASSIGN(ProofVerifierChromium);
 };