Improve logging, if we fail due to an internal error when executing supportsSeccompSandbox(). Previously, we would just report that the sandbox is unavailable. That's undesirable behavior, because it would lead the caller to think that they should continue without the sandbox. A simple bug in the sandbox compiler could thus result in us inadvertently disabling sandboxing for all users -- without necessarily noticing this issue for a while.

BUG=130662
TEST=make && ./demo32 && ./demo64

Review URL: https://chromiumcodereview.appspot.com/10545100

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@141938 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 65 insertions, 19 deletions

diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index 56931d1..5f7d028 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -31,6 +31,10 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
       sigprocmask(SIG_BLOCK, &newMask, &oldMask)) {
     die("sigprocmask() failed");
   }
+  int fds[2];
+  if (pipe2(fds, O_NONBLOCK|O_CLOEXEC)) {
+    die("pipe() failed");
+  }
 
   pid_t pid = fork();
   if (pid < 0) {
@@ -48,18 +52,28 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
   if (!pid) {
     // Test a very simple sandbox policy to verify that we can
     // successfully turn on sandboxing.
-    suppressLogging_ = true;
-    evaluators_.clear();
-    setSandboxPolicy(probeEvaluator, NULL);
-    setProcFd(proc_fd);
-    startSandbox();
-    if (syscall(__NR_getpid) < 0 && errno == EPERM) {
-      syscall(__NR_exit_group, (intptr_t)100);
+    dryRun_ = true;
+    if (HANDLE_EINTR(close(fds[0])) ||
+        dup2(fds[1], 2) != 2 ||
+        HANDLE_EINTR(close(fds[1]))) {
+      static const char msg[] = "Failed to set up stderr\n";
+      (void)HANDLE_EINTR(write(fds[1], msg, sizeof(msg)-1));
+    } else {
+      evaluators_.clear();
+      setSandboxPolicy(probeEvaluator, NULL);
+      setProcFd(proc_fd);
+      startSandbox();
+      if (syscall(__NR_getpid) < 0 && errno == EPERM) {
+        syscall(__NR_exit_group, (intptr_t)100);
+      }
     }
     die(NULL);
   }
 
   // In the parent process
+  if (HANDLE_EINTR(close(fds[1]))) {
+    die("close() failed");
+  }
   if (sigprocmask(SIG_SETMASK, &oldMask, NULL)) {
     die("sigprocmask() failed");
   }
@@ -67,7 +81,29 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
   if (HANDLE_EINTR(waitpid(pid, &status, 0)) != pid) {
     die("waitpid() failed unexpectedly");
   }
-  return WIFEXITED(status) && WEXITSTATUS(status) == 100;
+  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == 100;
+
+  // 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 most 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) {
+      while (len > 1 && buf[len-1] == '\n') {
+        --len;
+      }
+      buf[len] = '\000';
+      die(buf);
+    }
+  }
+  if (HANDLE_EINTR(close(fds[0]))) {
+    die("close() failed");
+  }
+
+  return rc;
 }
 
 Sandbox::SandboxStatus Sandbox::supportsSeccompSandbox(int proc_fd) {
@@ -312,9 +348,12 @@ void Sandbox::installFilter() {
   delete program;
 
   // Install BPF filter program
-  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
-      prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-    goto filter_failed;
+  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+    die(dryRun_ ? NULL : "Kernel refuses to enable no-new-privs");
+  } else {
+    if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
+      die(dryRun_ ? NULL : "Kernel refuses to turn on BPF filters");
+    }
   }
 
   return;
@@ -351,7 +390,7 @@ void Sandbox::sigSys(int nr, siginfo_t *info, void *void_context) {
 }
 
 
-bool Sandbox::suppressLogging_          = false;
+bool Sandbox::dryRun_                   = false;
 Sandbox::SandboxStatus Sandbox::status_ = STATUS_UNKNOWN;
 int    Sandbox::proc_fd_                = -1;
 std::vector<std::pair<Sandbox::EvaluateSyscall,
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.h b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
index 82e4656..9a50798 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.h
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
@@ -190,12 +190,9 @@ class Sandbox {
  protected:
   // Print an error message and terminate the program. Used for fatal errors.
   static void die(const char *msg) __attribute__((noreturn)) {
-    if (!suppressLogging_) {
-      if (msg) {
-#ifdef SECCOMP_BPF_STANDALONE
-        HANDLE_EINTR(write(2, msg, strlen(msg)));
-        HANDLE_EINTR(write(2, "\n", 1));
-#else
+    if (msg) {
+#ifndef SECCOMP_BPF_STANDALONE
+      if (!dryRun_) {
         // LOG(FATAL) is not neccessarily async-signal safe. It would be
         // better to always use the code for the SECCOMP_BPF_STANDALONE case.
         // But that prevents the logging and reporting infrastructure from
@@ -204,7 +201,17 @@ class Sandbox {
         // LOG(FATAL). In the long run, we probably want to rewrite this code
         // to be async-signal safe.
         LOG(FATAL) << msg;
+      } else
 #endif
+      {
+        // If there is no logging infrastructure in place, we just write error
+        // messages to stderr.
+        // We also write to stderr, if we are called in a child process from
+        // supportsSeccompSandbox(). This makes sure we can actually do the
+        // correct logging from the parent process, which is more likely to
+        // have access to logging infrastructure.
+        HANDLE_EINTR(write(2, msg, strlen(msg)));
+        HANDLE_EINTR(write(2, "\n", 1));
       }
     }
     for (;;) {
@@ -233,7 +240,7 @@ class Sandbox {
   static void      installFilter();
   static void      sigSys(int nr, siginfo_t *info, void *void_context);
 
-  static bool          suppressLogging_;
+  static bool          dryRun_;
   static SandboxStatus status_;
   static int           proc_fd_;
   static std::vector<std::pair<EvaluateSyscall,