Seccomp-BPF: relax failure in probe process setup

sandbox/linux/seccomp-bpf/sandbox_bpf.cc

 // Copyright (c) 2012 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.
 
 #include "sandbox/linux/seccomp-bpf/codegen.h"
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
 #include "sandbox/linux/seccomp-bpf/syscall_iterator.h"
 #include "sandbox/linux/seccomp-bpf/verifier.h"
 
+namespace {
+
+void WriteFailedStderrSetupMessage(int out_fd) {
+  const char* error_string = strerror(errno);
+  static const char msg[] = "Failed to set up stderr: ";
+  if (HANDLE_EINTR(write(out_fd, msg, sizeof(msg)-1)) > 0 && error_string &&
+      HANDLE_EINTR(write(out_fd, error_string, strlen(error_string))) > 0 &&
+      HANDLE_EINTR(write(out_fd, "\n", 1))) {

[Markus (顧孟勤), 2012/10/26 23:14:11]

After this change, do we still expect to see these messages in some place? If
so, should we consider logging a lot more data?

We could log the output from fstat(2) (we should probably collect this data
before trying to close stderr).

We could log the file descriptors returned by pipe() and we could log whether
they seem to be functional (e.g. do another fstat())

Instead of doing just a dup2(2), we could first attempt a close(2) and log it's
result.

There are probably a few other things that might be interesting but that I can't
think of right now.

In either case, since you can now call WriteFailedStderrSetupMessage() from
multiple locations, I would strongly suggest you add some data to disambiguate
why we returning this message. That could make debugging much easier.

[jln (very slow on Chromium), 2012/10/29 21:15:37]

That makes sense. But let's land a simple, easy to merge patch first, and we can
have something more audacious and add some logging information on the dev
channel afterwards. What do you think ?

+  }
+}
+
+}  // namespace
+
 // The kernel gives us a sandbox, we turn it into a playground :-)
 // This is version 2 of the playground; version 1 was built on top of
 // pre-BPF seccomp mode.
 namespace playground2 {
 
+const int kExpectedExitCode = 100;
+
 // We define a really simple sandbox policy. It is just good enough for us
 // to tell that the sandbox has actually been activated.
 ErrorCode Sandbox::probeEvaluator(int signo) {
   switch (signo) {
   case __NR_getpid:
     // Return EPERM so that we can check that the filter actually ran.
     return ErrorCode(EPERM);
   case __NR_exit_group:
     // Allow exit() with a non-default return code.
     return ErrorCode(ErrorCode::ERR_ALLOWED);
   default:
     // Make everything else fail in an easily recognizable way.
     return ErrorCode(EINVAL);
   }
 }
 
 void Sandbox::probeProcess(void) {
   if (syscall(__NR_getpid) < 0 && errno == EPERM) {
-    syscall(__NR_exit_group, (intptr_t)100);
+    syscall(__NR_exit_group, static_cast<intptr_t>(kExpectedExitCode));
   }
 }
 
 bool Sandbox::isValidSyscallNumber(int sysnum) {
   return SyscallIterator::IsValid(sysnum);
 }
 
 ErrorCode Sandbox::allowAllEvaluator(int sysnum) {
   if (!isValidSyscallNumber(sysnum)) {
     return ErrorCode(ENOSYS);
   }
   return ErrorCode(ErrorCode::ERR_ALLOWED);
 }
 
 void Sandbox::tryVsyscallProcess(void) {
   time_t current_time;
   // time() is implemented as a vsyscall. With an older glibc, with
   // vsyscall=emulate and some versions of the seccomp BPF patch
   // we may get SIGKILL-ed. Detect this!
   if (time(&current_time) != static_cast<time_t>(-1)) {
-    syscall(__NR_exit_group, (intptr_t)100);
+    syscall(__NR_exit_group, static_cast<intptr_t>(kExpectedExitCode));
   }
 }
 
 bool Sandbox::RunFunctionInPolicy(void (*CodeInSandbox)(),
                                   EvaluateSyscall syscallEvaluator,
                                   int proc_fd) {
   // Block all signals before forking a child process. This prevents an
   // attacker from manipulating our test by sending us an unexpected signal.
   sigset_t oldMask, newMask;
   if (sigfillset(&newMask) ||
       sigprocmask(SIG_BLOCK, &newMask, &oldMask)) {
     SANDBOX_DIE("sigprocmask() failed");
   }
   int fds[2];
   if (pipe2(fds, O_NONBLOCK|O_CLOEXEC)) {
     SANDBOX_DIE("pipe() failed");
   }
 
+  if (fds[0] <= 2 || fds[1] <= 2) {
+    SANDBOX_DIE("Process started without standard file descriptors");
+  }
+
   pid_t pid = fork();
   if (pid < 0) {
     // Die if we cannot fork(). We would probably fail a little later
     // anyway, as the machine is likely very close to running out of
     // memory.
     // But what we don't want to do is return "false", as a crafty
     // attacker might cause fork() to fail at will and could trick us
     // into running without a sandbox.
     sigprocmask(SIG_SETMASK, &oldMask, NULL);  // OK, if it fails
     SANDBOX_DIE("fork() failed unexpectedly");
   }
 
   // In the child process
   if (!pid) {
     // Test a very simple sandbox policy to verify that we can
     // successfully turn on sandboxing.
     Die::EnableSimpleExit();
-    errno = 0;
-    if (HANDLE_EINTR(close(fds[0])) ||
-        HANDLE_EINTR(dup2(fds[1], 2)) != 2 ||
-        HANDLE_EINTR(close(fds[1]))) {
-      const char* error_string = strerror(errno);
-      static const char msg[] = "Failed to set up stderr: ";
-      if (HANDLE_EINTR(write(fds[1], msg, sizeof(msg)-1)) > 0 && error_string &&
-          HANDLE_EINTR(write(fds[1], error_string, strlen(error_string))) > 0 &&
-          HANDLE_EINTR(write(fds[1], "\n", 1))) {
-      }
-    } else {
-      evaluators_.clear();
-      setSandboxPolicy(syscallEvaluator, NULL);
-      setProcFd(proc_fd);
 
-      // By passing "quiet=true" to "startSandboxInternal()" we suppress
-      // messages for expected and benign failures (e.g. if the current
-      // kernel lacks support for BPF filters).
-      startSandboxInternal(true);
+    if (HANDLE_EINTR(close(fds[0]))) {
+      WriteFailedStderrSetupMessage(fds[1]);
+      SANDBOX_DIE(NULL);
+    }
+    if (HANDLE_EINTR(dup2(fds[1], 2)) != 2) {

[Markus (顧孟勤), 2012/10/26 23:14:11]

Try calling HANDLE_EINTR(close(2)) first and ignore the exit code. That seems
harmless and might just be good enough to make dup2() happy. Who knows.

+      // Stderr could very well be a file descriptor to .xsession-errors, or
+      // another file, which could be backed by a file system that could cause
+      // dup2 to fail while trying to close stderr. It's important that we do
+      // not fail on trying to close stderr.
+      // If dup2 fails here, we will continue normally, this means that our
+      // parent won't cause a fatal failure if something writes to stderr in
+      // this child.

[Markus (顧孟勤), 2012/10/26 23:14:11]

Do you still want this to be fatal in debug builds?

[jln (very slow on Chromium), 2012/10/29 21:15:37]

Yeah, the first iteration of this patch, as you can see on the review tool),
made it so, but this should wait for a refactor of sandbox_bpf.cc into a clean
interface, because we're not even including proper header files here (and are
relying on the .h file to do it instead).

Since the way it works is to check for NDEBUG (for non debug), I was scared that
we may have a bug of NDEBUG not being defined not because we are in a debug
build, but just because something strange happened with our headers.

+    }
+    if (HANDLE_EINTR(close(fds[1]))) {
+      WriteFailedStderrSetupMessage(fds[1]);
+      SANDBOX_DIE(NULL);
+    }
 
-      // Run our code in the sandbox
-      CodeInSandbox();
-    }
+    evaluators_.clear();
+    setSandboxPolicy(syscallEvaluator, NULL);
+    setProcFd(proc_fd);
+
+    // By passing "quiet=true" to "startSandboxInternal()" we suppress
+    // messages for expected and benign failures (e.g. if the current
+    // kernel lacks support for BPF filters).
+    startSandboxInternal(true);
+
+    // Run our code in the sandbox.
+    CodeInSandbox();

[Markus (顧孟勤), 2012/10/26 23:14:11]

I'd prefer a blank line before the comment, as I feel it is easier to read
quickly. But it's your call, if you prefer the denser code without extra blank
lines.

[jln (very slow on Chromium), 2012/10/29 21:15:37]

Done.

+    // CodeInSandbox() is not supposed to return here.
     SANDBOX_DIE(NULL);
   }
 
   // In the parent process.
   if (HANDLE_EINTR(close(fds[1]))) {
     SANDBOX_DIE("close() failed");
   }
   if (sigprocmask(SIG_SETMASK, &oldMask, NULL)) {
     SANDBOX_DIE("sigprocmask() failed");
   }
   int status;
   if (HANDLE_EINTR(waitpid(pid, &status, 0)) != pid) {
     SANDBOX_DIE("waitpid() failed unexpectedly");
   }
-  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == 100;
+  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == kExpectedExitCode;
 
   // If we fail to support sandboxing, there might be an additional
   // error message. If so, this was an entirely unexpected and fatal
   // failure. We should report the failure and somebody must fix
   // things. This is probably a security-critical bug in the sandboxing
   // code.
   if (!rc) {
     char buf[4096];
     ssize_t len = HANDLE_EINTR(read(fds[0], buf, sizeof(buf) - 1));
     if (len > 0) {
@@ skipping 478 matching lines @@
 Sandbox::SandboxStatus Sandbox::status_ = STATUS_UNKNOWN;
 int    Sandbox::proc_fd_                = -1;
 Sandbox::Evaluators Sandbox::evaluators_;
 Sandbox::ErrMap Sandbox::errMap_;
 Sandbox::Traps *Sandbox::traps_         = NULL;
 Sandbox::TrapIds Sandbox::trapIds_;
 ErrorCode *Sandbox::trapArray_          = NULL;
 size_t Sandbox::trapArraySize_          = 0;
 
 }  // namespace