Reland "Experiment for enabling ExtensionPointDisablePolicy in browser process"

This relands commit d5556d4690.

Changes in reland: moved the STATUS_OBJECT_NAME_NOT_FOUND DCHECK under
the right if condition, fixing debug test break

Bug: 557798
Change-Id: I3f7565b05c1f678987755269e65f8fc39d5b3eab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3009979
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Stefan Smolen <ssmole@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#900680}

chrome/browser/win/chrome_elf_init.cc

@@ -115,25 +115,30 @@ void InitializeChromeElf() {
     BrowserBlacklistBeaconSetup();
   }
 
-  // Make sure the early finch emergency "off switch" for
+  // Make sure the registry key we read earlier in startup
   // sandbox::MITIGATION_EXTENSION_POINT_DISABLE is set properly in reg.
   // Note: the very existence of this key signals elf to not enable
   // this mitigation on browser next start.
-  const std::wstring finch_path(install_static::GetRegistryPath().append(
-      elf_sec::kRegSecurityFinchKeyName));
-  base::win::RegKey finch_security_registry_key(HKEY_CURRENT_USER,
-                                                finch_path.c_str(), KEY_READ);
+  const std::wstring reg_path(install_static::GetRegistryPath().append(
+      elf_sec::kRegBrowserExtensionPointKeyName));
+  base::win::RegKey browser_extension_point_registry_key(
+      HKEY_CURRENT_USER, reg_path.c_str(), KEY_READ);
 
-  RecordExtensionPointsEnableState(GetExtensionPointsEnableState());
+  ExtensionPointEnableState extension_point_enable_state =
+      GetExtensionPointsEnableState();
+  RecordExtensionPointsEnableState(extension_point_enable_state);
+  bool enable_extension_point_policy =
+      (extension_point_enable_state == EXTENSIONPOINT_ENABLED) &&
+      base::FeatureList::IsEnabled(
+          sandbox::policy::features::kWinSboxDisableExtensionPoints);
 
-  if (base::FeatureList::IsEnabled(
-          sandbox::policy::features::kWinSboxDisableExtensionPoints)) {
-    if (finch_security_registry_key.Valid())
-      finch_security_registry_key.DeleteKey(L"");
-  } else {
-    if (!finch_security_registry_key.Valid()) {
-      finch_security_registry_key.Create(HKEY_CURRENT_USER, finch_path.c_str(),
-                                         KEY_WRITE);
+  if (enable_extension_point_policy) {
+    if (!browser_extension_point_registry_key.Valid()) {
+      browser_extension_point_registry_key.Create(HKEY_CURRENT_USER,
+                                                  reg_path.c_str(), KEY_WRITE);
+    } else {
+      if (browser_extension_point_registry_key.Valid())
+        browser_extension_point_registry_key.DeleteKey(L"");
     }
   }
 }

chrome/chrome_elf/chrome_elf_constants.cc

@@ -17,6 +17,7 @@ const DWORD kBeaconMaxAttempts = 2;
 
 namespace elf_sec {
 
-const wchar_t kRegSecurityFinchKeyName[] = L"\\BrowserSboxFinch";
+const wchar_t kRegBrowserExtensionPointKeyName[] =
+    L"\\BrowserExtensionPointPolicy";
 
 }  // namespace elf_sec

chrome/chrome_elf/chrome_elf_constants.h

@@ -40,9 +40,9 @@ enum BlacklistState {
 
 namespace elf_sec {
 
-// The name of the registry key holding the finch "emergency-off"
-// switch for sandbox::MITIGATION_EXTENSION_POINT_DISABLE.
-extern const wchar_t kRegSecurityFinchKeyName[];
+// The name of the registry key which controls the enablement of
+// sandbox::MITIGATION_EXTENSION_POINT_DISABLE for the browser process.
+extern const wchar_t kRegBrowserExtensionPointKeyName[];
 
 }  // namespace elf_sec
 

chrome/chrome_elf/chrome_elf_main.cc

@@ -7,6 +7,7 @@
 #include <assert.h>
 #include <windows.h>
 
+#include "chrome/chrome_elf/chrome_elf_security.h"
 #include "chrome/chrome_elf/crash/crash_helper.h"
 #include "chrome/chrome_elf/third_party_dlls/beacon.h"
 #include "chrome/chrome_elf/third_party_dlls/main.h"
@@ -64,6 +65,9 @@ BOOL APIENTRY DllMain(HMODULE module, DWORD reason, LPVOID reserved) {
 
     if (install_static::IsBrowserProcess()) {
       __try {
+        // Disable third party extension points.
+        elf_security::EarlyBrowserSecurity();
+
         // Initialize the blocking of third-party DLLs if the initialization of
         // the safety beacon succeeds.
         if (third_party_dlls::LeaveSetupBeacon())

chrome/chrome_elf/chrome_elf_security.cc

@@ -23,21 +23,23 @@ void EarlyBrowserSecurity() {
   NTSTATUS ret_val = STATUS_SUCCESS;
   HANDLE handle = INVALID_HANDLE_VALUE;
 
-  // Check for kRegistrySecurityFinchPath.  If it exists,
-  // we do NOT disable extension points.  (Emergency off flag.)
-  if (nt::OpenRegKey(nt::HKCU,
-                     install_static::GetRegistryPath()
-                         .append(elf_sec::kRegSecurityFinchKeyName)
-                         .c_str(),
-                     KEY_QUERY_VALUE, &handle, &ret_val)) {
-    nt::CloseRegKey(handle);
+  // Check for kRegBrowserExtensionPointKeyName. We only disable extension
+  // points when this exists, for devices that have been vetted by our
+  // heuristic.
+  if (!nt::OpenRegKey(nt::HKCU,
+                      install_static::GetRegistryPath()
+                          .append(elf_sec::kRegBrowserExtensionPointKeyName)
+                          .c_str(),
+                      KEY_QUERY_VALUE, &handle, &ret_val)) {
+#ifdef _DEBUG
+    // The only failure expected is for the path not existing.
+    if (ret_val != STATUS_OBJECT_NAME_NOT_FOUND)
+      assert(false);
+#endif
     return;
   }
-#ifdef _DEBUG
-  // The only failure expected is for the path not existing.
-  if (ret_val != STATUS_OBJECT_NAME_NOT_FOUND)
-    assert(false);
-#endif
+
+  nt::CloseRegKey(handle);
 
   if (::IsWindows8OrGreater()) {
     SetProcessMitigationPolicyFunc set_process_mitigation_policy =

chrome/chrome_elf/chrome_elf_util_unittest.cc

@@ -18,18 +18,18 @@
 
 namespace {
 
-bool SetSecurityFinchFlag(bool creation) {
+bool SetExtensionPointEnabledFlag(bool creation) {
   bool success = true;
-  const std::wstring finch_path(install_static::GetRegistryPath().append(
-      elf_sec::kRegSecurityFinchKeyName));
+  const std::wstring reg_path(install_static::GetRegistryPath().append(
+      elf_sec::kRegBrowserExtensionPointKeyName));
   base::win::RegKey security_key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
 
   if (creation) {
     if (ERROR_SUCCESS !=
-        security_key.CreateKey(finch_path.c_str(), KEY_QUERY_VALUE))
+        security_key.CreateKey(reg_path.c_str(), KEY_QUERY_VALUE))
       success = false;
   } else {
-    if (ERROR_SUCCESS != security_key.DeleteKey(finch_path.c_str()))
+    if (ERROR_SUCCESS != security_key.DeleteKey(reg_path.c_str()))
       success = false;
   }
 
@@ -92,13 +92,12 @@ TEST(ChromeElfUtilTest, BrowserProcessSecurityTest) {
   registry_util::RegistryOverrideManager override_manager;
   ASSERT_NO_FATAL_FAILURE(RegRedirect(nt::HKCU, &override_manager));
 
-  // First, ensure that the emergency-off finch signal works.
-  EXPECT_TRUE(SetSecurityFinchFlag(true));
+  // First, ensure that the policy is not applied without the reg key.
   elf_security::EarlyBrowserSecurity();
   EXPECT_FALSE(IsSecuritySet());
-  EXPECT_TRUE(SetSecurityFinchFlag(false));
+  EXPECT_TRUE(SetExtensionPointEnabledFlag(true));
 
-  // Second, test that the process mitigation is set when no finch signal.
+  // Second, test that the process mitigation is set when the reg key exists.
   elf_security::EarlyBrowserSecurity();
   EXPECT_TRUE(IsSecuritySet());
 

sandbox/policy/features.cc

@@ -24,10 +24,10 @@ const base::Feature kNetworkServiceSandbox{"NetworkServiceSandbox",
 const base::Feature kWinSboxDisableKtmComponent{
     "WinSboxDisableKtmComponent", base::FEATURE_ENABLED_BY_DEFAULT};
 
-// Emergency "off switch" for new Windows sandbox security mitigation,
+// Experiment for Windows sandbox security mitigation,
 // sandbox::MITIGATION_EXTENSION_POINT_DISABLE.
 const base::Feature kWinSboxDisableExtensionPoints{
-    "WinSboxDisableExtensionPoint", base::FEATURE_ENABLED_BY_DEFAULT};
+    "WinSboxDisableExtensionPoint", base::FEATURE_DISABLED_BY_DEFAULT};
 
 // Enables GPU AppContainer sandbox on Windows.
 const base::Feature kGpuAppContainer{"GpuAppContainer",