Restrict mmap(2) flags for x64.

content/common/sandbox_seccomp_bpf_linux.cc

 // Copyright (c) 2012 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 <asm/unistd.h>
 #include <dlfcn.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/audit.h>
 #include <linux/filter.h>
@@ skipping 82 matching lines @@
   int syscall = args.nr;
   if (syscall >= 1024)
     syscall = 0;
   // Encode 8-bits of the 1st two arguments too, so we can discern which socket
   // type, which fcntl, ... etc., without being likely to hit a mapped
   // address.
   // Do not encode more bits here without thinking about increasing the
   // likelihood of collision with mapped pages.
   syscall |= ((args.args[0] & 0xffUL) << 12);
   syscall |= ((args.args[1] & 0xffUL) << 20);
+syscall = args.args[3] & 0xffffffffUL;
   // Purposefully dereference the syscall as an address so it'll show up very
   // clearly and easily in crash dumps.
   volatile char* addr = reinterpret_cast<volatile char*>(syscall);
   *addr = '\0';
   // In case we hit a mapped address, hit the null page with just the syscall,
   // for paranoia.
   syscall &= 0xfffUL;
   addr = reinterpret_cast<volatile char*>(syscall);
   *addr = '\0';
   for (;;)
@@ skipping 538 matching lines @@
     default:
       return false;
   }
 }
 #endif
 
 bool IsAllowedAddressSpaceAccess(int sysno) {
   switch (sysno) {
     case __NR_brk:
     case __NR_mlock:
+    case __NR_mprotect:
+    case __NR_munlock:
+    case __NR_munmap:
+      return true;
 #if defined(__i386__) || defined(__x86_64__)
-    case __NR_mmap:   // TODO(jln): to restrict flags.
+    case __NR_mmap:

[jln (very slow on Chromium), 2013/05/15 20:01:05]

Nit: if we keep it this way, let's move mmap and mmap2 down to keep alphabetical
ordering!

 #endif
 #if defined(__i386__) || defined(__arm__)
     case __NR_mmap2:
 #endif
-    case __NR_mprotect:
-    case __NR_munlock:
-    case __NR_munmap:
-      return true;
     case __NR_madvise:
     case __NR_mincore:
     case __NR_mlockall:
 #if defined(__i386__) || defined(__x86_64__)
     case __NR_modify_ldt:
 #endif
     case __NR_mremap:
     case __NR_msync:
     case __NR_munlockall:
     case __NR_readahead:
@@ skipping 548 matching lines @@
 #if defined(__arm__)
       IsArmPciConfig(sysno) ||
 #endif
       IsTimer(sysno)) {
     return true;
   } else {
     return false;
   }
 }
 
+ErrorCode RestrictMmapFlags(Sandbox *sandbox) {
+  // These flags are allowed. Significantly, we don't permit MAP_HUGETLB, or
+  // the newer flags such as MAP_POPULATE.
+  uint32_t mask = ~(MAP_SHARED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK |

[jln (very slow on Chromium), 2013/05/15 20:01:05]

want to rename that variable denied_mask to make things more clear?

+                    MAP_NORESERVE | MAP_FIXED);
+  return sandbox->Cond(3, ErrorCode::TP_32BIT, ErrorCode::OP_HAS_ANY_BITS,

[jln (very slow on Chromium), 2013/05/15 20:01:05]

Nit:

I guess we should have provided a OP_HAS_NO_BITS helper, it's a bit confusing
that we're inverting the order of what's allowed / denied in comparison to the
other similar functions.

Can you try to make things a little be more clear by having denied_mask on the
previous line and the two Trap and ERR_ALLOWED each on their own line ?

+                       mask, sandbox->Trap(CrashSIGSYS_Handler, NULL),
+                       ErrorCode(ErrorCode::ERR_ALLOWED));
+}
+
 ErrorCode BaselinePolicy(Sandbox *sandbox, int sysno) {
 #if defined(__x86_64__) || defined(__arm__)
   if (sysno == __NR_socketpair) {
     // Only allow AF_UNIX, PF_UNIX. Crash if anything else is seen.
     COMPILE_ASSERT(AF_UNIX == PF_UNIX, af_unix_pf_unix_different);
     return sandbox->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL, AF_UNIX,
                          ErrorCode(ErrorCode::ERR_ALLOWED),
                          sandbox->Trap(CrashSIGSYS_Handler, NULL));
   }
 #endif
-  if (sysno == __NR_madvise) {
+  switch (sysno) {

[jln (very slow on Chromium), 2013/05/15 20:01:05]

If you want to make this a switch, make sure you start the switch on top.

Then you'd need a default: case as per style guide requirements.

I wonder if it's worth it though, I found the if () return logic fairly
readable. Your call.

+  case __NR_madvise:
     // Only allow MADV_DONTNEED (aka MADV_FREE).
     return sandbox->Cond(2, ErrorCode::TP_32BIT,
                          ErrorCode::OP_EQUAL, MADV_DONTNEED,
                          ErrorCode(ErrorCode::ERR_ALLOWED),
                          ErrorCode(EPERM));
+#if defined(__x86_64__)
+  case __NR_mmap:
+    return RestrictMmapFlags(sandbox);
+#elif defined(__i386__)
+  case __NR_mmap:
+    return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
+#if defined(__i386__) || defined(__arm__)
+  case __NR_mmap2:
+    return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
   }
 
   if (IsBaselinePolicyAllowed(sysno)) {

[jln (DO NOT USE THIS), 2013/05/15 20:10:14]

Another remark:

The goal of putting things *above* IsBaseLinePolicyAllowed() is that we do deny
thing that would otherwise be allowed. You can put all of these (which are now
defined by default in the baseline policy) below.

     return ErrorCode(ErrorCode::ERR_ALLOWED);
   }
 
 #if defined(__i386__)
   // socketcall(2) should be tightened.
   if (IsSocketCall(sysno)) {
     return ErrorCode(ErrorCode::ERR_ALLOWED);
   }
 #endif
 
@@ skipping 486 matching lines @@
     // should enable it, enable it or die.
     bool started_sandbox = StartBpfSandbox(command_line, process_type);
     CHECK(started_sandbox);
     return true;
   }
 #endif
   return false;
 }
 
 }  // namespace content