Linux sandbox: Tighten up the NaCl sandbox policy.

components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc

Index: components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
diff --git a/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc b/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
index a1449cf42ed721eb9220c2cec05bfd143c81e77d..3153ffff049587e1886c2bc6fc00643a76b77a41 100644
--- a/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
+++ b/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
@@ -14,11 +14,14 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 
+#include "components/nacl/common/nacl_switches.h"
 #include "content/public/common/sandbox_init.h"
 #include "sandbox/linux/bpf_dsl/bpf_dsl.h"
+#include "sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h"
 #include "sandbox/linux/services/linux_syscalls.h"
 
 #endif  // defined(USE_SECCOMP_BPF)
@@ -52,18 +55,27 @@ class NaClBPFSandboxPolicy : public sandbox::bpf_dsl::SandboxBPFDSLPolicy {
 
 ResultExpr NaClBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
   DCHECK(baseline_policy_);
-  switch (sysno) {
-    // TODO(jln): NaCl's GDB debug stub uses the following socket system calls,
-    // see if it can be restricted a bit.
+
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kEnableNaClDebug)) {

[jln (very slow on Chromium), 2014/10/23 17:51:55]

Could you instead have a class variable to record this property and check this
when we instantiate the class instead?

as-is, we'll repeat this for every syscall and also it go against guaranteeing
that calling EvaluateSyscall() twice with the same parameters will yield the
same result.

[rickyz (no longer on Chrome), 2014/10/23 20:05:30]

Done.

+    switch (sysno) {
+      // NaCl's GDB debug stub uses the following socket system calls. We only
+      // allow then when --enable-nacl-debug is specified.
 #if defined(__x86_64__) || defined(__arm__) || defined(__mips__)
-    // transport_common.cc needs this.
-    case __NR_accept:
-    case __NR_setsockopt:
+      // transport_common.cc needs this.
+      case __NR_accept:
+      case __NR_setsockopt:
+        return Allow();
 #elif defined(__i386__)
-    case __NR_socketcall:
+      case __NR_socketcall:
+        return Allow();
 #endif
-    // trusted/service_runtime/linux/thread_suspension.c needs sigwait() and is
-    // used by NaCl's GDB debug stub.
+    }
+  }
+
+  switch (sysno) {
+    // trusted/service_runtime/linux/thread_suspension.c needs sigwait()
     case __NR_rt_sigtimedwait:
 #if defined(__i386__) || defined(__mips__)
     // Needed on i386 to set-up the custom segments.
@@ -88,10 +100,6 @@ ResultExpr NaClBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
     case __NR_pwrite64:
     case __NR_sched_get_priority_max:
     case __NR_sched_get_priority_min:
-    case __NR_sched_getaffinity:
-    case __NR_sched_getparam:
-    case __NR_sched_getscheduler:
-    case __NR_sched_setscheduler:
     case __NR_sysinfo:
     // __NR_times needed as clock() is called by CommandBufferHelper, which is
     // used by NaCl applications that use Pepper's 3D interfaces.
@@ -102,6 +110,11 @@ ResultExpr NaClBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
     case __NR_ioctl:
     case __NR_ptrace:
       return Error(EPERM);
+    case __NR_sched_getaffinity:
+    case __NR_sched_getparam:
+    case __NR_sched_getscheduler:
+    case __NR_sched_setscheduler:
+      return sandbox::RestrictSchedTarget(getpid(), sysno);

[jln (very slow on Chromium), 2014/10/23 17:51:55]

Why not policy_pid() ?

(As a general rule, we should not query "the environment" in EvaluateSyscall()
as it runs the risk that calling EvaluateSyscall() twice with the same parameter
would not yield the same result).

[rickyz (no longer on Chrome), 2014/10/23 20:05:30]

policy_pid isn't exposed via sandbox::bpf_dsl::SandboxBPFDSLPolicy, so I changed
NaClBPFSandboxPolicy to subclass public sandbox::BaselinePolicy instead of it as
a member and forwarding the functions explicitly. What do you think of this?

     default:
       return baseline_policy_->EvaluateSyscall(sysno);
   }