Initial snapshot of the new BPF-enabled seccomp sandbox. This code is

sandbox/linux/seccomp_bpf/sandbox_bpf.cc

Index: sandbox/linux/seccomp_bpf/sandbox_bpf.cc
===================================================================
--- sandbox/linux/seccomp_bpf/sandbox_bpf.cc	(revision 0)
+++ sandbox/linux/seccomp_bpf/sandbox_bpf.cc	(revision 0)
@@ -0,0 +1,234 @@
+// 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/sandbox_bpf.h"
+
+// 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 {
+
+
+static Sandbox::ErrorCode probeEvaluator(int signo) {
+  switch (signo) {
+  case __NR_getpid:
+    // Return EPERM so that we can check that the filter actually ran.
+    return (Sandbox::ErrorCode)EPERM;
+  case __NR_exit_group:
+    // Allow exit() with a non-default return code.
+    return Sandbox::SB_ALLOWED;
+  default:
+    // Make everything else fail in an easily recognizable way.
+    return (Sandbox::ErrorCode)EINVAL;
+  }
+}
+
+bool Sandbox::supportsSeccompSandbox(int proc_fd) {
+  if (status_ == STATUS_UNKNOWN) {
+    if (!isSingleThreaded(proc_fd)) {
+      status_ = STATUS_UNSUPPORTED;

[Chris Evans, 2012/06/01 20:07:55]

Should we have a specific status for that? As a user of the API, I'd probably
read "unsupported" to mean the kernel support isn't there, not that my program
(or point of sandbox initialization) can be tweaked to get it working.

[Markus (顧孟勤), 2012/06/01 21:38:52]

I see your point. While this has never come up in practice before, now that we
are putting sandboxes in more places, this could make sense.

I changed tracking of "status_" to be more complete.

+    } else {
+      sigset_t oldMask, newMask;
+      sigfillset(&newMask);

[Chris Evans, 2012/06/01 20:07:55]

Although this realistically isn't going to fail, sigfillset() does have a return
value, so it should be checked.

[Markus (顧孟勤), 2012/06/01 21:38:52]

Done.

+      if (!sigprocmask(SIG_BLOCK, &newMask, &oldMask)) {
+        pid_t pid = fork();

[Chris Evans, 2012/06/01 20:07:55]

I wonder what the overhead is of this check? We'll be doing it for every new
renderer process creation, probably.

[Markus (顧孟勤), 2012/06/01 21:38:52]

That's why the result is cached in status_. You should only ever do it once.

+        if (pid >= 0) {
+          if (!pid) {
+            // Test a very simple sandbox policy to verify that we can
+            // successfully turn on sandboxing.
+            setSandboxPolicy(probeEvaluator, NULL);
+            setProcFd(proc_fd);
+            startSandbox();
+            if (syscall(__NR_getpid) < 0 && errno == EPERM) {
+              syscall(__NR_exit_group, (intptr_t)100);

[Chris Evans, 2012/06/01 20:07:55]

Why use syscall(exit_group) here but _exit() below? Seems inconsistent.

[Markus (顧孟勤), 2012/06/01 21:38:52]

Good call. This was a mistake when I changed the code from what I had before
(using direct calls to prctl()) to using the same infrastructure as when
starting the "real" sandbox.

It probably would still have worked just fine; but the inconsistency makes it
harder to reason about correctness.

+            }
+            for (;;) {

[Chris Evans, 2012/06/01 20:07:55]

Why is the infinite loop needed?

[Markus (顧孟勤), 2012/06/01 21:38:52]

We really absolutely do not want to fall through. Crazy bad things could happen.
Our attempts at exiting the program should work. But since we just installed a
system call filter, there is the possibility that the system call actually
returned. If that's the case, our next best option is to loop. Hopefully,
somebody is going to notice...

+              _exit(1);
+            }
+          }
+          sigprocmask(SIG_SETMASK, &oldMask, NULL);

[Chris Evans, 2012/06/01 20:07:55]

Check return values where possible.

+          int status;
+          HANDLE_EINTR(waitpid(pid, &status, 0));

[Chris Evans, 2012/06/01 20:07:55]

Does this look at status without checking that waitpid returned a non-error code
return?

[Markus (顧孟勤), 2012/06/01 21:38:52]

We really should never see an error here. But you are correct that we should be
checking. For now, I return STATUS_UNAVAILABLE (signalling a temporary failure).
But I could easily be convinced to turn this into a fatal error, as a failure of
this system call is completely unexpected.

+          status_ = WIFEXITED(status) && WEXITSTATUS(status) == 100
+            ? STATUS_AVAILABLE : STATUS_UNSUPPORTED;
+        } else {

[Chris Evans, 2012/06/01 20:07:55]

Is this branch the case that fork() fails? Is it your intention to leave status_
at STATUS_UNSUPPORTED in this case?
Should the fork() failure be logged?

[Markus (顧孟勤), 2012/06/01 21:38:52]

Take a look at the new logic and let me know if you like this better. I think it
addresses your concerns, but maybe I am misunderstanding what you would like
here.

+          sigprocmask(SIG_SETMASK, &oldMask, NULL);
+        }
+      }
+    }
+  }
+  return status_ == STATUS_AVAILABLE;
+}
+
+void Sandbox::setProcFd(int proc_fd) {
+  proc_fd_ = proc_fd;
+}
+
+void Sandbox::startSandbox() {
+  if (status_ == STATUS_UNSUPPORTED) {

[Chris Evans, 2012/06/01 20:07:55]

What should we do for STATUS_UNKNOWN?

[Markus (顧孟勤), 2012/06/01 21:38:52]

STATUS_UNKNOWN is perfectly OK. This would happen if the user decided to call
startSandbox() without first checking whether it is supported.

This is explicitly allowed by the API, even if most users wouldn't want to use
it this way.

I could be persuaded to revisit this design decision, but I am generally
comfortable with leaving it this way. So, you'd have to bring a strong
counter-argument.

+    die("Trying to start sandbox, even though it is known to be unavailable");
+  }
+  if (proc_fd_ < 0) {
+    proc_fd_ = open("/proc", O_RDONLY|O_DIRECTORY);
+  }
+  if (proc_fd_ < 0) {
+    // For now, continue in degraded mode, if we can't access /proc.
+    // In the future, we might want to tighten this requirement.
+  }
+  if (!isSingleThreaded(proc_fd_)) {
+    die("Cannot start sandbox, if process is already multi-threaded");
+  }
+
+  // We no longer need access to any files in /proc. We want to do this
+  // before installing the filters, just in case that our policy denies
+  // close().
+  if (proc_fd_ >= 0) {
+    if (HANDLE_EINTR(close(proc_fd_))) {
+      die("Failed to close file descriptor for /proc");
+    }
+    proc_fd_ = -1;
+  }
+
+  // Install the filters.
+  installFilter();
+}
+
+bool Sandbox::isSingleThreaded(int proc_fd) {
+  if (proc_fd < 0) {
+    // Cannot determine whether program is single-threaded. Hope for
+    // the best...
+    return true;
+  }
+
+  struct stat sb;
+  int task = -1;
+  if (proc_fd < 0 ||
+      (task = openat(proc_fd, "self/task", O_RDONLY|O_DIRECTORY)) < 0 ||
+      fstat(task, &sb) != 0 ||
+      sb.st_nlink != 3 ||
+      HANDLE_EINTR(close(task))) {
+    if (task >= 0) {
+      HANDLE_EINTR(close(task));
+    }
+    return false;
+  }
+  return true;
+}
+
+void Sandbox::setSandboxPolicy(EvaluateSyscall syscallEvaluator,
+                               EvaluateArguments argumentEvaluator) {
+  evaluators_.push_back(std::make_pair(syscallEvaluator, argumentEvaluator));
+}
+
+void Sandbox::installFilter() {
+  // Verify that the user pushed a policy.
+  if (evaluators_.empty()) {
+  filter_failed:
+    die("Failed to configure system call filters");
+  }
+
+  // Set new SIGSYS handler
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sa.sa_sigaction = &sigSys;
+  sa.sa_flags = SA_SIGINFO;
+  if (sigaction(SIGSYS, &sa, NULL) < 0) {
+    goto filter_failed;
+  }
+
+  // Unmask SIGSYS
+  sigset_t mask;
+  sigemptyset(&mask);
+  sigaddset(&mask, SIGSYS);
+  if (sigprocmask(SIG_UNBLOCK, &mask, NULL)) {
+    goto filter_failed;
+  }
+
+  // We can't handle stacked evaluators, yet. We'll get there eventually
+  // though. Hang tight.
+  if (evaluators_.size() != 1) {
+    die("Not implemented");
+  }
+
+  // If the architecture doesn't match SECCOMP_ARCH, disallow the
+  // system call.
+  std::vector<struct sock_filter> program;
+  program.push_back((struct sock_filter)
+                    BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+                             offsetof(struct arch_seccomp_data, arch)));
+  program.push_back((struct sock_filter)
+    BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, SECCOMP_ARCH, 1, 0));
+  program.push_back((struct sock_filter)
+    BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_DENY));
+
+  // Grab the system call number, so that we can implement jump tables.
+  program.push_back((struct sock_filter)
+    BPF_STMT(BPF_LD+BPF_W+BPF_ABS, offsetof(struct arch_seccomp_data, nr)));
+
+  // Evaluate all possible system calls and depending on their
+  // exit codes generate a BPF filter.
+  // This is very inefficient right now. We need to be much smarter
+  // eventually.

[Jorge Lucangeli Obes, 2012/06/01 03:06:18]

Definitely. In particular, we cannot keep "hot" syscalls at the beginning of the
jump table without a non-trivial change to this section of the code. I.e. looks
like we'll need syscall number iterator or something.

[Chris Evans, 2012/06/01 20:07:55]

Agree with Jorge, checking e.g. syscall 200 will be very expensive! But we can
fix it in a follow-up CL.

BTW, I was very excited about the tree-like comparison thing, to get O(log N)
check time with number of permitted syscalls? Will that make a return?

[Markus (顧孟勤), 2012/06/01 21:38:52]

That's the plan. In my first attempt at writing a BPF compiler, I only used the
binary tree for making yes/no decisions. But any system call that required more
elaborate special-casing (e.g. inspection of arguments) still needed to be
handled by hand-coded linear search.

Having learned from this prototype, I now realize it is actually really easy to
always to a binary search for all decisions. So, we'll get a BPF program that is
close to optimal.

And since we now have a clear way to define what behavior we want the BPF filter
to do, we can also perform a much better run-time check to verify that the
compiled BPF program is in fact correct. That makes me very happy :-) No more
risk of sneaking in subtle bugs because of mistakes in hand-coded recipes.

+  EvaluateSyscall evaluateSyscall = evaluators_.begin()->first;
+  for (int sysnum = MIN_SYSCALL; sysnum <= MAX_SYSCALL; ++sysnum) {
+    ErrorCode err = evaluateSyscall(sysnum);
+    int ret;
+    switch (err) {
+    case SB_INSPECT_ARG_1...SB_INSPECT_ARG_6:
+      die("Not implemented");
+    case SB_TRAP:
+      ret = SECCOMP_RET_TRAP;
+      break;
+    case SB_ALLOWED:
+      ret = SECCOMP_RET_ALLOW;
+      break;
+    default:
+      ret = SECCOMP_RET_ERRNO + err;

[Chris Evans, 2012/06/01 20:07:55]

For paranoid, probably check that "err" is within bounds of what can be handled.
(Check the mask?)

[Markus (顧孟勤), 2012/06/01 21:38:52]

I like how you are thinking :-)

+      break;
+    }
+    program.push_back((struct sock_filter)
+      BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, sysnum, 0, 1));
+    program.push_back((struct sock_filter)
+      BPF_STMT(BPF_RET+BPF_K, ret));
+  }
+
+  // Everything that isn't allowed is forbidden. Eventually, we would
+  // like to have a way to log forbidden calls, when in debug mode.
+  program.push_back((struct sock_filter)
+    BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_DENY));
+
+  // Install BPF filter program
+  const struct sock_fprog prog = { program.size(), &program[0] };
+  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
+      prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
+    goto filter_failed;
+  }
+
+  return;
+}
+
+void Sandbox::sigSys(int nr, siginfo_t *info, void *void_context) {
+  if (info->si_code != SYS_SECCOMP || !void_context) {
+    die("Unexpected SIGSYS received");

[Chris Evans, 2012/06/01 20:07:55]

I'd also check that nr==SIGSYS

[Markus (顧孟勤), 2012/06/01 21:38:52]

Done.

+  }
+  ucontext_t *ctx = reinterpret_cast<ucontext_t *>(void_context);
+  int old_errno   = errno;
+  void *rc        =
+    (void *)(intptr_t)-(int)(SECCOMP_RET_DENY & SECCOMP_RET_DATA);

[Chris Evans, 2012/06/01 20:07:55]

I don't understand that; may be worth a comment?

[Markus (顧孟勤), 2012/06/01 21:38:52]

I added a comment. No idea whether it makes things easier or harder to
understand. The complication arises from the fact that we need to return a
negative integer value sign extended and cast to a void *. C/C++ makes this
difficult and I had subtle bugs in this code before. Each of the casts is
actually necessary.

Also, note that I use C style instead of C++ style casts. I find C++ style casts
hard to read without really contributing much value. But I am usually willing to
bow to the style guide.

This is an exception. With C++ casts, nobody could figure out what this line
does. With C style casts, there is a chance that the intention of the code is
decypherable.

+
+  // This is where we can add extra code to handle complex system calls.
+  // ...
+
+  ctx->uc_mcontext.gregs[REG_RESULT] = reinterpret_cast<greg_t>(rc);
+  errno                              = old_errno;
+  return;
+}
+
+
+Sandbox::SandboxStatus Sandbox::status_ = STATUS_UNKNOWN;
+int    Sandbox::proc_fd_                = -1;
+std::vector<std::pair<Sandbox::EvaluateSyscall,
+                      Sandbox::EvaluateArguments> > Sandbox::evaluators_;
+
+} // namespace
Property changes on: sandbox/linux/seccomp_bpf/sandbox_bpf.cc
___________________________________________________________________
Added: svn:eol-style
   + LF