ChromeOS: don't always restart net service when kerberos enabled

Since the network service sandbox and kerberos are incompatible,
a sandboxed network service has to be restarted unsandboxed when
an admin enables kerberos (at runtime) on ChromeOS.

Chrome was mistakenly restarting the network service the first time
an admin enabled kerberos, even if the network service was unsandboxed
(i.e. NetworkServiceSandbox feature is disabled). This was unnecessary
and tickled another bug (see crbug.com/1494567).

So the fix is to set `g_network_service_will_allow_gssapi_library_load`
to true for the duration of the process if the network service sandbox
is disabled by feature state.

Change-Id: Id11919f5dc728192aba18733933aa9c51b979a24
Bug: 1494567, 1474362, b/299795786
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4933285
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Felipe Andrade <fsandrade@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1213093}

chrome/browser/net/system_network_context_manager.cc

@@ -267,7 +267,6 @@ NetworkSandboxState IsNetworkSandboxEnabledInternal() {
   if (g_network_service_will_allow_gssapi_library_load ||
       (local_state && local_state->HasPrefPath(kGssapiDesiredPref) &&
        local_state->GetBoolean(kGssapiDesiredPref))) {
-    g_network_service_will_allow_gssapi_library_load = true;
     return NetworkSandboxState::kDisabledBecauseOfKerberos;
   }
 #endif  // BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX)
@@ -917,20 +916,35 @@ bool SystemNetworkContextManager::IsNetworkSandboxEnabled() {
   base::UmaHistogramEnumeration(
       "Chrome.SystemNetworkContextManager.NetworkSandboxState", state);
 
+  bool enabled = true;
   switch (state) {
     case NetworkSandboxState::kDisabledBecauseOfKerberos:
-      return false;
+      enabled = false;
+      break;
     case NetworkSandboxState::kDisabledByPlatform:
-      return false;
+      enabled = false;
+      break;
     case NetworkSandboxState::kEnabledByPlatform:
-      return true;
+      enabled = true;
+      break;
     case NetworkSandboxState::kDisabledByPolicy:
-      return false;
+      enabled = false;
+      break;
     case NetworkSandboxState::kEnabledByPolicy:
-      return true;
+      enabled = true;
+      break;
     case NetworkSandboxState::kDisabledBecauseOfFailedLaunch:
-      return false;
+      enabled = false;
+      break;
   }
+
+#if BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX)
+  if (!enabled) {
+    g_network_service_will_allow_gssapi_library_load = true;
+  }
+#endif  // BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX)
+
+  return enabled;
 }
 
 void SystemNetworkContextManager::FlushSSLConfigManagerForTesting() {

chrome/browser/net/system_network_context_manager_browsertest.cc

@@ -227,9 +227,10 @@ IN_PROC_BROWSER_TEST_F(SystemNetworkContextManagerBrowsertest, AuthParams) {
 // whether GSSAPI is desired, so if Chrome detects that GSSAPI is desired after
 // the network service has already started sandboxed, the network
 // service must be restarted so the sandbox can be removed.
-class SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest
+class SystemNetworkContextManagerNetworkServiceSandboxBrowsertest
     : public SystemNetworkContextManagerBrowsertest,
-      public content::ServiceProcessHost::Observer {
+      public content::ServiceProcessHost::Observer,
+      public testing::WithParamInterface<bool> {
  public:
   // On both ChromeOS and Linux, a pref determines whether GSSAPI is desired in
   // the network service. This pref will determine whether the network service
@@ -242,9 +243,10 @@ class SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest
       prefs::kReceivedHttpAuthNegotiateHeader;
 #endif
 
-  SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest() {
-    scoped_feature_list_.InitAndEnableFeature(
-        sandbox::policy::features::kNetworkServiceSandbox);
+  SystemNetworkContextManagerNetworkServiceSandboxBrowsertest() {
+    sandbox_desired_ = GetParam();
+    scoped_feature_list_.InitWithFeatureState(
+        sandbox::policy::features::kNetworkServiceSandbox, sandbox_desired_);
   }
 
   void SetUpOnMainThread() override {
@@ -272,28 +274,37 @@ class SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest
     launch_run_loop_->Run();
   }
 
-  void WaitForNetworkServiceReady() {
+  void ExpectNetworkService(bool seccomp_sandboxed,
+                            bool allows_gssapi_library_load) {
+    // The network service may have been launched but has not yet sandboxed
+    // itself. So, wait for the Mojo endpoints to start accepting messages.
     mojo::Remote<network::mojom::NetworkServiceTest> network_service_test;
     content::GetNetworkService()->BindTestInterfaceForTesting(
         network_service_test.BindNewPipeAndPassReceiver());
     mojo::ScopedAllowSyncCallForTesting allow_sync_call;
     // Log() is sync so this thread will wait for this call to succeed.
     network_service_test->Log(
         "Logging in network service to ensure it's ready.");
-  }
 
-  void ExpectNetworkServiceSeccompSandboxed(bool sandboxed) {
-    // The network service may have been launched but has not yet sandboxed
-    // itself. So, wait for the Mojo endpoints to start accepting messages.
-    WaitForNetworkServiceReady();
-    EXPECT_EQ(sandboxed, GetNetworkServiceProcess().IsSeccompSandboxed());
+    // Now the test can check if the seccomp sandbox has been applied.
+    EXPECT_EQ(seccomp_sandboxed,
+              GetNetworkServiceProcess().IsSeccompSandboxed());
+
+    bool network_service_allows_gssapi_library_load;
+    ASSERT_TRUE(network_service_test->AllowsGSSAPILibraryLoad(
+        &network_service_allows_gssapi_library_load));
+    EXPECT_EQ(allows_gssapi_library_load,
+              network_service_allows_gssapi_library_load);
   }
 
   base::Process GetNetworkServiceProcess() {
     CHECK(content::IsOutOfProcessNetworkService());
     return network_process_.Duplicate();
   }
 
+ protected:
+  bool sandbox_desired_;
+
  private:
   void OnServiceProcessLaunched(
       const content::ServiceProcessInfo& info) override {
@@ -317,33 +328,42 @@ class SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest
   absl::optional<base::RunLoop> launch_run_loop_;
 };
 
-IN_PROC_BROWSER_TEST_F(
-    SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest,
+IN_PROC_BROWSER_TEST_P(
+    SystemNetworkContextManagerNetworkServiceSandboxBrowsertest,
     NetworkServiceRestartsUnsandboxedOnGssapiDesired) {
   PrefService* local_state = g_browser_process->local_state();
 
   // Ensure GSSAPI starts as "undesired".
   EXPECT_FALSE(local_state->GetBoolean(kGssapiDesiredPref));
-  // Ensure the network service starts sandboxed.
-  ExpectNetworkServiceSeccompSandboxed(/*sandboxed=*/true);
+  // Ensure the network service starts sandboxed (if desired) and cannot load
+  // GSSAPI libraries.
+  ExpectNetworkService(/*seccomp_sandboxed=*/sandbox_desired_,
+                       /*allows_gssapi_library_load=*/false);
 
   // Now signal that GSSAPI is desired.
   local_state->SetBoolean(kGssapiDesiredPref, true);
   EXPECT_TRUE(local_state->GetBoolean(kGssapiDesiredPref));
-  // The network service should automatically restart, and be unsandboxed.
-  WaitForNextLaunch();
-  ExpectNetworkServiceSeccompSandboxed(/*sandboxed=*/false);
+  // If the network service was sandboxed it should automatically restart and
+  // be unsandboxed. In any case it should now respect the pref and allow GSSAPI
+  // library loads.
+  if (sandbox_desired_) {
+    WaitForNextLaunch();
+  }
+  ExpectNetworkService(/*seccomp_sandboxed=*/false,
+                       /*allows_gssapi_library_load=*/true);
 
-  // After killing the network service, it should still restart unsandboxed.
+  // After killing the network service, it should still restart unsandboxed and
+  // allow GSSAPI library loads.
   base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
       FROM_HERE,
       base::BindOnce(base::IgnoreResult(&content::RestartNetworkService)));
   WaitForNextLaunch();
-  ExpectNetworkServiceSeccompSandboxed(/*sandboxed=*/false);
+  ExpectNetworkService(/*seccomp_sandboxed=*/false,
+                       /*allows_gssapi_library_load=*/true);
 }
 
-IN_PROC_BROWSER_TEST_F(
-    SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest,
+IN_PROC_BROWSER_TEST_P(
+    SystemNetworkContextManagerNetworkServiceSandboxBrowsertest,
     PRE_NetworkServiceStartsUnsandboxedWithGssapiDesired) {
   PrefService* local_state = g_browser_process->local_state();
   // Signal that GSSAPI is desired. This should persist across browser restarts
@@ -352,13 +372,24 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_TRUE(local_state->GetBoolean(kGssapiDesiredPref));
 }
 
-IN_PROC_BROWSER_TEST_F(
-    SystemNetworkContextManagerNetworkServiceSandboxEnabledBrowsertest,
+IN_PROC_BROWSER_TEST_P(
+    SystemNetworkContextManagerNetworkServiceSandboxBrowsertest,
     NetworkServiceStartsUnsandboxedWithGssapiDesired) {
-  // Ensure the network service starts sandboxed.
-  ExpectNetworkServiceSeccompSandboxed(/*sandboxed=*/false);
+  // Ensure the network service starts sandboxed and allows GSSAPI library
+  // loads.
+  ExpectNetworkService(/*seccomp_sandboxed=*/false,
+                       /*allows_gssapi_library_load=*/true);
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    SystemNetworkContextManagerNetworkServiceSandboxBrowsertest,
+    testing::Bool(),
+    [](const testing::TestParamInfo<bool>& info) {
+      return info.param ? "NetworkSandboxDesired"
+                        : "NetworkSandboxFullyDisabled";
+    });
+
 #endif  // BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX)
 
 #if BUILDFLAG(IS_LINUX)

content/public/test/network_service_test_helper.cc

@@ -776,6 +776,21 @@ class NetworkServiceTestHelper::NetworkServiceTestImpl
   }
 #endif  // BUILDFLAG(IS_LINUX)
 
+  void AllowsGSSAPILibraryLoad(
+      AllowsGSSAPILibraryLoadCallback callback) override {
+    bool allow_gssapi_library_load;
+#if BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX)
+    allow_gssapi_library_load =
+        network::NetworkService::GetNetworkServiceForTesting()
+            ->http_auth_dynamic_network_service_params_for_testing()
+            ->allow_gssapi_library_load;
+#else
+    allow_gssapi_library_load = true;
+#endif
+
+    std::move(callback).Run(allow_gssapi_library_load);
+  }
+
  private:
   void OnMemoryPressure(
       base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {

services/network/network_service.h

@@ -340,6 +340,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkService
 
   bool data_use_updates_enabled() const { return data_use_updates_enabled_; }
 
+  const mojom::HttpAuthDynamicParamsPtr&
+  http_auth_dynamic_network_service_params_for_testing() const {
+    return http_auth_dynamic_network_service_params_;
+  }
+
   mojom::URLLoaderNetworkServiceObserver*
   GetDefaultURLLoaderNetworkServiceObserver();
 

services/network/public/mojom/network_service_test.mojom

@@ -252,7 +252,14 @@ interface NetworkServiceTest {
   // Sets the last IPv6 probe result.
   SetIPv6ProbeResult(bool success) => ();
 
+  // Get the network service's cached AddressMap, which contains a list of
+  // network interfaces on the user's machine.
   [Sync, EnableIf=use_network_interface_change_listener]
   GetAddressMapCacheLinux()
       => (network.mojom.AddressMap addr_map, network.mojom.OnlineLinks links);
+
+  // Returns true if the network service is allowed to load a GSSAPI library.
+  // Only relevant on ChromeOS and Linux.
+  [Sync]
+  AllowsGSSAPILibraryLoad() => (bool allow_gssapi_library_load);
 };