Linux sandbox: Tighten up the NaCl sandbox policy.

components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc

 // Copyright 2013 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 "components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.h"
 
 #include "build/build_config.h"
 
 #if defined(USE_SECCOMP_BPF)
 
 #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)
 
 namespace nacl {
 
 #if defined(USE_SECCOMP_BPF)
 
 namespace {
 
 using sandbox::bpf_dsl::Allow;
 using sandbox::bpf_dsl::Error;
 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;
   virtual ResultExpr InvalidSyscall() const override {
     return baseline_policy_->InvalidSyscall();
   }
 
  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_);
+
+  // 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:
+        return Allow();
+#elif defined(__i386__)
+      case __NR_socketcall:
+        return Allow();
+#endif
+      default:
+        break;
+    }
+  }
+
   switch (sysno) {
-    // TODO(jln): NaCl's GDB debug stub uses the following socket system calls,
-    // see if it can be restricted a bit.
-#if defined(__x86_64__) || defined(__arm__) || defined(__mips__)
-    // transport_common.cc needs this.
-    case __NR_accept:
-    case __NR_setsockopt:
-#elif defined(__i386__)
-    case __NR_socketcall:
-#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.

+    // 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.
     case __NR_modify_ldt:
 #endif
     // NaClAddrSpaceBeforeAlloc needs prlimit64.
     case __NR_prlimit64:
     // NaCl uses custom signal stacks.
     case __NR_sigaltstack:
     // Below is fairly similar to the policy for a Chromium renderer.
 #if defined(__i386__) || defined(__x86_64__) || defined(__mips__)
     case __NR_getrlimit:
 #endif
 #if defined(__i386__) || defined(__arm__)
     case __NR_ugetrlimit:
 #endif
     // NaCl runtime exposes clock_getres to untrusted code.
     case __NR_clock_getres:
     // NaCl runtime uses flock to simulate POSIX behavior for pwrite.
     case __NR_flock:
     case __NR_pread64:
     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.
     // See crbug.com/264856 for details.
     case __NR_times:
     case __NR_uname:
       return Allow();
     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);
   }
   NOTREACHED();
   // GCC wants this.
   return Error(EPERM);
 }
 
 void RunSandboxSanityChecks() {
   errno = 0;
@@ skipping 18 matching lines @@
       scoped_ptr<sandbox::bpf_dsl::Policy>(new NaClBPFSandboxPolicy));
   if (sandbox_is_initialized) {
     RunSandboxSanityChecks();
     return true;
   }
 #endif  // defined(USE_SECCOMP_BPF)
   return false;
 }
 
 }  // namespace nacl