Non-SFI mode: Sandbox support for NaCl async-signals.

components/nacl/loader/nonsfi/nonsfi_sandbox.cc

 // Copyright 2014 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/nonsfi/nonsfi_sandbox.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/net.h>
 #include <sys/mman.h>
 #include <sys/prctl.h>
 #include <sys/socket.h>
 #include <sys/syscall.h>
 #include <sys/time.h>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "content/public/common/sandbox_init.h"
 #include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h"
 #include "sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h"
 #include "sandbox/linux/system_headers/linux_futex.h"
+#include "sandbox/linux/system_headers/linux_signal.h"
 #include "sandbox/linux/system_headers/linux_syscalls.h"
 
 // Chrome OS Daisy (ARM) build environment and PNaCl toolchain do not define
 // MAP_STACK.
 #if !defined(MAP_STACK)
 # if defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARM_FAMILY)
 #  define MAP_STACK 0x20000
 # else
 // Note that, on other architecture, MAP_STACK has different value (e.g. mips'
 // MAP_STACK is 0x40000), though Non-SFI is not supported on such
@@ skipping 41 matching lines @@
 
 ResultExpr RestrictClone() {
   // We allow clone only for new thread creation.
   int clone_flags =
       CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
       CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS;
 #if !defined(OS_NACL_NONSFI)
   clone_flags |= CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID;
 #endif
   const Arg<int> flags(0);
-  return If(flags == clone_flags, Allow()).Else(CrashSIGSYSClone());
+  return If(flags == clone_flags ||

[Mark Seaborn, 2015/08/13 23:38:10]

How about adding a TODO to remove the variant without CLONE_PARENT_SETTID after
the NaCl-side change is rolled in?

[Luis Héctor Chávez, 2015/08/14 00:29:32]

Done.

+                flags == (clone_flags | CLONE_PARENT_SETTID),
+            Allow()).Else(CrashSIGSYSClone());
 }
 
 ResultExpr RestrictFutexOperation() {
   // TODO(hamaji): Allow only FUTEX_PRIVATE_FLAG futexes.
   const uint64_t kAllowedFutexFlags = FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME;
   const Arg<int> op(1);
   return Switch(op & ~kAllowedFutexFlags)
       .CASES((FUTEX_WAIT,
               FUTEX_WAKE,
               FUTEX_REQUEUE,
@@ skipping 42 matching lines @@
       MAP_SHARED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK | MAP_FIXED;
   // When PROT_EXEC is specified, IRT mmap of Non-SFI NaCl helper
   // calls mmap without PROT_EXEC and then adds PROT_EXEC by mprotect,
   // so we do not need to allow PROT_EXEC in mmap.
   const uint64_t kAllowedProtMask = PROT_READ | PROT_WRITE;
   const Arg<int> prot(2), flags(3);
   return If((prot & ~kAllowedProtMask) == 0 && (flags & ~kAllowedFlagMask) == 0,
             Allow()).Else(CrashSIGSYS());
 }
 
+ResultExpr RestrictTgkill() {
+  const Arg<int> tgid(0), signum(2);
+  // Only sending SIGUSR1 to a thread in the same process is allowed.
+  return If(tgid == getpid() && signum == LINUX_SIGUSR1,

[Mark Seaborn, 2015/08/13 23:38:10]

Should we check that the thread ID is positive too, just to be on the safe side?

The Linux man page for tgkill() doesn't document any special behaviour for 0 or
negative values, but the Linux man pages are notoriously incomplete.

[Luis Héctor Chávez, 2015/08/14 00:29:32]

Arg<int> does not support greater-than comparison :( (although a mask operation
with an Arg<uint32_t> could work in a pinch).

Looks like Linux does the lookup for the thread ID using equality (kernel/pid.c,
find_pid_ns()) and does not seem to give any special treatment to negative
values. Furthermore, negative thread IDs are invalid since alloc_pidmap()
returns -1 on failure. We should be safe with the code as-is.

[Mark Seaborn, 2015/08/14 20:12:41]

I suppose you can check that:

 * arg != 0
 * (arg & (1 << 31)) == 0  // arg is non-negative

And comment about the lack of a greater-than op.

Actually, can you add a test for this in
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc, please?  I forgot
about that before.
That may be true for current versions, but we're slightly paranoid that future
Linux versions might introduce weird corner cases. :-)

+            Allow()).Else(CrashSIGSYS());
+}
+
 #if !defined(OS_NACL_NONSFI) && (defined(__x86_64__) || defined(__arm__))
 ResultExpr RestrictSocketpair() {
   // Only allow AF_UNIX, PF_UNIX. Crash if anything else is seen.
   static_assert(AF_UNIX == PF_UNIX, "AF_UNIX must equal PF_UNIX.");
   const Arg<int> domain(0);
   return If(domain == AF_UNIX, Allow()).Else(CrashSIGSYS());
 }
 #endif
 
 bool IsGracefullyDenied(int sysno) {
@@ skipping 138 matching lines @@
 #if !defined(OS_NACL_NONSFI)
     // nacl_helper in Non-SFI mode still uses socketpair() internally
     // via libevent.
     // TODO(hidehiko): Remove this when the switching to nacl_helper_nonsfi
     // is completed.
     case __NR_socketpair:
       return RestrictSocketpair();
 #endif
 #endif
 
+    case __NR_tgkill:
+      return RestrictTgkill();
+
     case __NR_brk:
       // The behavior of brk on Linux is different from other system
       // calls. It does not return errno but the current break on
       // failure. glibc thinks brk failed if the return value of brk
       // is less than the requested address (i.e., brk(addr) < addr).
       // So, glibc thinks brk succeeded if we return -EPERM and we
       // need to return zero instead.
       return Error(0);
 
     default:
@@ skipping 13 matching lines @@
           new nacl::nonsfi::NaClNonSfiBPFSandboxPolicy()),
       proc_fd.Pass());
   if (!sandbox_is_initialized)
     return false;
   RunSandboxSanityChecks();
   return true;
 }
 
 }  // namespace nonsfi
 }  // namespace nacl