Add enterprise policy to control audio sandbox.

Some enterprise security systems or configurations can interfere with the audio sandbox and render audio unavailable. This policy allows enterprises to enable or disable the sandbox, which gives them flexibility to update their security setup without disrupting audio. Inspired by crrev.com/c/1465172 In addition, this CL enforces that running the APM in the audio service is possible only if the service is running in a sandboxed untility process. If the service runs in the browser, or in an unsandboxed utility process, the APM will run in the renderer process. The APM has never been configured to run unsandboxed, but it was possible to do it by explicitly setting the feature flags. Since the new policy provides another way to set flags, it is better to enforce that the APM always runs sandboxed no matter what the flag values are. Bug: 1009332 Change-Id: I09405d3058b7bf9b6f61e44583284ceaa8f0ed27 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1831763 Commit-Queue: Guido Urdaneta <guidou@chromium.org> Reviewed-by: Olga Sharonova <olka@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org> Cr-Commit-Position: refs/heads/master@{#704781}
FairyWorld · Oct 10, 2019 · d06ae84 · d06ae84
1 parent 274dc97
commit d06ae84
Show file tree

Hide file tree

Showing 20 changed files with 211 additions and 43 deletions.
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -309,6 +309,7 @@
 #include "media/base/media_switches.h"
 #include "media/media_buildflags.h"
 #include "media/mojo/buildflags.h"
+#include "media/webrtc/webrtc_switches.h"
 #include "mojo/public/cpp/bindings/pending_associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -456,6 +457,7 @@
 #if defined(OS_WIN) || defined(OS_MACOSX) || \
     (defined(OS_LINUX) && !defined(OS_CHROMEOS))
 #include "chrome/browser/browser_switcher/browser_switcher_navigation_throttle.h"
+#include "services/service_manager/sandbox/features.h"
 #endif
 
 #if defined(OS_LINUX)
@@ -3895,22 +3897,43 @@ void ChromeContentBrowserClient::BindHostReceiverForRendererOnIOThread(
 }
 
 void ChromeContentBrowserClient::WillStartServiceManager() {
-#if defined(OS_WIN)
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
   if (startup_data_) {
     auto* chrome_feature_list_creator =
         startup_data_->chrome_feature_list_creator();
     // This has to run very early before ServiceManagerContext is created.
-    const base::Value* force_network_in_process_value =
+    const policy::PolicyMap& policies =
         chrome_feature_list_creator->browser_policy_connector()
             ->GetPolicyService()
             ->GetPolicies(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME,
-                                                  std::string()))
-            .GetValue(policy::key::kForceNetworkInProcess);
+                                                  std::string()));
+
+#if defined(OS_WIN)
+    const base::Value* force_network_in_process_value =
+        policies.GetValue(policy::key::kForceNetworkInProcess);
     bool force_network_in_process = false;
     if (force_network_in_process_value)
       force_network_in_process_value->GetAsBoolean(&force_network_in_process);
     if (force_network_in_process)
       content::ForceInProcessNetworkService(true);
+#endif
+    bool enable_audio_process =
+        base::FeatureList::IsEnabled(features::kAudioServiceOutOfProcess);
+    bool enable_audio_sandbox = base::FeatureList::IsEnabled(
+        service_manager::features::kAudioServiceSandbox);
+    const base::Value* audio_sandbox_enabled_policy_value =
+        policies.GetValue(policy::key::kAudioSandboxEnabled);
+    if (audio_sandbox_enabled_policy_value)
+      audio_sandbox_enabled_policy_value->GetAsBoolean(&enable_audio_sandbox);
+    service_manager::EnableAudioSandbox(enable_audio_sandbox);
+    if (!enable_audio_sandbox || !enable_audio_process) {
+      // Disabling the audio process or audio sandbox implies disabling APM in
+      // the audio service for security reasons. Append a switch so that this
+      // is communicated to the audio and renderer processes.
+      base::CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kForceDisableWebRtcApmInAudioService);
+    }
   }
 #endif
 }

diff --git a/chrome/browser/media/webrtc/webrtc_text_log_handler.cc b/chrome/browser/media/webrtc/webrtc_text_log_handler.cc
@@ -38,6 +38,7 @@
 #include "net/base/network_interfaces.h"
 #include "services/network/public/mojom/network_service.mojom.h"
 #include "services/service_manager/sandbox/features.h"
+#include "services/service_manager/sandbox/sandbox_type.h"
 
 #if defined(OS_LINUX)
 #include "base/linux_util.h"
@@ -472,21 +473,26 @@ void WebRtcTextLogHandler::OnGetNetworkInterfaceList(
                       ", gl-version=" + gpu_info.gl_version);
 
   // AudioService features
-  auto enabled_or_disabled_string = [](auto& feature) {
+  auto enabled_or_disabled_feature_string = [](auto& feature) {
     return base::FeatureList::IsEnabled(feature) ? "enabled" : "disabled";
   };
+  auto enabled_or_disabled_bool_string = [](bool value) {
+    return value ? "enabled" : "disabled";
+  };
   LogToCircularBuffer(base::StrCat(
       {"AudioService: AudioStreams=",
-       enabled_or_disabled_string(features::kAudioServiceAudioStreams),
+       enabled_or_disabled_feature_string(features::kAudioServiceAudioStreams),
        ", OutOfProcess=",
-       enabled_or_disabled_string(features::kAudioServiceOutOfProcess),
+       enabled_or_disabled_feature_string(features::kAudioServiceOutOfProcess),
        ", LaunchOnStartup=",
-       enabled_or_disabled_string(features::kAudioServiceLaunchOnStartup),
+       enabled_or_disabled_feature_string(
+           features::kAudioServiceLaunchOnStartup),
        ", Sandbox=",
-       enabled_or_disabled_string(
-           service_manager::features::kAudioServiceSandbox),
+       enabled_or_disabled_bool_string(
+           service_manager::IsAudioSandboxEnabled()),
        ", ApmInAudioService=",
-       enabled_or_disabled_string(features::kWebRtcApmInAudioService)}));
+       enabled_or_disabled_bool_string(
+           media::IsWebRtcApmInAudioServiceEnabled())}));
 
   // Audio manager
   // On some platforms, this can vary depending on build flags and failure

diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
@@ -269,6 +269,12 @@
 #include "chrome/browser/ui/toolbar/media_router_action_controller.h"
 #endif
 
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+#include "media/webrtc/webrtc_switches.h"
+#include "services/service_manager/sandbox/features.h"
+#endif
+
 using content::BrowserThread;
 using safe_browsing::ReusedPasswordAccountType;
 using testing::_;
@@ -5820,4 +5826,66 @@ IN_PROC_BROWSER_TEST_F(SharedClipboardPolicyTest, SharedClipboardEnabled) {
   EXPECT_TRUE(prefs->GetBoolean(prefs::kSharedClipboardEnabled));
 }
 
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+
+class AudioSandboxEnabledTest
+    : public InProcessBrowserTest,
+      public ::testing::WithParamInterface<
+          /*policy::key::kAllowAudioSandbox=*/base::Optional<bool>> {
+ public:
+  // InProcessBrowserTest implementation:
+  void SetUp() override {
+    EXPECT_CALL(policy_provider_, IsInitializationComplete(testing::_))
+        .WillRepeatedly(testing::Return(true));
+    policy::PolicyMap values;
+    if (GetParam().has_value()) {
+      values.Set(policy::key::kAudioSandboxEnabled,
+                 policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_MACHINE,
+                 policy::POLICY_SOURCE_CLOUD,
+                 std::make_unique<base::Value>(*GetParam()), nullptr);
+    }
+    policy_provider_.UpdateChromePolicy(values);
+    policy::BrowserPolicyConnector::SetPolicyProviderForTesting(
+        &policy_provider_);
+
+    InProcessBrowserTest::SetUp();
+  }
+
+ private:
+  policy::MockConfigurationPolicyProvider policy_provider_;
+};
+
+IN_PROC_BROWSER_TEST_P(AudioSandboxEnabledTest, IsRespected) {
+  base::Optional<bool> enable_sandbox_via_policy = GetParam();
+  bool is_sandbox_enabled_by_default = base::FeatureList::IsEnabled(
+      service_manager::features::kAudioServiceSandbox);
+  bool is_apm_enabled_by_default =
+      base::FeatureList::IsEnabled(features::kWebRtcApmInAudioService);
+
+  ASSERT_EQ(enable_sandbox_via_policy.value_or(is_sandbox_enabled_by_default),
+            service_manager::IsAudioSandboxEnabled());
+  ASSERT_EQ(
+      is_apm_enabled_by_default && service_manager::IsAudioSandboxEnabled(),
+      media::IsWebRtcApmInAudioServiceEnabled());
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    Enabled,
+    AudioSandboxEnabledTest,
+    ::testing::Values(/*policy::key::kAudioSandboxEnabled=*/true));
+
+INSTANTIATE_TEST_SUITE_P(
+    Disabled,
+    AudioSandboxEnabledTest,
+    ::testing::Values(/*policy::key::kAudioSandboxEnabled=*/false));
+
+INSTANTIATE_TEST_SUITE_P(
+    NotSet,
+    AudioSandboxEnabledTest,
+    ::testing::Values(/*policy::key::kAudioSandboxEnabled=*/base::nullopt));
+
+#endif  //  defined(OS_WIN) || defined (OS_MACOSX) || (defined(OS_LINUX) &&
+        //  !defined(OS_CHROMEOS))
+
 }  // namespace policy
diff --git a/chrome/test/data/policy/policy_test_cases.json b/chrome/test/data/policy/policy_test_cases.json
@@ -3868,6 +3868,10 @@
     "pref_mappings": [{ "pref": "browser.shared_clipboard_enabled" }]
   },
 
+  "AudioSandboxEnabled": {
+    "note": "This policy is used directly through the policy service instead of through a pref."
+  },
+
   "----- Chrome OS device policies ---------------------------------------": {},
 
   "DevicePolicyRefreshRate": {

diff --git a/components/policy/resources/policy_templates.json b/components/policy/resources/policy_templates.json
@@ -18345,6 +18345,27 @@
       After it is enabled by default, administrators who need more time to upgrade affected proxies may use this policy to temporarily disable this security feature. This policy will be removed after version 85.
       '''
     },
+    {
+      'name': 'AudioSandboxEnabled',
+      'owners': ['services/audio/OWNERS'],
+      'type': 'main',
+      'schema': { 'type': 'boolean' },
+      'supported_on': ['chrome.win:79-', 'chrome.linux:79-', 'chrome.mac:79-'],
+      'features': {
+        'dynamic_refresh': False,
+        'per_profile': False,
+      },
+      'example_value': True,
+      'id': 627,
+      'caption': '''Allow the audio sandbox to run''',
+      'tags': ['system-security'],
+      'desc': '''This policy controls the audio process sandboxed.
+      If this policy is enabled, the audio process will run sandboxed.
+      If this policy is disabled, the audio process will run unsandboxed and the WebRTC audio-processing module will run in the renderer process.
+      This leaves users open to security risks related to running the audio stack unsandboxed.
+      If this policy is not set, the default configuration for the audio sandbox will be used, which may differ per platform.
+      This policy is intended to give enterprises flexibility to disable the audio sandbox if they use security software setups that interfere with the sandbox.'''
+    }
   ],
 
   'messages': {
@@ -19165,6 +19186,6 @@ The recommended way to configure policy on Windows is via GPO, although provisio
   ],
   'placeholders': [],
   'deleted_policy_ids': [412, 546, 562, 569],
-  'highest_id_currently_used': 626,
+  'highest_id_currently_used': 627,
   'highest_atomic_group_id_currently_used': 38
 }
diff --git a/content/browser/media/media_internals.cc b/content/browser/media/media_internals.cc
@@ -41,6 +41,7 @@
 #include "media/webrtc/webrtc_switches.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/service_manager/sandbox/features.h"
+#include "services/service_manager/sandbox/sandbox_type.h"
 
 #if !defined(OS_ANDROID)
 #include "media/filters/decrypting_video_decoder.h"
@@ -429,6 +430,11 @@ void MediaInternals::SendGeneralAudioInformation() {
                                                           : "Disabled"));
   };
 
+  auto set_explicit_feature_data = [&](auto& feature, bool feature_value) {
+    audio_info_data.SetKey(feature.name,
+                           base::Value(feature_value ? "Enabled" : "Disabled"));
+  };
+
   set_feature_data(features::kAudioServiceAudioStreams);
   set_feature_data(features::kAudioServiceOutOfProcess);
 
@@ -448,8 +454,10 @@ void MediaInternals::SendGeneralAudioInformation() {
                          base::Value(feature_value_string));
 
   set_feature_data(features::kAudioServiceLaunchOnStartup);
-  set_feature_data(service_manager::features::kAudioServiceSandbox);
-  set_feature_data(features::kWebRtcApmInAudioService);
+  set_explicit_feature_data(service_manager::features::kAudioServiceSandbox,
+                            service_manager::IsAudioSandboxEnabled());
+  set_explicit_feature_data(features::kWebRtcApmInAudioService,
+                            media::IsWebRtcApmInAudioServiceEnabled());
 
   base::string16 audio_info_update =
       SerializeUpdate("media.updateGeneralAudioInformation", &audio_info_data);

diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
@@ -3034,8 +3034,9 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
     switches::kEnableWebGLImageChromium,
     switches::kEnableWebVR,
     switches::kFileUrlPathAlias,
-    switches::kForceDisplayColorProfile,
     switches::kForceDeviceScaleFactor,
+    switches::kForceDisableWebRtcApmInAudioService,
+    switches::kForceDisplayColorProfile,
     switches::kForceGpuMemAvailableMb,
     switches::kForceGpuRasterization,
     switches::kForceHighContrast,

diff --git a/content/browser/utility_process_host.cc b/content/browser/utility_process_host.cc
@@ -426,6 +426,7 @@ bool UtilityProcessHost::StartProcess() {
       switches::kAudioServiceQuitTimeoutMs,
       switches::kDisableAudioOutput,
       switches::kFailAudioStreamCreation,
+      switches::kForceDisableWebRtcApmInAudioService,
       switches::kMuteAudio,
       switches::kUseFileForFakeAudioCapture,
       switches::kAgcStartupMinVolume,

diff --git a/content/utility/utility_service_factory.cc b/content/utility/utility_service_factory.cc
@@ -47,6 +47,7 @@
 #include "base/mac/mach_logging.h"
 #include "sandbox/mac/system_services.h"
 #include "services/service_manager/sandbox/features.h"
+#include "services/service_manager/sandbox/sandbox_type.h"
 #endif
 
 #if defined(OS_WIN)
@@ -199,8 +200,7 @@ UtilityServiceFactory::CreateAudioService(
 #if defined(OS_MACOSX)
   // Don't connect to launch services when running sandboxed
   // (https://crbug.com/874785).
-  if (base::FeatureList::IsEnabled(
-          service_manager::features::kAudioServiceSandbox)) {
+  if (service_manager::IsAudioSandboxEnabled()) {
     sandbox::DisableLaunchServices();
   }
 

diff --git a/media/webrtc/webrtc_switches.cc b/media/webrtc/webrtc_switches.cc
@@ -4,6 +4,9 @@
 
 #include "media/webrtc/webrtc_switches.h"
 
+#include "base/command_line.h"
+#include "build/build_config.h"
+
 namespace switches {
 
 // Override the default minimum starting volume of the Automatic Gain Control
@@ -29,3 +32,25 @@ const base::Feature kWebRtcHybridAgc{"WebRtcHybridAgc",
                                      base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace features
+
+namespace switches {
+
+const char kForceDisableWebRtcApmInAudioService[] =
+    "disable-webrtc-apm-in-audio-service";
+
+}  // namespace switches
+
+namespace media {
+
+bool IsWebRtcApmInAudioServiceEnabled() {
+#if defined(OS_WIN) || defined(OS_MACOSX) || \
+    (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+  return base::FeatureList::IsEnabled(features::kWebRtcApmInAudioService) &&
+         !base::CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kForceDisableWebRtcApmInAudioService);
+#else
+  return false;
+#endif
+}
+
+}  // namespace media
diff --git a/media/webrtc/webrtc_switches.h b/media/webrtc/webrtc_switches.h
@@ -25,4 +25,15 @@ extern const base::Feature kWebRtcHybridAgc;
 
 }  // namespace features
 
+namespace switches {
+COMPONENT_EXPORT(MEDIA_WEBRTC)
+extern const char kForceDisableWebRtcApmInAudioService[];
+}  // namespace switches
+
+namespace media {
+
+COMPONENT_EXPORT(MEDIA_WEBRTC) bool IsWebRtcApmInAudioServiceEnabled();
+
+}  // namespace media
+
 #endif  // MEDIA_WEBRTC_WEBRTC_SWITCHES_H_
diff --git a/services/audio/input_controller.cc b/services/audio/input_controller.cc
@@ -96,10 +96,6 @@ float AveragePower(const media::AudioBus& buffer) {
 
 #if defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
 
-bool CanRunApm() {
-  return base::FeatureList::IsEnabled(features::kWebRtcApmInAudioService);
-}
-
 bool SamplesNeedClamping(const media::AudioBus& bus) {
   const auto IsOutOfRange = [](float sample) {
     // See comment in CopySamplesWithClamping() for why the conditional is
@@ -411,7 +407,8 @@ InputController::InputController(
 
 #if defined(AUDIO_PROCESSING_IN_AUDIO_SERVICE)
   if (processing_config_) {
-    if (processing_config_->settings.requires_apm() && CanRunApm()) {
+    if (processing_config_->settings.requires_apm() &&
+        media::IsWebRtcApmInAudioServiceEnabled()) {
       processing_helper_.emplace(
           params, processing_config_->settings,
           std::move(processing_config_->controls_receiver));