NaCl: enable a real seccomp-bpf sandbox.

chrome/nacl/nacl_sandbox_linux.cc

Index: chrome/nacl/nacl_sandbox_linux.cc
diff --git a/chrome/nacl/nacl_sandbox_linux.cc b/chrome/nacl/nacl_sandbox_linux.cc
index 19585eaf26d693bbd8daeb85194820ba90774d6f..96e8122acc843e4ac17a7c324d3d51e0711127d6 100644
--- a/chrome/nacl/nacl_sandbox_linux.cc
+++ b/chrome/nacl/nacl_sandbox_linux.cc
@@ -19,26 +19,87 @@ using playground2::Sandbox;
 
 namespace {
 
-// This policy does very little:
-// - Any invalid system call for the current architecture is handled by
-//   the baseline policy.
-// - ptrace() is denied.
-// - Anything else is allowed.
-// Note that the seccomp-bpf sandbox always prevents cross-architecture
-// system calls (on x86, long/compatibility/x32).
-// So even this trivial policy has a security benefit.
+#if defined(__x86_64__) || defined(__arm__)

[Mark Seaborn, 2013/07/22 23:45:24]

Can you add a comment about why there's an #if here?  i.e. x86-32 has __NR_ipc
instead.  Or just merge this with the following function, or do "#elif
defined(__i386__)" below.

Why not put the #if inside the function, so that we don't need an #if later?

[jln (very slow on Chromium), 2013/07/23 00:18:16]

Added a comment.

This looks quite ugly indeed, but since it's duplicating a tiny part of code
inside content/, I wanted to keep it the same.

Upcoming refactors should bring more sanity to this.

+bool IsSystemVSharedMemory(int sysno) {
+  switch (sysno) {
+    case __NR_shmat:
+    case __NR_shmctl:
+    case __NR_shmdt:
+    case __NR_shmget:
+      return true;
+    default:
+      return false;
+  }
+}
+#endif
+
+#if defined(__i386__)
+// Big system V multiplexing system call.
+bool IsSystemVIpc(int sysno) {
+  switch (sysno) {
+    case __NR_ipc:
+      return true;
+    default:
+      return false;
+  }
+}
+#endif
+
 ErrorCode NaClBpfSandboxPolicy(
-    playground2::Sandbox* sb, int sysnum, void* aux) {
+    playground2::Sandbox* sb, int sysno, void* aux) {
   const playground2::BpfSandboxPolicyCallback baseline_policy =
       content::GetBpfSandboxBaselinePolicy();
-  if (!playground2::Sandbox::IsValidSyscallNumber(sysnum)) {
-    return baseline_policy.Run(sb, sysnum, aux);
-  }
-  switch (sysnum) {
+  switch (sysno) {
+    // TODO: jln: figure out what in NaClGdbDebugStubTest.Breakpoint
+    // needs the 4 following system calls.
+#if defined(__x86_64__) || defined(__arm__)
+    case __NR_accept:

[Mark Seaborn, 2013/07/22 23:45:24]

It's used by native_client/src/trusted/debug_stub/transport_common.cc

[jln (very slow on Chromium), 2013/07/23 00:18:16]

Done.

+    case __NR_setsockopt:
+#elif defined(__i386__)
+    case __NR_socketcall:
+#endif
+    case __NR_rt_sigtimedwait:

[Mark Seaborn, 2013/07/22 23:45:24]

Can you comment that this is used by the sigwait() call used by
linux/thread_suspension.c?

[jln (very slow on Chromium), 2013/07/23 00:18:16]

Done.

+#if defined(__i386__)
+    // Needed on i386 to set-up the custom segments.
+    case __NR_modify_ldt:
+#endif
+    // NaClAddrSpaceBeforeAlloc needs this.
+    case __NR_prlimit64:
+    // NaCl uses custom signal stacks.
+    case __NR_sigaltstack:
+    // Below is fairly similar to the policy for a Chromium renderer.
+    // TODO(jln): restrict clone(), ioctl() and prctl().
+    case __NR_ioctl:

[Mark Seaborn, 2013/07/22 23:45:24]

I don't think NaCl uses ioctl().  Does it work to remove this?  Maybe this is
used for TCP socket settings by the GDB debug stub.

[jln (very slow on Chromium), 2013/07/23 00:18:16]

This needs to be restricted (via parameters) like we did for renderers.

TCGETS and FIONREAD are used by glibc I think.

I think NaCl may need something more, but I didn't investigate yet. I'll
definitely execute on the above TODO, but I would like to get something working
soon.

+#if defined(__i386__) || defined(__x86_64__)
+    case __NR_getrlimit:
+#endif
+#if defined(__i386__) || defined(__arm__)
+    case __NR_ugetrlimit:
+#endif
+    case __NR_pread64:
+    case __NR_pwrite64:
+    case __NR_sched_get_priority_max:

[Mark Seaborn, 2013/07/22 23:45:24]

I don't know whether any of these sched_* calls are required.  How did you
arrive at this list?

[jln (very slow on Chromium), 2013/07/23 00:18:16]

Not sure what needs it (it's something in glibc IIRC), but they are definitely
required.

+    case __NR_sched_get_priority_min:
+    case __NR_sched_getaffinity:
+    case __NR_sched_getparam:
+    case __NR_sched_getscheduler:
+    case __NR_sched_setscheduler:
+    case __NR_setpriority:
+    case __NR_sysinfo:
+    case __NR_uname:
+      return ErrorCode(ErrorCode::ERR_ALLOWED);
     case __NR_ptrace:
       return ErrorCode(EPERM);
     default:
-      return ErrorCode(ErrorCode::ERR_ALLOWED);
+      // TODO(jln): look into getting rid of System V shared memory.

[Mark Seaborn, 2013/07/22 23:45:24]

Does Chromium have the renderer use X's SysV shared memory segments these days? 
I thought Chromium just does a copy these days instead.  If so, we don't need to
allow SysV IPC.

[jln (very slow on Chromium), 2013/07/23 00:18:16]

We don't need Sys V shm in Chromium on Aura, but we still need it on GTK.

However, NaCl needs it unconditionally because of
platform_qualify/linux/sysv_shm_and_mmap.c

I suspect we can get rid of that, but again, I would rather land something soon
and improve on it.

[Mark Seaborn, 2013/07/23 17:13:03]

OK, can you put that information into a comment, please?

+#if defined(__x86_64__) || defined(__arm__)
+      if (IsSystemVSharedMemory(sysno))
+        return ErrorCode(ErrorCode::ERR_ALLOWED);
+#elif defined(__i386__)
+      if (IsSystemVIpc(sysno))
+        return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
+      return baseline_policy.Run(sb, sysno, aux);
   }
   NOTREACHED();
   // GCC wants this.