Allow using the namespace sandbox in zygote host.

content/zygote/zygote_main_linux.cc

Index: content/zygote/zygote_main_linux.cc
diff --git a/content/zygote/zygote_main_linux.cc b/content/zygote/zygote_main_linux.cc
index 8f89580d42f6ed7a97b86e0e8651acf0905675a5..2cfdd8efc8d5913e7fcb8418bcabe16212ab6588 100644
--- a/content/zygote/zygote_main_linux.cc
+++ b/content/zygote/zygote_main_linux.cc
@@ -39,8 +39,10 @@
 #include "content/public/common/zygote_fork_delegate_linux.h"
 #include "content/zygote/zygote_linux.h"
 #include "crypto/nss_util.h"
+#include "sandbox/linux/services/credentials.h"
 #include "sandbox/linux/services/init_process_reaper.h"
 #include "sandbox/linux/services/libc_urandom_override.h"
+#include "sandbox/linux/services/namespace_sandbox.h"
 #include "sandbox/linux/suid/client/setuid_sandbox_client.h"
 #include "third_party/icu/source/i18n/unicode/timezone.h"
 #include "third_party/skia/include/ports/SkFontConfigInterface.h"
@@ -399,6 +401,41 @@ static bool CreateInitProcessReaper(base::Closure* post_fork_parent_callback) {
   return true;
 }
 
+static bool MaybeSetProcessNonDumpable() {
+  // Previously, we required that the binary be non-readable. This causes the

[jln (very slow on Chromium), 2015/02/06 02:17:04]

You could remove this ancient comment as part of the cleanup.

[rickyz (no longer on Chrome), 2015/02/06 03:01:02]

Done.

+  // kernel to mark the process as non-dumpable at startup. The thinking was
+  // that, although we were putting the renderers into a PID namespace (with
+  // the SUID sandbox), they would nonetheless be in the /same/ PID
+  // namespace. So they could ptrace each other unless they were non-dumpable.
+  //
+  // If the binary was readable, then there would be a window between process
+  // startup and the point where we set the non-dumpable flag in which a
+  // compromised renderer could ptrace attach.
+  //
+  // However, now that we have a zygote model, only the (trusted) zygote
+  // exists at this point and we can set the non-dumpable flag which is
+  // inherited by all our renderer children.
+  //
+  // Note: a non-dumpable process can't be debugged. To debug sandbox-related
+  // issues, one can specify --allow-sandbox-debugging to let the process be
+  // dumpable.
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+  if (command_line.HasSwitch(switches::kAllowSandboxDebugging)) {
+    // If sandbox debugging is allowed, install a handler for sandbox-related
+    // crash testing.
+    InstallSandboxCrashTestHandler();
+    return true;
+  }
+
+  if (prctl(PR_SET_DUMPABLE, 0) != 0) {
+    PLOG(ERROR) << "Failed to set non-dumpable flag";
+    return false;
+  }
+
+  return prctl(PR_GET_DUMPABLE) == 0;
+}
+
 // Enter the setuid sandbox. This requires the current process to have been
 // created through the setuid sandbox.
 static bool EnterSuidSandbox(sandbox::SetuidSandboxClient* setuid_sandbox,
@@ -433,41 +470,27 @@ static bool EnterSuidSandbox(sandbox::SetuidSandboxClient* setuid_sandbox,
     CHECK(CreateInitProcessReaper(post_fork_parent_callback));
   }
 
-#if !defined(OS_OPENBSD)
-  // Previously, we required that the binary be non-readable. This causes the
-  // kernel to mark the process as non-dumpable at startup. The thinking was
-  // that, although we were putting the renderers into a PID namespace (with
-  // the SUID sandbox), they would nonetheless be in the /same/ PID
-  // namespace. So they could ptrace each other unless they were non-dumpable.
-  //
-  // If the binary was readable, then there would be a window between process
-  // startup and the point where we set the non-dumpable flag in which a
-  // compromised renderer could ptrace attach.
-  //
-  // However, now that we have a zygote model, only the (trusted) zygote
-  // exists at this point and we can set the non-dumpable flag which is
-  // inherited by all our renderer children.
-  //
-  // Note: a non-dumpable process can't be debugged. To debug sandbox-related
-  // issues, one can specify --allow-sandbox-debugging to let the process be
-  // dumpable.
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (!command_line.HasSwitch(switches::kAllowSandboxDebugging)) {
-    prctl(PR_SET_DUMPABLE, 0, 0, 0, 0);
-    if (prctl(PR_GET_DUMPABLE, 0, 0, 0, 0)) {
-      LOG(ERROR) << "Failed to set non-dumpable flag";
-      return false;
-    }
-  } else {
-    // If sandbox debugging is allowed, install a handler for sandbox-related
-    // crash testing.
-    InstallSandboxCrashTestHandler();
+  CHECK(MaybeSetProcessNonDumpable());
+  return true;
+}
+
+static void EnterNamespaceSandbox(base::Closure* post_fork_parent_callback) {
+  pid_t pid = getpid();
+  if (sandbox::NamespaceSandbox::InNewPidNamespace()) {
+    CHECK_EQ(1, pid);
   }
 
-#endif
+  CHECK(sandbox::Credentials::MoveToNewUserNS());
+  CHECK(sandbox::Credentials::DropFileSystemAccess());
+  CHECK(sandbox::Credentials::DropAllCapabilities());
 
-  return true;
+  // This needs to happen after moving to a new user NS, since doing so involves
+  // writing the UID/GID map.

[jln (very slow on Chromium), 2015/02/06 02:17:04]

That shouldn't be the case. What's happening?

[rickyz (no longer on Chrome), 2015/02/06 03:01:02]

I think we're hitting the normal uid/gid checks, since prctl(PR_SET_DUMPABLE, 0)
makes the file owned by root. Opening it for read works, on the other hand. You
can see the behavior with a program like this:

#include <err.h>
#include <sys/prctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main(int argc, char **argv) {
  if (prctl(PR_SET_DUMPABLE, 0) != 0) {
    err(1, "prctl");
  }

  printf("/proc/%d/uid_map\n", getpid());
  getchar();

  if (open("/proc/self/uid_map", O_WRONLY) == -1) {
    err(1, "open");
  }
}

+  CHECK(MaybeSetProcessNonDumpable());
+
+  if (pid == 1) {
+    CHECK(CreateInitProcessReaper(post_fork_parent_callback));
+  }
 }
 
 #if defined(ADDRESS_SANITIZER)
@@ -526,10 +549,7 @@ static pid_t ForkSanitizerCoverageHelper(
 
 #endif  // defined(ADDRESS_SANITIZER)
 
-// If |is_suid_sandbox_child|, then make sure that the setuid sandbox is
-// engaged.
 static void EnterLayerOneSandbox(LinuxSandbox* linux_sandbox,
-                                 bool is_suid_sandbox_child,
                                  base::Closure* post_fork_parent_callback) {
   DCHECK(linux_sandbox);
 
@@ -542,10 +562,11 @@ static void EnterLayerOneSandbox(LinuxSandbox* linux_sandbox,
 
   sandbox::SetuidSandboxClient* setuid_sandbox =
       linux_sandbox->setuid_sandbox_client();
-
-  if (is_suid_sandbox_child) {
+  if (setuid_sandbox->IsSuidSandboxChild()) {
     CHECK(EnterSuidSandbox(setuid_sandbox, post_fork_parent_callback))
         << "Failed to enter setuid sandbox";
+  } else if (sandbox::NamespaceSandbox::InNewUserNamespace()) {
+    EnterNamespaceSandbox(post_fork_parent_callback);
   }
 }
 
@@ -583,9 +604,12 @@ bool ZygoteMain(const MainFunctionParams& params,
     linux_sandbox->PreinitializeSandbox();
   }
 
-  const bool must_enable_setuid_sandbox =
+  const bool using_setuid_sandbox =
       linux_sandbox->setuid_sandbox_client()->IsSuidSandboxChild();
-  if (must_enable_setuid_sandbox) {
+  const bool using_namespace_sandbox =
+      sandbox::NamespaceSandbox::InNewUserNamespace();
+
+  if (using_setuid_sandbox) {
     linux_sandbox->setuid_sandbox_client()->CloseDummyFile();
 
     // Let the ZygoteHost know we're booting up.
@@ -597,10 +621,10 @@ bool ZygoteMain(const MainFunctionParams& params,
 
   VLOG(1) << "ZygoteMain: initializing " << fork_delegates.size()
           << " fork delegates";
-  for (ScopedVector<ZygoteForkDelegate>::iterator i = fork_delegates.begin();
-       i != fork_delegates.end();
-       ++i) {
-    (*i)->Init(GetSandboxFD(), must_enable_setuid_sandbox);
+  const bool using_layer1_sandbox =
+      using_setuid_sandbox || using_namespace_sandbox;
+  for (ZygoteForkDelegate* fork_delegate : fork_delegates) {
+    fork_delegate->Init(GetSandboxFD(), using_layer1_sandbox);
   }
 
   const std::vector<int> sandbox_fds_to_close_post_fork =
@@ -613,8 +637,7 @@ bool ZygoteMain(const MainFunctionParams& params,
       base::Bind(&CloseFds, fds_to_close_post_fork);
 
   // Turn on the first layer of the sandbox if the configuration warrants it.
-  EnterLayerOneSandbox(linux_sandbox, must_enable_setuid_sandbox,

[jln (very slow on Chromium), 2015/02/06 02:17:04]

The reason for passing this flag (rather than relying on the function to requery
the environment) was to have a very tight bond between what happens (due to
must_enable_setuid_sandbox being set) and what we report as the status (see the
check line 641).

I kind of liked this design and I think I would prefer to keep these variables
rather than relying on the environment to not change, but it's a detail and I
don't want to delay your CL further.

[rickyz (no longer on Chrome), 2015/02/06 03:01:02]

Yeah, I'm not a fan of the environment stuff either. I readded the variable with
a check - eventually it'd be nice to have these functions be inside a class
which is aware of what kind of sandbox we are using.

-                       &post_fork_parent_callback);
+  EnterLayerOneSandbox(linux_sandbox, &post_fork_parent_callback);
 
   // Extra children and file descriptors created that the Zygote must have
   // knowledge of.
@@ -638,7 +661,7 @@ bool ZygoteMain(const MainFunctionParams& params,
 
   int sandbox_flags = linux_sandbox->GetStatus();
   bool setuid_sandbox_engaged = sandbox_flags & kSandboxLinuxSUID;
-  CHECK_EQ(must_enable_setuid_sandbox, setuid_sandbox_engaged);
+  CHECK_EQ(using_setuid_sandbox, setuid_sandbox_engaged);

[jln (very slow on Chromium), 2015/02/06 02:17:04]

Want do add the same check for the namespace sandbox?

[rickyz (no longer on Chrome), 2015/02/06 03:01:02]

Once the about:sandbox change goes in, yes, though even with that, we would
effectively be doing a CHECK_EQ(sandbox::NamespaceSandbox::InNewUserNamespace(),
sandbox::NamespaceSandbox::InNewUserNamespace())

 
   Zygote zygote(sandbox_flags, fork_delegates.Pass(), extra_children,
                 extra_fds);