Reland "Remove the pipe lockdown feature and mark it enabled by defau…

…lt." This is a reland of commit 1f545d6 PS2 contains the only change, which is to avoid an unused variable `result` in sandbox_win.cc by scoping it for use in each of the two #if clauses. Original change's description: > Remove the pipe lockdown feature and mark it enabled by default. > > This code shipped in m109 and hasn't been shown to cause any > issues, so the feature can be enabled by default and the > code to handle the disabled case can be completely removed. > > BUG=1378724, 1378339 > > Change-Id: Ie6d7cda507c8400c3d794c98042bd0befebf23ee > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4252457 > Reviewed-by: Alex Gough <ajgo@chromium.org> > Commit-Queue: Will Harris <wfh@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1105875} Bug: 1378724, 1378339 Change-Id: If285e1a13b036f10c5669727028cdeae6e15f678 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4257554 Reviewed-by: Alex Gough <ajgo@chromium.org> Commit-Queue: Will Harris <wfh@chromium.org> Cr-Commit-Position: refs/heads/main@{#1106000}
qweri0p · Feb 16, 2023 · 7efef62 · 7efef62
1 parent 302c006
commit 7efef62
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 90 deletions.
diff --git a/content/browser/ppapi_plugin_sandboxed_process_launcher_delegate.cc b/content/browser/ppapi_plugin_sandboxed_process_launcher_delegate.cc
@@ -12,7 +12,6 @@
 #include "sandbox/policy/mojom/sandbox.mojom.h"
 
 #if BUILDFLAG(IS_WIN)
-#include "sandbox/policy/features.h"
 #include "sandbox/policy/win/sandbox_win.h"
 #include "sandbox/win/src/process_mitigations.h"
 #include "sandbox/win/src/sandbox_policy.h"
@@ -49,17 +48,6 @@ bool PpapiPluginSandboxedProcessLauncherDelegate::PreSpawnTarget(
   if (sandbox::SBOX_ALL_OK != config->SetDelayedProcessMitigations(flags))
     return false;
 
-  if (base::FeatureList::IsEnabled(
-          sandbox::policy::features::kChromePipeLockdown)) {
-    return true;
-  }
-
-  result = config->AddRule(sandbox::SubSystem::kNamedPipes,
-                           sandbox::Semantics::kNamedPipesAllowAny,
-                           L"\\\\.\\pipe\\chrome.*");
-  if (result != sandbox::SBOX_ALL_OK)
-    return false;
-
   return true;
 }
 #endif  // BUILDFLAG(IS_WIN)

diff --git a/content/browser/renderer_host/render_process_host_sandbox_policy_win_browsertest.cc b/content/browser/renderer_host/render_process_host_sandbox_policy_win_browsertest.cc
@@ -42,32 +42,22 @@ IN_PROC_BROWSER_TEST_F(RendererAppContainerFeatureBrowserTest, Navigate) {
 }
 
 // Test class to verify the behavior of the pipe interceptions for renderers.
-class PipeLockdownFeatureBrowserTest
-    : public ContentBrowserTest,
-      public ::testing::WithParamInterface</* Pipe Lockdown Enabled */ bool> {
+class PipeLockdownFeatureBrowserTest : public ContentBrowserTest {
  public:
-  PipeLockdownFeatureBrowserTest() {
-    scoped_feature_list_.InitWithFeatureState(
-        sandbox::policy::features::kChromePipeLockdown, PipeLockdownEnabled());
-  }
+  PipeLockdownFeatureBrowserTest() = default;
 
   void SetUpOnMainThread() override {
     // Support multiple sites on the test server.
     host_resolver()->AddRule("*", "127.0.0.1");
   }
 
  protected:
-  bool PipeLockdownEnabled() const { return GetParam(); }
-
   WebContentsImpl* contents() const {
     return static_cast<WebContentsImpl*>(shell()->web_contents());
   }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-IN_PROC_BROWSER_TEST_P(PipeLockdownFeatureBrowserTest, Navigate) {
+IN_PROC_BROWSER_TEST_F(PipeLockdownFeatureBrowserTest, Navigate) {
   ASSERT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("foo.com", "/title1.html")));
@@ -138,24 +128,9 @@ IN_PROC_BROWSER_TEST_P(PipeLockdownFeatureBrowserTest, Navigate) {
   // There should never be a way to Create pipes for chrome.*.
   EXPECT_FALSE(found_chrome_pipe_create_pipe_rule);
 
-  if (PipeLockdownEnabled()) {
-    // With pipe lockdown enabled, no pipe rules should exist for renderers.
-    EXPECT_FALSE(found_chrome_sync_pipe_create_pipe_rule);
-    EXPECT_FALSE(found_chrome_pipe_open_rule);
-  } else {
-    // Old behavior allowed chrome.sync.* to be created (for base::SyncSocket).
-    EXPECT_TRUE(found_chrome_sync_pipe_create_pipe_rule);
-    // Old behavior allowed chrome.* to be opened (for legacy IPC).
-    EXPECT_TRUE(found_chrome_pipe_open_rule);
-  }
+  // With pipe lockdown enabled, no pipe rules should exist for renderers.
+  EXPECT_FALSE(found_chrome_sync_pipe_create_pipe_rule);
+  EXPECT_FALSE(found_chrome_pipe_open_rule);
 }
 
-INSTANTIATE_TEST_SUITE_P(Enabled,
-                         PipeLockdownFeatureBrowserTest,
-                         /* Pipe Lockdown Enabled */ ::testing::Values(true));
-
-INSTANTIATE_TEST_SUITE_P(Disabled,
-                         PipeLockdownFeatureBrowserTest,
-                         /* Pipe Lockdown Enabled */ ::testing::Values(false));
-
 }  // namespace content
diff --git a/content/browser/utility_sandbox_delegate_win.cc b/content/browser/utility_sandbox_delegate_win.cc
@@ -15,7 +15,6 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "printing/buildflags/buildflags.h"
-#include "sandbox/policy/features.h"
 #include "sandbox/policy/mojom/sandbox.mojom.h"
 #include "sandbox/policy/win/sandbox_win.h"
 #include "sandbox/win/src/app_container.h"
@@ -65,15 +64,13 @@ bool AudioPreSpawnTarget(sandbox::TargetConfig* config) {
   if (result != sandbox::SBOX_ALL_OK)
     return false;
 
-  if (base::FeatureList::IsEnabled(
-          sandbox::policy::features::kChromePipeLockdown)) {
-    // The Audio Service process uses a base::SyncSocket for transmitting audio
-    // data.
-    result = config->AddRule(sandbox::SubSystem::kNamedPipes,
-                             sandbox::Semantics::kNamedPipesAllowAny,
-                             L"\\\\.\\pipe\\chrome.sync.*");
-    if (result != sandbox::SBOX_ALL_OK)
-      return false;
+  // The Audio Service process uses a base::SyncSocket for transmitting audio
+  // data.
+  result = config->AddRule(sandbox::SubSystem::kNamedPipes,
+                           sandbox::Semantics::kNamedPipesAllowAny,
+                           L"\\\\.\\pipe\\chrome.sync.*");
+  if (result != sandbox::SBOX_ALL_OK) {
+    return false;
   }
 
   config->SetDesktop(sandbox::Desktop::kAlternateWinstation);

diff --git a/sandbox/policy/features.cc b/sandbox/policy/features.cc
@@ -45,14 +45,6 @@ BASE_FEATURE(kSharedSandboxPolicies,
              "SharedSandboxPolicies",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
-// Emergency "off switch" for pipe security changes, which apply more
-// restrictions to sandboxed processes from opening or creating pipes. This
-// feature can be removed around the M112 timeline. See
-// https://crbug.com/1378724.
-BASE_FEATURE(kChromePipeLockdown,
-             "ChromePipeLockdown",
-             base::FEATURE_ENABLED_BY_DEFAULT);
-
 // Emergency "off switch" for renderer environment filtering, this feature can
 // be removed around the M113 timeline. See https://crbug.com/1403087.
 BASE_FEATURE(kRendererFilterEnvironment,

diff --git a/sandbox/policy/features.h b/sandbox/policy/features.h
@@ -25,7 +25,6 @@ SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kGpuAppContainer);
 SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kGpuLPAC);
 SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kRendererAppContainer);
 SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kSharedSandboxPolicies);
-SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kChromePipeLockdown);
 SANDBOX_POLICY_EXPORT BASE_DECLARE_FEATURE(kRendererFilterEnvironment);
 #endif  // BUILDFLAG(IS_WIN)
 

diff --git a/sandbox/policy/win/sandbox_win.cc b/sandbox/policy/win/sandbox_win.cc
@@ -265,37 +265,21 @@ std::wstring PrependWindowsSessionPath(const wchar_t* object) {
 // Adds the generic config rules to a sandbox TargetConfig.
 ResultCode AddGenericConfig(sandbox::TargetConfig* config) {
   DCHECK(!config->IsConfigured());
-  ResultCode result;
-
-  if (!base::FeatureList::IsEnabled(
-          sandbox::policy::features::kChromePipeLockdown)) {
-    // Add the policy for the client side of a pipe. It is just a file
-    // in the \pipe\ namespace. We restrict it to pipes that start with
-    // "chrome." so the sandboxed process cannot connect to system services.
-    result = config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowAny,
-                             L"\\??\\pipe\\chrome.*");
-    if (result != SBOX_ALL_OK)
-      return result;
-
-    // Allow the server side of sync sockets, which are pipes that have
-    // the "chrome.sync" namespace and a randomly generated suffix.
-    result =
-        config->AddRule(SubSystem::kNamedPipes, Semantics::kNamedPipesAllowAny,
-                        L"\\\\.\\pipe\\chrome.sync.*");
-    if (result != SBOX_ALL_OK)
-      return result;
-  }
 
 // Add the policy for read-only PDB file access for stack traces.
 #if !defined(OFFICIAL_BUILD)
   base::FilePath exe;
   if (!base::PathService::Get(base::FILE_EXE, &exe))
     return SBOX_ERROR_GENERIC;
   base::FilePath pdb_path = exe.DirName().Append(L"*.pdb");
-  result = config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowReadonly,
-                           pdb_path.value().c_str());
-  if (result != SBOX_ALL_OK)
-    return result;
+  {
+    ResultCode result =
+        config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowReadonly,
+                        pdb_path.value().c_str());
+    if (result != SBOX_ALL_OK) {
+      return result;
+    }
+  }
 #endif
 
 #if defined(SANITIZER_COVERAGE)
@@ -312,10 +296,14 @@ ResultCode AddGenericConfig(sandbox::TargetConfig* config) {
     CHECK(coverage_dir.size() == coverage_dir_size);
     base::FilePath sancov_path =
         base::FilePath(coverage_dir).Append(L"*.sancov");
-    result = config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowAny,
-                             sancov_path.value().c_str());
-    if (result != SBOX_ALL_OK)
-      return result;
+    {
+      ResultCode result =
+          config->AddRule(SubSystem::kFiles, Semantics::kFilesAllowAny,
+                          sancov_path.value().c_str());
+      if (result != SBOX_ALL_OK) {
+        return result;
+      }
+    }
   }
 #endif