Plumb sandbox rules through the helper executable.

chrome/app/chrome_exe_main_mac.cc

Index: chrome/app/chrome_exe_main_mac.cc
diff --git a/chrome/app/chrome_exe_main_mac.cc b/chrome/app/chrome_exe_main_mac.cc
new file mode 100644
index 0000000000000000000000000000000000000000..680cd169d3065b3e50599ef0808bc3a22febdb8c
--- /dev/null
+++ b/chrome/app/chrome_exe_main_mac.cc
@@ -0,0 +1,164 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// The entry point for all Mac Chromium processes, including the outer app
+// bundle (browser) and helper app (renderer, plugin, and friends).
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <libgen.h>
+#include <mach-o/dyld.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <vector>
+
+#include "chrome/common/chrome_version.h"
+
+#if defined(HELPER_EXECUTABLE)
+#include "sandbox/mac/seatbelt_exec.h"
+#endif  // defined(HELPER_EXECUTABLE)
+
+namespace {
+
+typedef int (*ChromeMainPtr)(int, char**);
+
+#if defined(HELPER_EXECUTABLE)
+// The name of the parameter containing the executable path.
+constexpr char exec_param[] = "EXECUTABLE_PATH";

[Mark Mentovai, 2017/05/22 18:53:18]

WHY ARE WE YELLING?

[Greg K, 2017/05/22 23:17:55]

All sandbox parameters are written in uppercase, so this continues that
convention.

+// The name of the parameter containing the PID of Chrome.
+constexpr char pid_param[] = "CHROMIUM_PID";
+// The command line parameter to engage the v2 sandbox.
+constexpr char v2_sandbox_arg[] = "--v2-sandbox";
+// The command line parameter for the file descriptor used to receive the
+// sandbox policy.
+constexpr char fd_mapping_arg[] = "--fd_mapping";
+
+int SandboxExec(int argc, char* argv[], int fd_mapping) {
+  char rp[MAXPATHLEN];
+  if (realpath(argv[0], rp) == NULL)
+    abort();
+
+  sandbox::SeatbeltExecServer server(fd_mapping);
+
+  if (!server.SetParameter(exec_param, rp) ||
+      !server.SetParameter(pid_param, std::to_string(getpid()))) {

[Mark Mentovai, 2017/05/22 18:53:17]

#include <string>

[Greg K, 2017/05/22 23:17:55]

Done.

+    fprintf(stderr, "Failed to setup parameters for sandbox.\n");

[Mark Mentovai, 2017/05/22 18:53:18]

“set up”

[Greg K, 2017/05/22 23:17:55]

Done.

+    return -1;
+  }
+
+  if (server.InitializeSandbox() != 0)
+    abort();
+
+  std::vector<char*> new_argv;
+  for (int i = 1; i < argc; ++i) {
+    if (strcmp(argv[i], v2_sandbox_arg) != 0 &&
+        strncmp(argv[i], fd_mapping_arg, strlen(fd_mapping_arg)) != 0) {

[Mark Mentovai, 2017/05/22 18:53:18]

Shouldn’t you be looking for “--fd_mapping=”?

[Greg K, 2017/05/22 23:17:55]

Done.

+      new_argv.push_back(argv[i]);
+    }
+  }
+  new_argv.push_back(nullptr);
+
+  // The helper executable re-executes itself under the sandbox.
+  execve(argv[0], new_argv.data(), nullptr);

[Mark Mentovai, 2017/05/22 18:53:18]

Why is envp nullptr?

[Greg K, 2017/05/22 23:17:55]

Good catch, it needs to pass the current environment through.

+  perror("execve");
+  return 1;
+}
+#endif  // defined(HELPER_EXECUTABLE)
+
+}  // namespace
+
+__attribute__((visibility("default"))) int main(int argc, char* argv[]) {
+#if defined(HELPER_EXECUTABLE)
+  bool enable_v2_sandbox = false;
+  int fd_mapping = -1;
+  for (int i = 1; i < argc; i++) {
+    if (strcmp(argv[i], v2_sandbox_arg) == 0) {
+      enable_v2_sandbox = true;
+    } else if (strncmp(argv[i], fd_mapping_arg, strlen(fd_mapping_arg)) == 0) {
+      // Parse --fd_mapping=X to get the file descriptor X.
+      std::string arg(argv[i]);
+      std::string fd_str = arg.substr(arg.find("=") + 1, arg.length());
+      fd_mapping = std::stoi(fd_str);

[Mark Mentovai, 2017/05/22 18:53:18]

This piece of junk will return 0 if fd_str isn’t really a number, which you’ll
happily treat as a real file descriptor. (And it is a real file descriptor.)

[Greg K, 2017/05/22 23:17:55]

libc++abi.dylib: terminating with uncaught exception of type
std::invalid_argument: stoi: no conversion

The behavior I read and saw is that it throws an exception. Is that not always
true?

+    }
+  }
+  if (enable_v2_sandbox && fd_mapping == -1) {
+    fprintf(stderr, "Must pass a valid file descriptor to --fd_mapping.\n");
+    return -1;
+  }
+
+  if (enable_v2_sandbox)
+    return SandboxExec(argc, argv, fd_mapping);
+
+  const char* const rel_path =
+      "../../../" PRODUCT_FULLNAME_STRING
+      " Framework.framework/" PRODUCT_FULLNAME_STRING " Framework";
+#else
+  const char* const rel_path =
+      "../Versions/" CHROME_VERSION_STRING "/" PRODUCT_FULLNAME_STRING
+      " Framework.framework/" PRODUCT_FULLNAME_STRING " Framework";
+#endif  // defined(HELPER_EXECUTABLE)
+
+  uint32_t exec_path_size = 0;
+  int rv = _NSGetExecutablePath(NULL, &exec_path_size);
+  if (rv != -1) {
+    fprintf(stderr, "_NSGetExecutablePath: get length failed\n");
+    abort();
+  }
+
+  char* exec_path = new char[exec_path_size];
+  if (!exec_path) {

[Greg K, 2017/05/22 18:34:03]

I know Chrome doesn't use exceptions, but the underlying C++ new[] here and
elsewhere in this file will throw an exception, which will be unhandled and
terminate execution, rendering this check not needed, right?

[Mark Mentovai, 2017/05/22 18:53:17]

Right.

+    fprintf(stderr, "new %u failed.\n", exec_path_size);
+    abort();
+  }
+
+  rv = _NSGetExecutablePath(exec_path, &exec_path_size);
+  if (rv != 0) {
+    fprintf(stderr, "_NSGetExecutablePath: get path failed\n");
+    abort();
+  }
+
+  // Slice off the last part of the main executable path, and append the
+  // version framework information.
+  const char* parent_dir = dirname(exec_path);
+  if (!parent_dir) {
+    fprintf(stderr, "dirname %s: %s\n", exec_path, strerror(errno));
+    abort();
+  }
+  delete[] exec_path;
+
+  const size_t parent_path_len = strlen(parent_dir);
+  const size_t rel_path_len = strlen(rel_path);
+  // 2 accounts for a trailing NUL byte and the '/' in the middle of the paths.
+  const size_t framework_path_size = parent_path_len + rel_path_len + 2;
+  char* framework_path = new char[framework_path_size];
+  if (!framework_path) {
+    fprintf(stderr, "new %zu failed.\n", framework_path_size);
+    abort();
+  }
+  snprintf(framework_path, framework_path_size, "%s/%s", parent_dir, rel_path);

[Greg K, 2017/05/22 18:34:03]

Unless there's a good reason to prefer C style format strings, should I replace
this with std::string concatenation?

[Mark Mentovai, 2017/05/22 18:53:18]

Greg K wrote:
Whatever you prefer.

[Greg K, 2017/05/22 23:17:55]

Acknowledged.

+
+  void* library = dlopen(framework_path, RTLD_LAZY | RTLD_LOCAL | RTLD_FIRST);
+  if (!library) {
+    fprintf(stderr, "dlopen %s: %s\n", framework_path, dlerror());
+    abort();
+  }
+  delete[] framework_path;
+
+  const ChromeMainPtr chrome_main =
+      reinterpret_cast<ChromeMainPtr>(dlsym(library, "ChromeMain"));
+  if (!chrome_main) {
+    fprintf(stderr, "dlsym ChromeMain: %s\n", dlerror());
+    abort();
+  }
+  rv = chrome_main(argc, argv);
+
+  // exit, don't return from main, to avoid the apparent removal of main from
+  // stack backtraces under tail call optimization.
+  exit(rv);
+}