Linux: use sandbox binary alongside chrome.

content/browser/browser_main_loop.cc

Index: content/browser/browser_main_loop.cc
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
index d95dbb594142047be0deb447367a014e2279b908..12b8b79629494f183bcb26fd4a3becc3cebd180c 100644
--- a/content/browser/browser_main_loop.cc
+++ b/content/browser/browser_main_loop.cc
@@ -11,6 +11,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/path_service.h"
 #include "base/pending_task.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/process/process_metrics.h"
@@ -117,19 +118,24 @@ void SetupSandbox(const CommandLine& parsed_command_line) {
   TRACE_EVENT0("startup", "SetupSandbox");
   // TODO(evanm): move this into SandboxWrapper; I'm just trying to move this
   // code en masse out of chrome_main for now.
-  const char* sandbox_binary = NULL;
+  base::FilePath sandbox_binary;
+  bool env_chrome_devel_sandbox_set = false;
   struct stat st;
 
   // In Chromium branded builds, developers can set an environment variable to

[jln (very slow on Chromium), 2013/07/30 02:03:59]

The comment is misleading (not your fault).

Could you change it to something more clear, such as "In user-managed builds,
including development builds an environment variable ... is required ..."

[Paweł Hajdan Jr., 2013/07/30 19:12:33]

Done.

   // use the development sandbox. See
   // http://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment
-  if (stat(base::kProcSelfExe, &st) == 0 && st.st_uid == getuid())
-    sandbox_binary = getenv("CHROME_DEVEL_SANDBOX");
-
-#if defined(LINUX_SANDBOX_PATH)
-  if (!sandbox_binary)
-    sandbox_binary = LINUX_SANDBOX_PATH;
-#endif
+  if (stat(base::kProcSelfExe, &st) == 0 && st.st_uid == getuid()) {
+    const char* devel_sandbox_path = getenv("CHROME_DEVEL_SANDBOX");
+    if (devel_sandbox_path) {
+      env_chrome_devel_sandbox_set = true;
+      sandbox_binary = base::FilePath(devel_sandbox_path);
+    }
+  } else {
+    base::FilePath exe_dir;

[jln (very slow on Chromium), 2013/07/30 02:03:59]

I think Release Chrome will no longer start as root with this change (unless
CHROME_DEVEL_SANDBOX is set).

I'm not sure what the best solution is.

What do you think about the following:

- First, check if the file exe_dir.AppendASCII("chrome-sandbox"); exists.
- If that's the case, use this as sandbox_binary.
- Otherwise, if (stat()...getuid()) {}

This should work because it's very unlikely that a developer would have
chrome-sandbox in their directory (it should be called chrome_sandbox there).
And if they had it with the wrong permission, we would later crash with
LOG(FATAL) anyways (please give it a manual try).

[Paweł Hajdan Jr., 2013/07/30 19:12:33]

Done.

+    if (PathService::Get(base::DIR_EXE, &exe_dir))
+      sandbox_binary = exe_dir.AppendASCII("chrome-sandbox");
+  }
 
   const bool want_setuid_sandbox =
       !parsed_command_line.HasSwitch(switches::kNoSandbox) &&
@@ -139,26 +145,23 @@ void SetupSandbox(const CommandLine& parsed_command_line) {
     static const char no_suid_error[] = "Running without the SUID sandbox! See "
         "https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment "
         "for more information on developing with the sandbox on.";
-    if (!sandbox_binary) {
-      // This needs to be fatal. Talk to security@chromium.org if you feel
-      // otherwise.
-      LOG(FATAL) << no_suid_error;
-    }
-    // TODO(jln): an empty CHROME_DEVEL_SANDBOX environment variable (as
-    // opposed to a non existing one) is not fatal yet. This is needed because
-    // of existing bots and scripts. Fix it (crbug.com/245376).
-    if (sandbox_binary && *sandbox_binary == '\0')
+    if (sandbox_binary.empty()) {
+      if (!env_chrome_devel_sandbox_set) {
+        // This needs to be fatal. Talk to security@chromium.org if you feel
+        // otherwise.
+        LOG(FATAL) << no_suid_error;
+      }
+
+      // TODO(jln): an empty CHROME_DEVEL_SANDBOX environment variable (as
+      // opposed to a non existing one) is not fatal yet. This is needed
+      // because of existing bots and scripts. Fix it (crbug.com/245376).
       LOG(ERROR) << no_suid_error;
-  }
-
-  std::string sandbox_cmd;
-  if (want_setuid_sandbox && sandbox_binary) {
-    sandbox_cmd = sandbox_binary;
+    }
   }
 
   // Tickle the sandbox host and zygote host so they fork now.
-  RenderSandboxHostLinux::GetInstance()->Init(sandbox_cmd);
-  ZygoteHostImpl::GetInstance()->Init(sandbox_cmd);
+  RenderSandboxHostLinux::GetInstance()->Init(sandbox_binary.value());
+  ZygoteHostImpl::GetInstance()->Init(sandbox_binary.value());
 }
 #endif