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 9bd807e473bdfc252cb56ef15538ed5620be6451..b7dbe64aab238b9ed64d035997a8ec63a88d41f4 100644
--- a/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
+++ b/components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc
@@ -11,15 +11,20 @@
 #include <errno.h>
 #include <signal.h>
 #include <sys/ptrace.h>
+#include <sys/types.h>
+#include <unistd.h>
 
 #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/bpf_dsl/policy.h"
+#include "sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h"
 #include "sandbox/linux/services/linux_syscalls.h"
 
 #endif  // defined(USE_SECCOMP_BPF)
@@ -37,7 +42,12 @@ using sandbox::bpf_dsl::ResultExpr;
 class NaClBPFSandboxPolicy : public sandbox::bpf_dsl::Policy {
  public:
   NaClBPFSandboxPolicy()
-      : baseline_policy_(content::GetBPFSandboxBaselinePolicy()) {}
+      : baseline_policy_(content::GetBPFSandboxBaselinePolicy()),
+        policy_pid_(getpid()) {
+    const base::CommandLine* command_line =
+        base::CommandLine::ForCurrentProcess();
+    enable_nacl_debug_ = command_line->HasSwitch(switches::kEnableNaClDebug);

[Mark Seaborn, 2014/10/28 00:30:14]

Maybe add a comment like:

// nacl_process_host.cc doesn't always enable the debug stub when
// kEnableNaClDebug is passed, but it's OK to enable the extra syscalls
// whenever kEnableNaClDebug is passed.

[rickyz (no longer on Chrome), 2014/10/29 21:07:36]

Done.

+  }
   virtual ~NaClBPFSandboxPolicy() {}
 
   virtual ResultExpr EvaluateSyscall(int system_call_number) const override;
@@ -47,24 +57,35 @@ class NaClBPFSandboxPolicy : public sandbox::bpf_dsl::Policy {
 
  private:
   scoped_ptr<sandbox::bpf_dsl::Policy> baseline_policy_;
+  bool enable_nacl_debug_;
+  pid_t policy_pid_;

[jln (very slow on Chromium), 2014/10/29 18:44:45]

const

[rickyz (no longer on Chrome), 2014/10/29 21:07:36]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(NaClBPFSandboxPolicy);
 };
 
 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.
+
+  // NaCl's GDB debug stub uses the following socket system calls. We only
+  // allow then when --enable-nacl-debug is specified.

[Mark Seaborn, 2014/10/28 00:30:14]

"then" -> "them"

+  if (enable_nacl_debug_) {
+    switch (sysno) {
 #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.

[Mark Seaborn, 2014/10/28 00:30:14]

I think it would be worth keeping this comment and disabling rt_sigtimedwait(),
to reduce the attack surface.  We can always change this policy if it turns out
we do need thread suspension for other reasons.  My earlier comment
(https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s...)
was just for context, really.

[rickyz (no longer on Chrome), 2014/10/29 21:07:36]

Ah, OK - moved it into the list of syscalls only allowed under kEnableNaClDebug.

+      default:
+        break;
+    }
+  }
+
+  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.
@@ -89,10 +110,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.
@@ -103,6 +120,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(policy_pid_, sysno);

[Mark Seaborn, 2014/10/28 00:30:14]

Why not call getpid() here?  I'm not sure why you're caching it on the object. 
Maybe add a comment if there's a reason for caching it?

[jln (very slow on Chromium), 2014/10/29 18:40:16]

I actually requested that (see previous comments). The main reason is that
EvaluateSyscall(X) needs to be guaranteed to always return the same thing. It is
good practice to not call functions that depend on the environment from there.
The policy is computed as being "bound" to a pid (which we happen to choose as
the current one). This is much better expressed by a member variable for the
policy itself.

Moreover, and for some historical context, getpid() is very problematic because
of pid caching (for processes created with clone). We solved this once and for
all in the "baseline" policy by using syscall(SYS_getppid); and putting that in
a member variable. We can unfortunately not use it here, due to the fact that we
have to request that policy from content/ and the interface I created for that
doesn't have that notion.

[rickyz (no longer on Chrome), 2014/10/29 21:07:36]

I added a CHECK in EvaluateSyscall - hopefully that documents the the
requirement (and also somewhat justifies the member variable).

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