Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
247563004:
Non-SFI NaCl: Disallow mmap with PROT_EXEC (Closed)
|
Created:
6 years, 8 months ago by hamaji Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNon-SFI NaCl: Disallow mmap with PROT_EXEC
Now nacl_irt_mmap with PROT_EXEC is implemented by non-EXEC
mmap and mprotect thanks to
https://codereview.chromium.org/239763005
BUG=359285
TEST=nacl_loader_unittests, trybots
R=jln@chromium.org, mseaborn@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266453
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : comment update #
Messages
Total messages: 25 (0 generated)
https://chromiumcodereview.appspot.com/247563004/diff/20001/components/nacl/l... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/247563004/diff/20001/components/nacl/l... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:113: ErrorCode RestrictMemoryProtection(SandboxBPF* sb, int argno) { Maybe change this to "RestrictMprotect", without |argno| now? Or do you want to keep it for flexibility? https://chromiumcodereview.appspot.com/247563004/diff/20001/components/nacl/l... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/247563004/diff/20001/components/nacl/l... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:312: } Do you mind adding another test with PROT_EXEC | PROT_WRITE for instance? Generating BPF filters with bitmasks is notoriously difficult, so I wouldn't mind a bit more testing or the protections / flags.
https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:127: const uint32_t denied_prot_mask = ~(PROT_READ | PROT_WRITE); 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?
On 2014/04/24 16:49:30, Mark Seaborn wrote: > https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... > components/nacl/loader/nonsfi/nonsfi_sandbox.cc:127: const uint32_t > denied_prot_mask = ~(PROT_READ | PROT_WRITE); > 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. Semantically, that's certainly true. In terms of kernel attack surface, it's always nice to close-off what we can close-off. It's not impossible in theory that there would be a bug exploitable through mmap + PROT_EXEC, but not via mprotect. But it is extremely unlikely. > 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?
https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:113: ErrorCode RestrictMemoryProtection(SandboxBPF* sb, int argno) { On 2014/04/24 16:46:38, jln wrote: > Maybe change this to "RestrictMprotect", without |argno| now? > > Or do you want to keep it for flexibility? No, I'd prefer renaming this to RestrictMprotect. Thanks for the catch. https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:127: const uint32_t denied_prot_mask = ~(PROT_READ | PROT_WRITE); On 2014/04/24 16:49:30, Mark Seaborn wrote: > 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. 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. > 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? 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. https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/247563004/diff/20001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:312: } On 2014/04/24 16:46:38, jln wrote: > Do you mind adding another test with PROT_EXEC | PROT_WRITE for instance? > > Generating BPF filters with bitmasks is notoriously difficult, so I wouldn't > mind a bit more testing or the protections / flags. Added cases for RX and WX.
lgtm
LGTM https://chromiumcodereview.appspot.com/247563004/diff/40001/components/nacl/l... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://chromiumcodereview.appspot.com/247563004/diff/40001/components/nacl/l... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:128: // calls mmap without PROT_EXEC and then add PROT_EXEC by mprotect, "adds"
https://codereview.chromium.org/247563004/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right): https://codereview.chromium.org/247563004/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_sandbox.cc:128: // calls mmap without PROT_EXEC and then add PROT_EXEC by mprotect, On 2014/04/26 00:26:01, Mark Seaborn wrote: > "adds" Done.
The CQ bit was checked by hamaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/247563004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by hamaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/247563004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by hamaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/247563004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by hamaji@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/247563004/60001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Message was sent while issue was closed.
Committed patchset #4 manually as r266453 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
