Add seccomp sandbox for non-SFI NaCl

components/nacl/loader/nacl_sandbox_linux.cc

Index: components/nacl/loader/nacl_sandbox_linux.cc
diff --git a/components/nacl/loader/nacl_sandbox_linux.cc b/components/nacl/loader/nacl_sandbox_linux.cc
index f1e4a49a4f71b1ab1efaaade862ea8a9d9f399b2..f49695cb1f4e4a6730e4713191f649de1d23cf4b 100644
--- a/components/nacl/loader/nacl_sandbox_linux.cc
+++ b/components/nacl/loader/nacl_sandbox_linux.cc
@@ -5,8 +5,10 @@
 #include "components/nacl/loader/nacl_sandbox_linux.h"
 
 #include <errno.h>
+#include <linux/net.h>
 #include <signal.h>
 #include <sys/ptrace.h>
+#include <sys/syscall.h>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
@@ -16,8 +18,11 @@
 
 #if defined(USE_SECCOMP_BPF)
 #include "content/public/common/sandbox_init.h"
+#include "sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h"
+#include "sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h"
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf_policy.h"
+#include "sandbox/linux/seccomp-bpf/trap.h"
 #include "sandbox/linux/services/linux_syscalls.h"
 
 using sandbox::ErrorCode;
@@ -122,6 +127,7 @@ ErrorCode NaClBPFSandboxPolicy::EvaluateSyscall(
     case __NR_uname:
       return ErrorCode(ErrorCode::ERR_ALLOWED);
     case __NR_ptrace:
+      // For RunSandboxSanityChecks().
       return ErrorCode(EPERM);
     default:
       // TODO(jln): look into getting rid of System V shared memory:
@@ -142,6 +148,175 @@ ErrorCode NaClBPFSandboxPolicy::EvaluateSyscall(
   return ErrorCode(EPERM);
 }
 
+// The seccomp sandbox policy for NaCl non-SFI mode. Note that this
+// policy must be as strong as possible, as non-SFI mode heavily
+// depends on seccomp sandbox.
+class NonSfiNaClBPFSandboxPolicy : public SandboxBPFPolicy {
+ public:
+  explicit NonSfiNaClBPFSandboxPolicy()
+      : baseline_policy_(content::GetBPFSandboxBaselinePolicy()) {
+  }
+  virtual ~NonSfiNaClBPFSandboxPolicy() {}
+
+  virtual ErrorCode EvaluateSyscall(SandboxBPF* sandbox_compiler,
+                                    int system_call_number) const OVERRIDE;
+
+ private:
+  scoped_ptr<SandboxBPFPolicy> baseline_policy_;
+  DISALLOW_COPY_AND_ASSIGN(NonSfiNaClBPFSandboxPolicy);
+};
+
+ErrorCode NonSfiNaClBPFSandboxPolicy::EvaluateSyscall(
+    sandbox::SandboxBPF* sb, int sysno) const {
+  DCHECK(baseline_policy_);
+  ErrorCode ret = baseline_policy_->EvaluateSyscall(sb, sysno);

[Mark Seaborn, 2014/03/18 23:53:58]

You're falling through to the baseline policy.  I don't think that's good for
Bare Metal Mode because the baseline policy is quite complex.

I think we'll want to define a minimal standalone policy.

[jln (very slow on Chromium), 2014/03/20 00:54:53]

I absolutely agree with Mark here. We need a short whitelist of what's allowed.
The rest must be explicitly denied here.

This complex combination of a blacklist on top of an external policy is not
safe.

[hamaji, 2014/03/24 15:56:37]

Done. I also stopped using sandbox::Restrict* as they might be changed without
thinking about BMM.

+  switch (sysno) {
+    // __NR_times needed as clock() is called by CommandBufferHelper, which is
+    // used by NaCl applications that use Pepper's 3D interfaces.
+    // See crbug.com/264856 for details.
+    case __NR_times:
+      ret = ErrorCode(ErrorCode::ERR_ALLOWED);
+      break;
+
+    // Conditionally allowed syscalls:
+    case __NR_clone: {
+      // We allow clone only for new thread creation.
+      ret = sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
+                     CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+                     CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS |
+                     CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID,
+                     ErrorCode(ErrorCode::ERR_ALLOWED),
+            sb->Trap(sandbox::SIGSYSCloneFailure, NULL));
+      break;
+    }
+    case __NR_prctl:
+      // We only allow PR_SET_NAME, PR_SET_DUMPABLE, and PR_GET_DUMPABLE.

[Mark Seaborn, 2014/03/18 23:53:58]

I don't think these are operations that should be needed or allowed.  Also this
comment duplicates what RestrictPrctl() checks, which could get out of date.

[hamaji, 2014/03/24 15:56:37]

I stopped using RestrictPrctl. Now it returns EPERM for PR_SET_NAME and
otherwise it crashes.

+      ret = sandbox::RestrictPrctl(sb);
+      break;
+#if defined(__i686__)

[Mark Seaborn, 2014/03/18 23:53:58]

__i386__ would be preferred.

[hamaji, 2014/03/24 15:56:37]

Done.

+    case __NR_socketcall: {
+      // We only allow socketpair, sendmsg, and recvmsg.
+      ret = sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
+                     SYS_SOCKETPAIR, ErrorCode(ErrorCode::ERR_ALLOWED),
+            sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
+                     SYS_SENDMSG, ErrorCode(ErrorCode::ERR_ALLOWED),
+            sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
+                     SYS_RECVMSG, ErrorCode(ErrorCode::ERR_ALLOWED),
+                     ErrorCode(EPERM))));
+      break;
+    }
+#endif
+
+    // It is allowed to call the following syscalls, but they just
+    // return EPERM.
+    case __NR_ptrace:
+      // For RunSandboxSanityChecks().
+      ret = ErrorCode(EPERM);
+      break;
+    case __NR_set_robust_list:
+      // glibc uses this for its pthread implementation. If we return
+      // EPERM for this, glibc will stop using this.
+      // TODO(hamaji): newlib does not use this. Make this SIGTRAP once
+      // we have switched to newlib.
+      ret = ErrorCode(EPERM);
+      break;
+#if !defined(__x86_64__)
+    case __NR_getegid32:
+    case __NR_geteuid32:
+    case __NR_getgid32:
+    case __NR_getuid32:
+#endif
+      // third_party/libevent uses them, but we can just return -1 from
+      // them as it is just checking getuid() != geteuid() and
+      // getgid() != getegid()
+      ret = ErrorCode(EPERM);
+      break;
+#if !defined(__arm__)

[Mark Seaborn, 2014/03/18 23:53:58]

Should be "defined(__i386__) || defined(__x86_64__)" for x86 things such as
modify_ldt.

[hamaji, 2014/03/24 15:56:37]

Done.

+    case __NR_time:
+      // This is obsolete in ARM EABI, but x86 glibc indirectly calls
+      // this in sysconf.
+    case __NR_modify_ldt:
+      // nacl_helper calls this from service_runtime/linux/x86/nacl_ldt.c

[Mark Seaborn, 2014/03/18 23:53:58]

But this doesn't apply to Non-SFI Mode.

[hamaji, 2014/03/24 15:56:37]

It seems current nacl_helper always calls NaClTlsInit. I created another CL so
that we can avoid calling NaClTlsInit in non-SFI mode.

https://codereview.chromium.org/209423004/

+      ret = ErrorCode(EPERM);
+      break;
+#endif
+
+    // Followings are restricted syscalls.
+
+    // glibc internally calls brk in its memory allocation.

[Mark Seaborn, 2014/03/18 23:53:58]

glibc does call brk(), but it doesn't require it to work.  It's OK for brk() to
fail: glibc will fall back to using mmap().

[jln (very slow on Chromium), 2014/03/20 00:54:53]

Yes, in other words, just EPERM it.

[hamaji, 2014/03/24 15:56:37]

Oops, my comment was wrong. brk was called by tcmalloc. We can get rid of brk
call by removing #define HAVE_SBRK 1 from
third_party/tcmalloc/chromium/src/config_linux.h. I think there are some
options:

1. Remove HAVE_SBRK from config_linux.h. I feel this is the right choice, but
I'd like to avoid this, because this might affect a lot of things (memory
profiling tools, etc.).
2. Remove HAVE_SBRK only from nacl_helper. Currently, nacl_helper uses the same
liballocator.a as chrome's, so some gyp tweaks would be needed.
3. Stop using tcmalloc from nacl_helper. I'm not sure if nacl_helper really
wants tcmalloc.
4. Wait until newlib switch. Once we have switched to newlib and split
nacl_helper into SFI and non-SFI helpers, we don't need to link tcmalloc in
non-SFI nacl_helper.

[Mark Seaborn, 2014/03/28 16:41:39]

Does tcmalloc currently require brk(), or does it fall back to mmap() if brk()
fails?

Either way, I think it would make sense to stop linking tcmalloc into
nacl_helper.  Statically linking tcmalloc is mostly just wasting space.  We
don't need tcmalloc for SFI mode.  Unlike a renderer, NaCl's trusted runtime
doesn't do a lot of allocations, so there's no need to tweak the allocator by
overriding glibc's one.

+    // TODO(hamaji): Uncomment this once newlib switch has been done.
+    // case __NR_brk:
+    case __NR_capget:

[Mark Seaborn, 2014/03/18 23:53:58]

This is a blacklist of disallowed syscalls, but I don't think having a blacklist
is a good idea.  Disallowing via SIGSYS should really be the default.

Maybe disallowed syscalls should just be listed in a test that checks that
kill() etc. are disabled by the BPF filter.

It looks like this blacklist aims to override syscalls that are allowed by the
baseline policy defined by sandbox/linux/seccomp-bpf-helpers/baseline_policy.cc.

[hamaji, 2014/03/24 15:56:37]

Done.

+    case __NR_dup3:
+    case __NR_epoll_create1:
+    case __NR_fcntl:
+    case __NR_fork:
+    case __NR_get_robust_list:
+    case __NR_getgroups:
+    case __NR_getpid:
+    case __NR_getppid:
+    case __NR_getresgid:
+    case __NR_getresuid:
+    case __NR_getsid:
+    case __NR_kill:
+    case __NR_mlock:
+    case __NR_munlock:
+    case __NR_pause:
+    case __NR_pipe2:
+    case __NR_poll:
+    case __NR_ppoll:
+    case __NR_pselect6:
+    case __NR_readv:
+    case __NR_recvmmsg:
+    case __NR_rt_sigaction:
+    case __NR_sendmmsg:
+    case __NR_tgkill:
+    case __NR_tkill:
+    case __NR_wait4:
+    case __NR_waitid:
+    case __NR_writev:
+    // We do not need 32bit versions of them.
+    case __NR_fstat:
+    case __NR_lseek:
+#if !defined(__arm__)
+    // They do not exist on ARM EABI.
+    case __NR_select:
+#endif
+#if defined(__i686__)
+    // This is i686 only.
+    case __NR_waitpid:
+#endif
+#if !defined(__i686__)
+    // i686 uses socketcall instead of them.
+    case __NR_sendto:
+    case __NR_shutdown:
+    case __NR_recvfrom:
+#if defined(__arm__)
+    case __NR_send:
+    case __NR_recv:
+#endif
+#endif
+#if !defined(__x86_64__)
+    // They do not exist on x86-64.
+    case __NR__newselect:
+    case __NR_getegid:
+    case __NR_geteuid:
+    case __NR_getgid:
+    case __NR_getgroups32:
+    case __NR_getresgid32:
+    case __NR_getresuid32:
+    case __NR_getuid:
+    case __NR_rt_sigprocmask:
+    case __NR_rt_sigreturn:
+    case __NR_sigprocmask:
+    case __NR_sigreturn:
+#endif
+      ret = sb->Trap(sandbox::CrashSIGSYS_Handler, NULL);
+      break;
+  }
+  return ret;
+}
+
 void RunSandboxSanityChecks() {
   errno = 0;
   // Make a ptrace request with an invalid PID.
@@ -172,3 +347,15 @@ bool InitializeBPFSandbox() {
 #endif  // defined(USE_SECCOMP_BPF)
   return false;
 }
+
+bool InitializeBPFSandboxForNonSfi() {
+#if defined(USE_SECCOMP_BPF)
+  bool sandbox_is_initialized = content::InitializeSandbox(

[hamaji, 2014/03/14 12:46:23]

I thought we should add something like

 #if !defined(__arm__)
 return false;
 #endif

because i686 and x86-64 support could be incomplete. However, Hidehiko says it
will break our unit tests and having no guard for this would be OK, as non-SFI
mode is guarded by --enable-nacl-nonsfi-mode flag anyway.

[jln (very slow on Chromium), 2014/03/20 00:54:53]

We should not return false in non SFI mode. We should LOG(FATAL) if this fails.

[hamaji, 2014/03/24 15:56:37]

Done.

+      scoped_ptr<SandboxBPFPolicy>(new NonSfiNaClBPFSandboxPolicy()));
+  if (sandbox_is_initialized) {
+    RunSandboxSanityChecks();
+    return true;
+  }
+#endif  // defined(USE_SECCOMP_BPF)
+  return false;
+}