Use CertVerifyProcBuiltin on Chrome OS if configured.

Add a mojo parameter to use the built-in cert verifier on Chrome OS.
Pass a SystemTrustStoreProvider to CertVerifyProcBuiltin on ChromeOS
which will provide SystemTrustStores that filter trusted certificates
based on the NSS slot.

Bug: 939344, 978069
Test: browser_tests --gtest_filter=SSLUITestCustomCACerts*
Change-Id: I6b3cb864098f6f802c14092871e295b2576e7cde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1676170
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676156}

chrome/browser/net/profile_network_context_service.cc

@@ -467,9 +467,7 @@ ProfileNetworkContextService::CreateNetworkContextParams(
 
 #if defined(OS_CHROMEOS)
   user_manager::UserManager* user_manager = user_manager::UserManager::Get();
-  if (user_manager &&
-      policy::PolicyCertServiceFactory::CreateAndStartObservingForProfile(
-          profile_)) {
+  if (user_manager) {
     const user_manager::User* user =
         chromeos::ProfileHelper::Get()->GetUserByProfile(profile_);
     // No need to initialize NSS for users with empty username hash:
@@ -479,15 +477,17 @@ ProfileNetworkContextService::CreateNetworkContextParams(
     if (user && !user->username_hash().empty()) {
       network_context_params->username_hash = user->username_hash();
       network_context_params->nss_path = profile_->GetPath();
-
-      policy::PolicyCertService* service =
-          policy::PolicyCertServiceFactory::GetForProfile(profile_);
-      network_context_params->initial_additional_certificates =
-          network::mojom::AdditionalCertificates::New();
-      network_context_params->initial_additional_certificates
-          ->all_certificates = service->all_server_and_authority_certs();
-      network_context_params->initial_additional_certificates->trust_anchors =
-          service->trust_anchors();
+      if (policy::PolicyCertServiceFactory::CreateAndStartObservingForProfile(
+              profile_)) {
+        policy::PolicyCertService* service =
+            policy::PolicyCertServiceFactory::GetForProfile(profile_);
+        network_context_params->initial_additional_certificates =
+            network::mojom::AdditionalCertificates::New();
+        network_context_params->initial_additional_certificates
+            ->all_certificates = service->all_server_and_authority_certs();
+        network_context_params->initial_additional_certificates->trust_anchors =
+            service->trust_anchors();
+      }
     }
   }
 #endif

chrome/browser/ssl/ssl_browsertest.cc

@@ -48,6 +48,7 @@
 #include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ssl/bad_clock_blocking_page.h"
 #include "chrome/browser/ssl/captive_portal_blocking_page.h"
 #include "chrome/browser/ssl/cert_report_helper.h"
@@ -175,11 +176,22 @@
 
 #if defined(USE_NSS_CERTS)
 #include "chrome/browser/certificate_manager_model.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/net/nss_context.h"
+#include "components/user_manager/scoped_user_manager.h"
+#include "components/user_manager/user_manager.h"
+#include "crypto/scoped_test_nss_db.h"
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/x509_util_nss.h"
 #endif  // defined(USE_NSS_CERTS)
 
+#if defined(OS_CHROMEOS)
+#include "base/path_service.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chromeos/constants/chromeos_switches.h"
+#include "components/session_manager/core/session_manager.h"
+#endif  // defined(OS_CHROMEOS)
+
 using namespace ssl_test_util;
 
 using base::ASCIIToUTF16;
@@ -5895,6 +5907,148 @@ IN_PROC_BROWSER_TEST_F(SSLUITestNoCert, NewCertificateAuthority) {
   EXPECT_FALSE(IsShowingInterstitial(tab));
 }
 
+// A test class which prepares two profiles and allows importing certificates
+// into their NSS databases.
+class SSLUITestCustomCACerts : public SSLUITestNoCert {
+ public:
+  SSLUITestCustomCACerts() = default;
+  ~SSLUITestCustomCACerts() override = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    SSLUITestNoCert::SetUpCommandLine(command_line);
+    // Don't require policy for our sessions - this is required so the policy
+    // code knows not to expect cached policy for the secondary profile.
+    command_line->AppendSwitchASCII(chromeos::switches::kProfileRequiresPolicy,
+                                    "false");
+  }
+
+  void SetUpOnMainThread() override {
+    SSLUITestNoCert::SetUpOnMainThread();
+
+    profile_1_ = browser()->profile();
+
+    // Create a second profile.
+    {
+      static const char kSecondProfileAccount[] = "profile2@test.com";
+      static const char kSecondProfileGaiaId[] = "9876543210";
+      static const char kSecondProfileHash[] = "testProfile2";
+
+      base::FilePath user_data_directory;
+      base::PathService::Get(chrome::DIR_USER_DATA, &user_data_directory);
+      session_manager::SessionManager::Get()->CreateSession(
+          AccountId::FromUserEmailGaiaId(kSecondProfileAccount,
+                                         kSecondProfileGaiaId),
+          kSecondProfileHash, false);
+      // Set up the secondary profile.
+      base::FilePath profile_dir = user_data_directory.Append(
+          chromeos::ProfileHelper::GetUserProfileDir(kSecondProfileHash)
+              .BaseName());
+      profile_2_ =
+          g_browser_process->profile_manager()->GetProfile(profile_dir);
+    }
+
+    // Get cert databases for both profiles.
+    {
+      base::RunLoop loop;
+      GetNSSCertDatabaseForProfile(
+          profile_1_,
+          base::Bind(&SSLUITestCustomCACerts::DidGetCertDatabase,
+                     base::Unretained(this), &loop, &profile_1_cert_db_));
+      loop.Run();
+    }
+
+    {
+      base::RunLoop loop;
+      GetNSSCertDatabaseForProfile(
+          profile_2_,
+          base::Bind(&SSLUITestCustomCACerts::DidGetCertDatabase,
+                     base::Unretained(this), &loop, &profile_2_cert_db_));
+      loop.Run();
+    }
+
+    // Double-check that the profile initialization was correct and the two
+    // profiles have distinct NSS databases with distinc NSS public slots.
+    EXPECT_NE(profile_1_cert_db_, profile_2_cert_db_);
+    EXPECT_NE(profile_1_cert_db_->GetPublicSlot().get(),
+              profile_2_cert_db_->GetPublicSlot().get());
+  }
+
+ protected:
+  void ImportCACertAsTrusted(const std::string& cert_file_name,
+                             net::NSSCertDatabase* cert_db) {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+
+    net::ScopedCERTCertificateList ca_cert_list =
+        net::CreateCERTCertificateListFromFile(
+            net::GetTestCertsDirectory(), cert_file_name,
+            net::X509Certificate::FORMAT_AUTO);
+    ASSERT_FALSE(ca_cert_list.empty());
+    net::NSSCertDatabase::ImportCertFailureList failures;
+    ASSERT_TRUE(cert_db->ImportCACerts(
+        ca_cert_list, net::NSSCertDatabase::TRUSTED_SSL, &failures));
+    ASSERT_TRUE(failures.empty());
+  }
+
+  // The first profile.
+  Profile* profile_1_;
+  // The second profile.
+  Profile* profile_2_;
+
+  // The NSSCertDatabase for |profile_1_|.
+  net::NSSCertDatabase* profile_1_cert_db_;
+
+  // The NSSCertDatabase for |profile_2_|.
+  net::NSSCertDatabase* profile_2_cert_db_;
+
+ private:
+  void DidGetCertDatabase(base::RunLoop* loop,
+                          net::NSSCertDatabase** out_cert_db,
+                          net::NSSCertDatabase* cert_db) {
+    *out_cert_db = cert_db;
+    loop->Quit();
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(SSLUITestCustomCACerts);
+};
+
+// Imports a trusted CA certiifcate into a profile's NSS database.
+// Verifies that the certificate is trusted in the context of the profile it was
+// imported for.
+// Verifies that the certificate is *not* trusted in the context of a different
+// profile.
+IN_PROC_BROWSER_TEST_F(SSLUITestCustomCACerts,
+                       TrustedCertOnlyRespectedInProfileThatOwnsIt) {
+  ASSERT_TRUE(https_server_.Start());
+
+  ASSERT_NO_FATAL_FAILURE(
+      ImportCACertAsTrusted("root_ca_cert.pem", profile_2_cert_db_));
+
+  // Flush the network service instance so persistent NSS Database changes are
+  // reflected in the network service.
+  content::FlushNetworkServiceInstanceForTesting();
+
+  // The certificate that is trusted in |profile_2_| should not be respected in
+  // browsers that belong to |profile_1_|.
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_.GetURL("/ssl/google.html"));
+
+  WebContents* tab_for_profile_1 =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  WaitForInterstitial(tab_for_profile_1);
+  CheckAuthenticationBrokenState(tab_for_profile_1,
+                                 net::CERT_STATUS_AUTHORITY_INVALID,
+                                 AuthState::SHOWING_INTERSTITIAL);
+
+  // The certificate that is trusted in |profile_2_| should be respected in
+  // browsers that belong to |profile_2_|.
+  Browser* browser_for_profile_2 = CreateBrowser(profile_2_);
+  ui_test_utils::NavigateToURL(browser_for_profile_2,
+                               https_server_.GetURL("/ssl/google.html"));
+  WebContents* tab_for_profile_2 =
+      browser_for_profile_2->tab_strip_model()->GetActiveWebContents();
+  CheckAuthenticatedState(tab_for_profile_2, AuthState::NONE);
+}
+
 #endif  // defined(OS_CHROMEOS)
 
 // Regression test for http://crbug.com/635833 (crash when a window with no

services/network/BUILD.gn

@@ -208,6 +208,8 @@ jumbo_component("network_service") {
       "cert_verify_proc_chromeos.h",
       "nss_temp_certs_cache_chromeos.cc",
       "nss_temp_certs_cache_chromeos.h",
+      "system_trust_store_provider_chromeos.cc",
+      "system_trust_store_provider_chromeos.h",
     ]
   }
 

services/network/network_context.cc

@@ -107,10 +107,13 @@
 #if defined(OS_CHROMEOS)
 #include "crypto/nss_util_internal.h"
 #include "net/cert/caching_cert_verifier.h"
+#include "net/cert/cert_verify_proc_builtin.h"
+#include "net/cert/internal/system_trust_store.h"
 #include "net/cert/multi_threaded_cert_verifier.h"
 #include "services/network/cert_verifier_with_trust_anchors.h"
 #include "services/network/cert_verify_proc_chromeos.h"
 #include "services/network/nss_temp_certs_cache_chromeos.h"
+#include "services/network/system_trust_store_provider_chromeos.h"
 #endif
 
 #if !defined(OS_IOS)
@@ -2104,7 +2107,7 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext() {
   if (g_cert_verifier_for_testing) {
     cert_verifier = std::make_unique<WrappedTestingCertVerifier>();
   } else {
-#if defined(OS_ANDROID) || defined(OS_FUCHSIA) || \
+#if defined(OS_ANDROID) || defined(OS_FUCHSIA) || defined(OS_CHROMEOS) || \
     BUILDFLAG(TRIAL_COMPARISON_CERT_VERIFIER_SUPPORTED)
     cert_net_fetcher_ = base::MakeRefCounted<net::CertNetFetcherImpl>();
 #endif
@@ -2112,7 +2115,7 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext() {
     if (params_->username_hash.empty()) {
       cert_verifier = std::make_unique<net::CachingCertVerifier>(
           std::make_unique<net::MultiThreadedCertVerifier>(
-              base::MakeRefCounted<CertVerifyProcChromeOS>()));
+              CreateCertVerifyProcWithoutUserSlots(cert_net_fetcher_)));
     } else {
       // Make sure NSS is initialized for the user.
       crypto::InitializeNSSForChromeOSUser(params_->username_hash,
@@ -2120,8 +2123,9 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext() {
 
       crypto::ScopedPK11Slot public_slot =
           crypto::GetPublicSlotForChromeOSUser(params_->username_hash);
-      scoped_refptr<net::CertVerifyProc> verify_proc(
-          new CertVerifyProcChromeOS(std::move(public_slot)));
+      scoped_refptr<net::CertVerifyProc> verify_proc =
+          CreateCertVerifyProcForUser(cert_net_fetcher_,
+                                      std::move(public_slot));
 
       cert_verifier_with_trust_anchors_ = new CertVerifierWithTrustAnchors(
           base::Bind(&NetworkContext::TrustAnchorUsed, base::Unretained(this)));
@@ -2300,6 +2304,30 @@ void NetworkContext::OnCertVerifyForSignedExchangeComplete(int cert_verify_id,
 void NetworkContext::TrustAnchorUsed() {
   network_service_->client()->OnTrustAnchorUsed(params_->username_hash);
 }
+
+scoped_refptr<net::CertVerifyProc> NetworkContext::CreateCertVerifyProcForUser(
+    scoped_refptr<net::CertNetFetcher> net_fetcher,
+    crypto::ScopedPK11Slot user_public_slot) {
+  if (params_->use_builtin_cert_verifier) {
+    return net::CreateCertVerifyProcBuiltin(
+        std::move(net_fetcher),
+        std::make_unique<SystemTrustStoreProviderChromeOS>(
+            std::move(user_public_slot)));
+  }
+  return base::MakeRefCounted<CertVerifyProcChromeOS>(
+      std::move(user_public_slot));
+}
+
+scoped_refptr<net::CertVerifyProc>
+NetworkContext::CreateCertVerifyProcWithoutUserSlots(
+    scoped_refptr<net::CertNetFetcher> net_fetcher) {
+  if (params_->use_builtin_cert_verifier) {
+    return net::CreateCertVerifyProcBuiltin(
+        std::move(net_fetcher),
+        std::make_unique<SystemTrustStoreProviderChromeOS>());
+  }
+  return base::MakeRefCounted<CertVerifyProcChromeOS>();
+}
 #endif
 
 void NetworkContext::InitializeCorsParams() {

services/network/network_context.h

@@ -51,13 +51,19 @@
 #include "services/network/socket_factory.h"
 #include "services/network/url_request_context_owner.h"
 
+#if defined(OS_CHROMEOS)
+#include "crypto/scoped_nss_types.h"
+#endif
+
 namespace base {
 class UnguessableToken;
 }  // namespace base
 
 namespace net {
+class CertNetFetcher;
 class CertNetFetcherImpl;
 class CertVerifier;
+class CertVerifyProc;
 class HostPortPair;
 class ReportSender;
 class StaticHttpUserAgentSettings;
@@ -448,6 +454,13 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkContext
 
 #if defined(OS_CHROMEOS)
   void TrustAnchorUsed();
+
+  scoped_refptr<net::CertVerifyProc> CreateCertVerifyProcForUser(
+      scoped_refptr<net::CertNetFetcher> net_fetcher,
+      crypto::ScopedPK11Slot user_public_slot);
+
+  scoped_refptr<net::CertVerifyProc> CreateCertVerifyProcWithoutUserSlots(
+      scoped_refptr<net::CertNetFetcher> net_fetcher);
 #endif
 
 #if BUILDFLAG(IS_CT_SUPPORTED)

services/network/public/mojom/network_context.mojom

@@ -325,6 +325,11 @@ struct NetworkContextParams {
   [EnableIf=is_chromeos]
   AdditionalCertificates? initial_additional_certificates;
 
+  // If true, the built-in cert verifier will be used. If false, the platform
+  // cert verifier will be used.
+  [EnableIf=is_chromeos]
+  bool use_builtin_cert_verifier = false;
+
   // Parameters for the cert verifier comparison trial. This is a temporary
   // interface and embedders should not use it.
   // See https://crbug.com/649026

services/network/system_trust_store_provider_chromeos.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/network/system_trust_store_provider_chromeos.h"
+
+#include <pk11pub.h>
+
+#include <utility>
+
+#include "net/cert/internal/system_trust_store_nss.h"
+
+namespace network {
+
+SystemTrustStoreProviderChromeOS::SystemTrustStoreProviderChromeOS() {}
+
+SystemTrustStoreProviderChromeOS::SystemTrustStoreProviderChromeOS(
+    crypto::ScopedPK11Slot user_slot)
+    : user_slot_(std::move(user_slot)) {}
+
+SystemTrustStoreProviderChromeOS::~SystemTrustStoreProviderChromeOS() = default;
+
+std::unique_ptr<net::SystemTrustStore>
+SystemTrustStoreProviderChromeOS::CreateSystemTrustStore() {
+  if (user_slot_) {
+    auto user_slot_copy =
+        crypto::ScopedPK11Slot(PK11_ReferenceSlot(user_slot_.get()));
+    return net::CreateSslSystemTrustStoreNSSWithUserSlotRestriction(
+        std::move(user_slot_copy));
+  }
+
+  return net::CreateSslSystemTrustStoreNSSWithNoUserSlots();
+}
+
+}  // namespace network

services/network/system_trust_store_provider_chromeos.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_NETWORK_SYSTEM_TRUST_STORE_PROVIDER_CHROMEOS_H_
+#define SERVICES_NETWORK_SYSTEM_TRUST_STORE_PROVIDER_CHROMEOS_H_
+
+#include <certt.h>
+#include <memory>
+
+#include "base/macros.h"
+#include "crypto/scoped_nss_types.h"
+#include "net/cert/cert_verify_proc_builtin.h"
+#include "net/cert/nss_profile_filter_chromeos.h"
+
+namespace net {
+class SystemTrustStore;
+}
+
+namespace network {
+
+// A SystemTrustStoreProvider that supports creating SystemTrustStore instances
+// which will only consider user-imported certificates trusted if they are on a
+// specific NSS slot.
+class SystemTrustStoreProviderChromeOS : public net::SystemTrustStoreProvider {
+ public:
+  // Creates a SystemTrustStoreProvider that will provide SystemTrustStore
+  // instances which will not allow trusting user-imported certififcates.
+  SystemTrustStoreProviderChromeOS();
+
+  // Creates a SystemTrustStoreProvider that will provide SystemTrustStore
+  // instances which will only consider user-imported certificates trusted if
+  // they are on |user_slot|.
+  explicit SystemTrustStoreProviderChromeOS(crypto::ScopedPK11Slot user_slot);
+
+  ~SystemTrustStoreProviderChromeOS() override;
+
+  // As SystemTrustStoreProvider states, this must be thread-safe and will be
+  // called concurrently from worker threads.
+  std::unique_ptr<net::SystemTrustStore> CreateSystemTrustStore() override;
+
+ private:
+  const crypto::ScopedPK11Slot user_slot_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemTrustStoreProviderChromeOS);
+};
+
+}  // namespace network
+
+#endif  // SERVICES_NETWORK_SYSTEM_TRUST_STORE_PROVIDER_CHROMEOS_H_