Non-SFI NaCl: Disallow mmap with PROT_EXEC

components/nacl/loader/nonsfi/nonsfi_sandbox.cc

Index: components/nacl/loader/nonsfi/nonsfi_sandbox.cc
diff --git a/components/nacl/loader/nonsfi/nonsfi_sandbox.cc b/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
index a58ba840b73991c29485556968f9845b3045d82f..866f1da431ec540f9f532936bb107e56147daa19 100644
--- a/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
+++ b/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
@@ -124,12 +124,16 @@ ErrorCode RestrictMemoryProtection(SandboxBPF* sb, int argno) {
 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) {