Non-SFI NaCl: Disallow mmap with PROT_EXEC

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/prctl.h>
@@ skipping 92 matching lines @@
          sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
                   SYS_RECVMSG,
                   ErrorCode(ErrorCode::ERR_ALLOWED),
          sb->Cond(0, ErrorCode::TP_32BIT, ErrorCode::OP_EQUAL,
                   SYS_SHUTDOWN,
                   ErrorCode(ErrorCode::ERR_ALLOWED),
                   sb->Trap(sandbox::CrashSIGSYS_Handler, NULL)))));
 }
 #endif
 
 ErrorCode RestrictMemoryProtection(SandboxBPF* sb, int argno) {

[jln (very slow on Chromium), 2014/04/24 16:46:38]

Maybe change this to "RestrictMprotect", without |argno| now?

Or do you want to keep it for flexibility?

[hamaji, 2014/04/25 01:44:47]

No, I'd prefer renaming this to RestrictMprotect. Thanks for the catch.

   // TODO(jln, keescook, drewry): Limit the use of mmap/mprotect by
   // adding some features to linux kernel.
   const uint32_t denied_mask = ~(PROT_READ | PROT_WRITE | PROT_EXEC);
   return sb->Cond(argno, ErrorCode::TP_32BIT,
                   ErrorCode::OP_HAS_ANY_BITS,
                   denied_mask,
          sb->Trap(sandbox::CrashSIGSYS_Handler, NULL),
                   ErrorCode(ErrorCode::ERR_ALLOWED));
 }
 
 ErrorCode RestrictMmap(SandboxBPF* sb) {
   const uint32_t denied_flag_mask = ~(MAP_SHARED | MAP_PRIVATE |
                                       MAP_ANONYMOUS | MAP_STACK | MAP_FIXED);
-  // TODO(hamaji): Disallow RWX mmap.
+  const uint32_t denied_prot_mask = ~(PROT_READ | PROT_WRITE);

[Mark Seaborn, 2014/04/24 16:49:30]

It's not really obvious why you'd want to do this.  Without changing mprotect,
it doesn't provide any security benefit since you can use mprotect to make the
mapping executable.

Is your intention to make sure we don't accidentally regress Chrome OS when
we're only testing on non-Chrome OS systems?  If so, can you add a comment
explaining that, please?

[hamaji, 2014/04/25 01:44:47]

I just guessed this would slight improve security. I thought

1. Running less kernel code would be nice. (maybe similar to what Julien said)
2. This could reduce the pattern of shellcode attackers can use (for the
whitelisted
 launch)? I don't know if this makes sense as attackers can just reuse existing
pages
by mprotect.
I didn't think of this case.

I'm not sure what kind of comment is appropriate for this. For now, I've added a
comment to explain why we can disallow PROT_EXEC. Suggestion of a better comment
is appreciated.

   return sb->Cond(3, ErrorCode::TP_32BIT,
                   ErrorCode::OP_HAS_ANY_BITS,
                   denied_flag_mask,
-         sb->Trap(sandbox::CrashSIGSYS_Handler, NULL),
-                  RestrictMemoryProtection(sb, 2));
+                  sb->Trap(sandbox::CrashSIGSYS_Handler, NULL),
+         sb->Cond(2, ErrorCode::TP_32BIT,
+                  ErrorCode::OP_HAS_ANY_BITS,
+                  denied_prot_mask,
+                  sb->Trap(sandbox::CrashSIGSYS_Handler, NULL),
+                  ErrorCode(ErrorCode::ERR_ALLOWED)));
 }
 
 ErrorCode RestrictSocketpair(SandboxBPF* sb) {
   // Only allow AF_UNIX, PF_UNIX. Crash if anything else is seen.
   COMPILE_ASSERT(AF_UNIX == PF_UNIX, af_unix_pf_unix_different);
   return sb->Cond(0, ErrorCode::TP_32BIT,
                   ErrorCode::OP_EQUAL, AF_UNIX,
                   ErrorCode(ErrorCode::ERR_ALLOWED),
          sb->Trap(sandbox::CrashSIGSYS_Handler, NULL));
 }
@@ skipping 163 matching lines @@
       scoped_ptr<sandbox::SandboxBPFPolicy>(
           new nacl::nonsfi::NaClNonSfiBPFSandboxPolicy()));
   if (!sandbox_is_initialized)
     return false;
   RunSandboxSanityChecks();
   return true;
 }
 
 }  // namespace nonsfi
 }  // namespace nacl