system-proxy: Secure policy credentials

Currently System-proxy sends the policy set credentials with every
connect request to a remote proxy. Since less secure authentication
schemes send the credentials in clear to the proxy, an attacker can
easily obtain the policy set credentials.

This CL allows admins to restrict the auth schemes for which
System-proxy can use the policy set credentials.

      - manually tested proxies set by extensions (managed and user
      installed extensions)

Bug: 1132247
Test: - browser_tests  --gtest_filter=SystemProxyManagerPolicyCredentialsBrowserTest
Change-Id: I4583762b565ec43bfdad8be4c6db87d3e2bec223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2484444Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Commit-Queue: Andreea-Elena Costinas <acostinas@google.com>
Cr-Commit-Position: refs/heads/master@{#823964}

chrome/browser/chromeos/policy/system_proxy_manager.cc

@@ -20,12 +20,17 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/system_proxy/system_proxy_client.h"
 #include "chromeos/network/network_event_log.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/proxy/proxy_config_service_impl.h"
+#include "chromeos/network/proxy/ui_proxy_config_service.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
 #include "components/arc/arc_prefs.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/proxy_config/proxy_config_pref_names.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/storage_partition.h"
@@ -70,12 +75,18 @@ SystemProxyManager::SystemProxyManager(chromeos::CrosSettings* cros_settings,
       prefs::kKerberosEnabled,
       base::BindRepeating(&SystemProxyManager::OnKerberosEnabledChanged,
                           weak_factory_.GetWeakPtr()));
-
+  DCHECK(chromeos::NetworkHandler::IsInitialized());
+  chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver(
+      this, FROM_HERE);
   // Fire it once so we're sure we get an invocation on startup.
   OnSystemProxySettingsPolicyChanged();
 }
 
-SystemProxyManager::~SystemProxyManager() = default;
+SystemProxyManager::~SystemProxyManager() {
+  DCHECK(chromeos::NetworkHandler::IsInitialized());
+  chromeos::NetworkHandler::Get()->network_state_handler()->RemoveObserver(
+      this, FROM_HERE);
+}
 
 std::string SystemProxyManager::SystemServicesProxyPacString() const {
   return system_proxy_enabled_ && !system_services_address_.empty()
@@ -85,6 +96,7 @@ std::string SystemProxyManager::SystemServicesProxyPacString() const {
 
 void SystemProxyManager::StartObservingPrimaryProfilePrefs(Profile* profile) {
   primary_profile_ = profile;
+  extension_prefs_util_ = std::make_unique<extensions::PrefsUtil>(profile);
   // Listen to pref changes.
   profile_pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
   profile_pref_change_registrar_->Init(primary_profile_->GetPrefs());
@@ -96,7 +108,12 @@ void SystemProxyManager::StartObservingPrimaryProfilePrefs(Profile* profile) {
       arc::prefs::kArcEnabled,
       base::BindRepeating(&SystemProxyManager::OnArcEnabledChanged,
                           weak_factory_.GetWeakPtr()));
+  profile_pref_change_registrar_->Add(
+      proxy_config::prefs::kProxy,
+      base::BindRepeating(&SystemProxyManager::OnProxyConfigChanged,
+                          base::Unretained(this)));
   if (system_proxy_enabled_) {
+    OnProxyConfigChanged();
     OnKerberosAccountChanged();
     OnArcEnabledChanged();
   }
@@ -105,6 +122,7 @@ void SystemProxyManager::StartObservingPrimaryProfilePrefs(Profile* profile) {
 void SystemProxyManager::StopObservingPrimaryProfilePrefs() {
   profile_pref_change_registrar_->RemoveAll();
   profile_pref_change_registrar_.reset();
+  extension_prefs_util_.reset();
   primary_profile_ = nullptr;
 }
 
@@ -153,8 +171,6 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
     CloseAuthenticationUI();
     return;
   }
-  system_proxy::SetAuthenticationDetailsRequest request;
-  system_proxy::Credentials credentials;
   const std::string* username = proxy_settings->FindStringKey(
       chromeos::kSystemProxySettingsKeySystemServicesUsername);
 
@@ -165,16 +181,26 @@ void SystemProxyManager::OnSystemProxySettingsPolicyChanged() {
     NET_LOG(DEBUG) << "Proxy credentials for system traffic not set: "
                    << kSystemProxyService;
   } else {
-    credentials.set_username(*username);
-    credentials.set_password(*password);
-    *request.mutable_credentials() = credentials;
+    system_services_username_ = *username;
+    system_services_password_ = *password;
+  }
+  if (IsManagedProxyConfigured()) {
+    SendPolicyAuthenticationCredentials(system_services_username_,
+                                        system_services_password_,
+                                        /*force_send=*/true);
+  } else {
+    // To avoid leaking the policy set credentials, don't send them to
+    // System-proxy if there's no managed proxy on the network.
+    // Note: When SystemProxyManager is starting, the credentials are empty and
+    // they were never sent before. We need to force send them, otherwise
+    // `SendPolicyAuthenticationCredentials` will detect that no change to the
+    // credentials occurred and will not trigger a D-Bus request. This means the
+    // worker service for Chrome OS system services will not be started.
+    SendPolicyAuthenticationCredentials(/*username=*/"",
+                                        /*password=*/"",
+                                        /*force_send=*/true);
   }
 
-  request.set_traffic_type(system_proxy::TrafficOrigin::SYSTEM);
-
-  chromeos::SystemProxyClient::Get()->SetAuthenticationDetails(
-      request, base::BindOnce(&SystemProxyManager::OnSetAuthenticationDetails,
-                              weak_factory_.GetWeakPtr()));
   // Fire once to cover the case where the SystemProxySetting policy is set
   // during a user session.
   if (IsArcEnabled()) {
@@ -251,6 +277,35 @@ void SystemProxyManager::SendUserAuthenticationCredentials(
                               weak_factory_.GetWeakPtr()));
 }
 
+void SystemProxyManager::SendPolicyAuthenticationCredentials(
+    const std::string& username,
+    const std::string& password,
+    bool force_send) {
+  if (!system_proxy_enabled_)
+    return;
+
+  if (!force_send &&
+      (last_sent_username_ == username && last_sent_password_ == password)) {
+    // Credentials were already sent.
+    return;
+  }
+
+  last_sent_username_ = username;
+  last_sent_password_ = password;
+
+  system_proxy::SetAuthenticationDetailsRequest request;
+  system_proxy::Credentials credentials;
+  credentials.set_username(username);
+  credentials.set_password(password);
+  *request.mutable_credentials() = credentials;
+
+  request.set_traffic_type(system_proxy::TrafficOrigin::SYSTEM);
+
+  chromeos::SystemProxyClient::Get()->SetAuthenticationDetails(
+      request, base::BindOnce(&SystemProxyManager::OnSetAuthenticationDetails,
+                              weak_factory_.GetWeakPtr()));
+}
+
 void SystemProxyManager::SendKerberosAuthenticationDetails() {
   if (!system_proxy_enabled_) {
     return;
@@ -322,6 +377,62 @@ void SystemProxyManager::OnSetAuthenticationDetails(
     send_auth_details_closure_for_test_.Run();
 }
 
+// This function is called when the default network changes or when any of its
+// properties change.
+void SystemProxyManager::DefaultNetworkChanged(
+    const chromeos::NetworkState* network) {
+  if (!network)
+    return;
+  OnProxyConfigChanged();
+}
+
+void SystemProxyManager::OnProxyConfigChanged() {
+  if (!IsManagedProxyConfigured()) {
+    SendPolicyAuthenticationCredentials(/*username=*/"", /*password=*/"",
+                                        /*force_send=*/false);
+    return;
+  }
+  SendPolicyAuthenticationCredentials(system_services_username_,
+                                      system_services_password_,
+                                      /*force_send=*/false);
+}
+
+bool SystemProxyManager::IsManagedProxyConfigured() {
+  DCHECK(chromeos::NetworkHandler::IsInitialized());
+  chromeos::NetworkHandler* network_handler = chromeos::NetworkHandler::Get();
+  base::Value proxy_settings(base::Value::Type::DICTIONARY);
+
+  // |ui_proxy_config_service| may be missing in tests. If the device is offline
+  // (no network connected) the |DefaultNetwork| is null.
+  if (chromeos::NetworkHandler::HasUiProxyConfigService() &&
+      network_handler->network_state_handler()->DefaultNetwork()) {
+    // Check if proxy is enforced by user policy, force installed extension or
+    // ONC policies. This will only read managed settings.
+    chromeos::NetworkHandler::GetUiProxyConfigService()
+        ->MergeEnforcedProxyConfig(
+            network_handler->network_state_handler()->DefaultNetwork()->guid(),
+            &proxy_settings);
+  }
+  if (proxy_settings.DictEmpty())
+    return false;  // no managed proxy set
+
+  if (IsProxyConfiguredByUserViaExtension())
+    return false;
+
+  // Proxy was configured by the admin
+  return true;
+}
+
+bool SystemProxyManager::IsProxyConfiguredByUserViaExtension() {
+  if (!extension_prefs_util_)
+    return false;
+
+  std::unique_ptr<extensions::api::settings_private::PrefObject> pref =
+      extension_prefs_util_->GetPref(proxy_config::prefs::kProxy);
+  return pref && pref->extension_can_be_disabled &&
+         *pref->extension_can_be_disabled;
+}
+
 void SystemProxyManager::OnShutDownProcess(
     const system_proxy::ShutDownResponse& response) {
   if (response.has_error_message() && !response.error_message().empty()) {

chrome/browser/chromeos/policy/system_proxy_manager.h

@@ -13,10 +13,13 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
+#include "chromeos/network/network_state_handler_observer.h"
 #include "net/base/auth.h"
 
 namespace chromeos {
+class NetworkState;
 class RequestSystemProxyCredentialsView;
 class SystemProxyNotification;
 }  // namespace chromeos
@@ -43,7 +46,9 @@ namespace policy {
 // also listens for the |WorkerActive| dbus signal sent by the System-proxy
 // daemon and stores connection information regarding the active worker
 // processes.
-class SystemProxyManager {
+// TODO(acostinas, https://crbug.com/1145174): Move the logic that tracks
+// managed network changes to another class.
+class SystemProxyManager : public chromeos::NetworkStateHandlerObserver {
  public:
   SystemProxyManager(chromeos::CrosSettings* cros_settings,
                      PrefService* local_state);
@@ -51,7 +56,7 @@ class SystemProxyManager {
 
   SystemProxyManager& operator=(const SystemProxyManager&) = delete;
 
-  ~SystemProxyManager();
+  ~SystemProxyManager() override;
 
   // If System-proxy is enabled by policy, it returns the URL of the local proxy
   // instance that authenticates system services, in PAC format, e.g.
@@ -76,6 +81,18 @@ class SystemProxyManager {
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
  private:
+  // NetworkStateHandlerObserver implementation
+  void DefaultNetworkChanged(const chromeos::NetworkState* network) override;
+  // Called when the proxy configurations may have changed either by updates to
+  // the kProxy policy or updates to the default network.
+  void OnProxyConfigChanged();
+  // Returns true if there's a policy configured proxy on the default network
+  // (via device or user ONC policy, user policy or force installed extension).
+  bool IsManagedProxyConfigured();
+  // Returns true if the `kProxy` preference set by an extension can be changed
+  // by the user.
+  bool IsProxyConfiguredByUserViaExtension();
+
   void OnSetAuthenticationDetails(
       const system_proxy::SetAuthenticationDetailsResponse& response);
   void OnShutDownProcess(const system_proxy::ShutDownResponse& response);
@@ -95,6 +112,14 @@ class SystemProxyManager {
       const system_proxy::ProtectionSpace& protection_space,
       const std::string& username,
       const std::string& password);
+
+  // Sends policy set credentials to System-proxy via D-Bus. Credentials are
+  // sent only if `username` and `password` are different than
+  // `last_sent_username_` and `last_sent_password_` or if `force_send` is true.
+  void SendPolicyAuthenticationCredentials(const std::string& username,
+                                           const std::string& password,
+                                           bool force_send);
+
   // Send the Kerberos enabled state and active principal name to System-proxy
   // via D-Bus.
   void SendKerberosAuthenticationDetails();
@@ -153,6 +178,15 @@ class SystemProxyManager {
   // The authority URI in the format host:port of the local proxy worker for
   // system services.
   std::string system_services_address_;
+  std::string system_services_username_;
+  std::string system_services_password_;
+  // The credentials which were last sent to System-proxy. They can differ from
+  // `system_services_username_` and `system_services_username_` if the proxy
+  // configuration is not managed; in this case `last_sent_username_` and
+  // `last_sent_password_` are both empty even if credentials were specified by
+  // policy.
+  std::string last_sent_username_;
+  std::string last_sent_password_;
 
   // Local state prefs, not owned.
   PrefService* local_state_ = nullptr;
@@ -168,6 +202,7 @@ class SystemProxyManager {
 
   // Primary profile, not owned.
   Profile* primary_profile_ = nullptr;
+  std::unique_ptr<extensions::PrefsUtil> extension_prefs_util_;
 
   // Observer for Kerberos-related prefs.
   std::unique_ptr<PrefChangeRegistrar> local_state_pref_change_registrar_;

chrome/browser/chromeos/policy/system_proxy_manager_unittest.cc

@@ -14,8 +14,10 @@
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/dbus/shill/shill_clients.h"
 #include "chromeos/dbus/system_proxy/system_proxy_client.h"
 #include "chromeos/dbus/system_proxy/system_proxy_service.pb.h"
+#include "chromeos/network/network_handler.h"
 #include "components/arc/arc_prefs.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/network_service_instance.h"
@@ -39,8 +41,6 @@ using testing::Invoke;
 using testing::WithArg;
 
 namespace {
-constexpr char kSystemServicesUsername[] = "test_username";
-constexpr char kSystemServicesPassword[] = "test_password";
 constexpr char kBrowserUsername[] = "browser_username";
 constexpr char kBrowserPassword[] = "browser_password";
 constexpr char kKerberosActivePrincipalName[] = "kerberos_princ_name";
@@ -86,6 +86,9 @@ class SystemProxyManagerTest : public testing::Test {
   // testing::Test
   void SetUp() override {
     testing::Test::SetUp();
+    chromeos::shill_clients::InitializeFakes();
+    chromeos::NetworkHandler::Initialize();
+
     profile_ = std::make_unique<TestingProfile>();
     chromeos::SystemProxyClient::InitializeFake();
     system_proxy_manager_ = std::make_unique<SystemProxyManager>(
@@ -96,7 +99,10 @@ class SystemProxyManagerTest : public testing::Test {
 
   void TearDown() override {
     system_proxy_manager_->StopObservingPrimaryProfilePrefs();
+    system_proxy_manager_.reset();
     chromeos::SystemProxyClient::Shutdown();
+    chromeos::NetworkHandler::Shutdown();
+    chromeos::shill_clients::Shutdown();
   }
 
  protected:
@@ -127,28 +133,6 @@ class SystemProxyManagerTest : public testing::Test {
   chromeos::ScopedStubInstallAttributes test_install_attributes_;
 };
 
-// Verifies that System-proxy is configured with the system traffic credentials
-// set by |kSystemProxySettings| policy.
-TEST_F(SystemProxyManagerTest, SetAuthenticationDetails) {
-  EXPECT_EQ(0, client_test_interface()->GetSetAuthenticationDetailsCallCount());
-
-  SetPolicy(true /* system_proxy_enabled */, "" /* system_services_username */,
-            "" /* system_services_password */);
-  // Don't send empty credentials.
-  EXPECT_EQ(1, client_test_interface()->GetSetAuthenticationDetailsCallCount());
-
-  SetPolicy(true /* system_proxy_enabled */, kSystemServicesUsername,
-            kSystemServicesPassword);
-  EXPECT_EQ(2, client_test_interface()->GetSetAuthenticationDetailsCallCount());
-
-  system_proxy::SetAuthenticationDetailsRequest request =
-      client_test_interface()->GetLastAuthenticationDetailsRequest();
-
-  ASSERT_TRUE(request.has_credentials());
-  EXPECT_EQ(kSystemServicesUsername, request.credentials().username());
-  EXPECT_EQ(kSystemServicesPassword, request.credentials().password());
-}
-
 // Verifies requests to shut down are sent to System-proxy according to the
 // |kSystemProxySettings| policy.
 TEST_F(SystemProxyManagerTest, ShutDownDaemon) {