Move FileUtilService to kService on Windows

Sandbox::kFileUtilService only exists so that we can pin user32 early in utility_main for this service. This CL moves FileUtilService to use the new WithPinUser32 service process host option, and deletes the now unused sandbox type. LaunchFileUtilService is moved inside a helper class to allow access to the passkeys needed without a complex replication of the launching function's signature in service_process_host_passkeys.h. Tests: browser_tests Bug: 1435571,1408988 Change-Id: Iafdde12b482f1fe8413670baad33444dfbb2c6af Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4534184 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Commit-Queue: Alex Gough <ajgo@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/main@{#1146976}
chromium · May 21, 2023 · 71f4a4b · 71f4a4b
1 parent 7f63957
commit 71f4a4b
Show file tree

Hide file tree

Showing 12 changed files with 34 additions and 46 deletions.
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -4603,7 +4603,6 @@ std::wstring ChromeContentBrowserClient::GetAppContainerSidForSandboxType(
     case sandbox::mojom::Sandbox::kIconReader:
     case sandbox::mojom::Sandbox::kMediaFoundationCdm:
     case sandbox::mojom::Sandbox::kWindowsSystemProxyResolver:
-    case sandbox::mojom::Sandbox::kFileUtil:
       // Should never reach here.
       CHECK(0);
       return std::wstring();
@@ -4693,7 +4692,6 @@ bool ChromeContentBrowserClient::PreSpawnChild(
     case sandbox::mojom::Sandbox::kIconReader:
     case sandbox::mojom::Sandbox::kMediaFoundationCdm:
     case sandbox::mojom::Sandbox::kWindowsSystemProxyResolver:
-    case sandbox::mojom::Sandbox::kFileUtil:
       break;
   }
 

diff --git a/chrome/browser/file_util_service.cc b/chrome/browser/file_util_service.cc
@@ -4,17 +4,36 @@
 
 #include "chrome/browser/file_util_service.h"
 
+#include "build/build_config.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/services/file_util/public/mojom/file_util_service.mojom.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/service_process_host.h"
+#include "content/public/browser/service_process_host_passkeys.h"
+
+namespace chrome {
+// Class allows us to friend the passkeys we need to launch the service.
+class FileUtilServiceLauncher {
+ public:
+  static mojo::PendingRemote<chrome::mojom::FileUtilService>
+  LaunchFileUtilService() {
+    mojo::PendingRemote<chrome::mojom::FileUtilService> remote;
+    content::ServiceProcessHost::Launch<chrome::mojom::FileUtilService>(
+        remote.InitWithNewPipeAndPassReceiver(),
+        content::ServiceProcessHost::Options()
+#if BUILDFLAG(IS_WIN)
+            // The FileUtilService supports archive inspection, which uses unrar
+            // for inspecting rar archives. Unrar depends on user32.dll for
+            // handling upper/lowercase.
+            .WithPinUser32(content::ServiceProcessHostPinUser32::GetPassKey())
+#endif  // BUILDFLAG(IS_WIN)
+            .WithDisplayName(IDS_UTILITY_PROCESS_FILE_UTILITY_NAME)
+            .Pass());
+    return remote;
+  }
+};
+}  // namespace chrome
 
 mojo::PendingRemote<chrome::mojom::FileUtilService> LaunchFileUtilService() {
-  mojo::PendingRemote<chrome::mojom::FileUtilService> remote;
-  content::ServiceProcessHost::Launch<chrome::mojom::FileUtilService>(
-      remote.InitWithNewPipeAndPassReceiver(),
-      content::ServiceProcessHost::Options()
-          .WithDisplayName(IDS_UTILITY_PROCESS_FILE_UTILITY_NAME)
-          .Pass());
-  return remote;
+  return chrome::FileUtilServiceLauncher::LaunchFileUtilService();
 }
diff --git a/chrome/services/file_util/public/mojom/file_util_service.mojom b/chrome/services/file_util/public/mojom/file_util_service.mojom
@@ -15,16 +15,9 @@ import "chrome/services/file_util/public/mojom/zip_file_creator.mojom";
 [EnableIf=extractors]
 import "chrome/services/file_util/public/mojom/single_file_extractor.mojom";
 
-// On Windows we need specific extra DLLs that are provided by the kFileUtil
-// sandbox.
-[EnableIf=is_win]
-const sandbox.mojom.Sandbox kFileUtilSandbox = sandbox.mojom.Sandbox.kFileUtil;
-[EnableIfNot=is_win]
-const sandbox.mojom.Sandbox kFileUtilSandbox = sandbox.mojom.Sandbox.kService;
-
 // The main interface to the file utility service. Binds any of various
 // specific utility receivers.
-[ServiceSandbox=kFileUtilSandbox]
+[ServiceSandbox=sandbox.mojom.Sandbox.kService]
 interface FileUtilService {
   // Binds an instance of the ZipFileCreator interface.
   [EnableIf=is_chromeos_ash]

diff --git a/content/browser/utility_sandbox_delegate.cc b/content/browser/utility_sandbox_delegate.cc
@@ -47,7 +47,6 @@ UtilitySandboxedProcessLauncherDelegate::
       sandbox_type_ == sandbox::mojom::Sandbox::kIconReader ||
       sandbox_type_ == sandbox::mojom::Sandbox::kMediaFoundationCdm ||
       sandbox_type_ == sandbox::mojom::Sandbox::kWindowsSystemProxyResolver ||
-      sandbox_type_ == sandbox::mojom::Sandbox::kFileUtil ||
 #endif
 #if BUILDFLAG(IS_MAC)
       sandbox_type_ == sandbox::mojom::Sandbox::kMirroring ||

diff --git a/content/browser/utility_sandbox_delegate_win.cc b/content/browser/utility_sandbox_delegate_win.cc
@@ -354,16 +354,14 @@ bool UtilitySandboxedProcessLauncherDelegate::InitializeConfig(
   }
 
   if (sandbox_type_ == sandbox::mojom::Sandbox::kService ||
-      sandbox_type_ == sandbox::mojom::Sandbox::kServiceWithJit ||
-      sandbox_type_ == sandbox::mojom::Sandbox::kFileUtil) {
+      sandbox_type_ == sandbox::mojom::Sandbox::kServiceWithJit) {
     auto result = sandbox::policy::SandboxWin::AddWin32kLockdownPolicy(config);
     if (result != sandbox::SBOX_ALL_OK) {
       return false;
     }
   }
 
-  if (sandbox_type_ == sandbox::mojom::Sandbox::kService ||
-      sandbox_type_ == sandbox::mojom::Sandbox::kFileUtil) {
+  if (sandbox_type_ == sandbox::mojom::Sandbox::kService) {
     auto delayed_flags = config->GetDelayedProcessMitigations();
     delayed_flags |= sandbox::MITIGATION_DYNAMIC_CODE_DISABLE;
     auto result = config->SetDelayedProcessMitigations(delayed_flags);

diff --git a/content/public/browser/service_process_host_passkeys.h b/content/public/browser/service_process_host_passkeys.h
@@ -12,6 +12,10 @@ namespace screen_ai {
 class ScreenAIServiceRouter;
 }  // namespace screen_ai
 
+namespace chrome {
+class FileUtilServiceLauncher;
+}  // namespace chrome
+
 namespace content {
 class ServiceProcessHostPreloadLibraries {
  public:
@@ -44,6 +48,7 @@ class ServiceProcessHostPinUser32 {
 
   // Service launchers using `ServiceProcessHost::Options::WithPinUser32`
   // should be added here and must be reviewed by the security team.
+  friend class chrome::FileUtilServiceLauncher;
 
   // Tests.
   FRIEND_TEST_ALL_PREFIXES(ServiceProcessHostBrowserTest, PinUser32);

diff --git a/content/utility/utility_main.cc b/content/utility/utility_main.cc
@@ -371,13 +371,6 @@ int UtilityMain(MainFunctionParams parameters) {
     base::win::EnableHighDPISupport();
   }
 
-  // The FileUtilService supports archive inspection, which uses unrar for
-  // inspecting rar archives. Unrar depends on user32.dll for handling
-  // upper/lowercase.
-  if (sandbox_type == sandbox::mojom::Sandbox::kFileUtil) {
-    base::win::PinUser32();
-  }
-
   if (!sandbox::policy::IsUnsandboxedSandboxType(sandbox_type) &&
       sandbox_type != sandbox::mojom::Sandbox::kCdm &&
       sandbox_type != sandbox::mojom::Sandbox::kMediaFoundationCdm &&

diff --git a/sandbox/policy/mojom/sandbox.mojom b/sandbox/policy/mojom/sandbox.mojom
@@ -72,10 +72,6 @@ enum Sandbox {
   [EnableIf=is_fuchsia]
   kVideoCapture,
 
-  // Used to preload the appropriate DLLs for archive analysis within a sandbox.
-  [EnableIf=is_win]
-  kFileUtil,
-
   // Allows access to file contents and Windows APIs for parsing icons from PE
   // files.
   [EnableIf=is_win]

diff --git a/sandbox/policy/sandbox_type.cc b/sandbox/policy/sandbox_type.cc
@@ -34,7 +34,6 @@ bool IsUnsandboxedSandboxType(Sandbox sandbox_type) {
     case Sandbox::kIconReader:
     case Sandbox::kMediaFoundationCdm:
     case Sandbox::kWindowsSystemProxyResolver:
-    case Sandbox::kFileUtil:
       return false;
 #endif
     case Sandbox::kAudio:
@@ -138,7 +137,6 @@ void SetCommandLineFlagsForSandboxType(base::CommandLine* command_line,
     case Sandbox::kIconReader:
     case Sandbox::kMediaFoundationCdm:
     case Sandbox::kWindowsSystemProxyResolver:
-    case Sandbox::kFileUtil:
 #endif  // BUILDFLAG(IS_WIN)
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_ASH)
     case Sandbox::kHardwareVideoDecoding:
@@ -310,10 +308,6 @@ std::string StringFromUtilitySandboxType(Sandbox sandbox_type) {
       return switches::kLibassistantSandbox;
 #endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
-#if BUILDFLAG(IS_WIN)
-    case Sandbox::kFileUtil:
-      return switches::kFileUtilSandbox;
-#endif
       // The following are not utility processes so should not occur.
     case Sandbox::kRenderer:
     case Sandbox::kGpu:
@@ -375,9 +369,6 @@ sandbox::mojom::Sandbox UtilitySandboxTypeFromString(
     return Sandbox::kMediaFoundationCdm;
   if (sandbox_string == switches::kWindowsSystemProxyResolverSandbox)
     return Sandbox::kWindowsSystemProxyResolver;
-  if (sandbox_string == switches::kFileUtilSandbox) {
-    return Sandbox::kFileUtil;
-  }
 #endif
 #if BUILDFLAG(IS_MAC)
   if (sandbox_string == switches::kMirroringSandbox)

diff --git a/sandbox/policy/switches.cc b/sandbox/policy/switches.cc
@@ -47,7 +47,6 @@ const char kXrCompositingSandbox[] = "xr_compositing";
 const char kIconReaderSandbox[] = "icon_reader";
 const char kMediaFoundationCdmSandbox[] = "mf_cdm";
 const char kWindowsSystemProxyResolverSandbox[] = "proxy_resolver_win";
-const char kFileUtilSandbox[] = "file_util";
 #endif  // BUILDFLAG(IS_WIN)
 
 #if BUILDFLAG(IS_MAC)

diff --git a/sandbox/policy/switches.h b/sandbox/policy/switches.h
@@ -50,7 +50,6 @@ SANDBOX_POLICY_EXPORT extern const char kXrCompositingSandbox[];
 SANDBOX_POLICY_EXPORT extern const char kIconReaderSandbox[];
 SANDBOX_POLICY_EXPORT extern const char kMediaFoundationCdmSandbox[];
 SANDBOX_POLICY_EXPORT extern const char kWindowsSystemProxyResolverSandbox[];
-SANDBOX_POLICY_EXPORT extern const char kFileUtilSandbox[];
 #endif  // BUILDFLAG(IS_WIN)
 
 #if BUILDFLAG(IS_MAC)

diff --git a/sandbox/policy/win/sandbox_win.cc b/sandbox/policy/win/sandbox_win.cc
@@ -1084,8 +1084,6 @@ std::string SandboxWin::GetSandboxTypeInEnglish(Sandbox sandbox_type) {
       return "Icon Reader";
     case Sandbox::kWindowsSystemProxyResolver:
       return "Windows System Proxy Resolver";
-    case Sandbox::kFileUtil:
-      return "File Util";
   }
 }