Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19980003:
NaCl: enable a real seccomp-bpf sandbox. (Closed)
|
Created:
7 years, 5 months ago by jln (very slow on Chromium) Modified:
7 years, 5 months ago Reviewers:
Mark Seaborn CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, native-client-reviews_googlegroups.com, jam, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNaCl: enable a real seccomp-bpf sandbox on x86.
This enables a seccomp-bpf sandbox for NaCl on x86_64 and i386.
This policy is a little bit less tight than Chromium's renderers policy and
should be tightened in the future.
BUG=168812
R=mseaborn@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213268
Patch Set 1 : #
Total comments: 13
Patch Set 2 : Address comments. #
Total comments: 12
Patch Set 3 : Address Mark's comments. Disable the sandbox on ARM. #
Total comments: 2
Patch Set 4 : Closing quote. #Messages
Total messages: 9 (0 generated)
Mark, please take a look! This initial sandbox should work. There is more work to tighten it (and to allow for the custom SIGSYS handler), but I would rather get this out in the field sooner than later. There are large pending refactors to how the seccomp-bpf policies are written and this code in NaCl will benefit from this. I tried to mimic as much as possible the layout of Chromium's renderers policy so that re-factoring opportunities become apparent.
https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... File chrome/nacl/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:22: #if defined(__x86_64__) || defined(__arm__) Can you add a comment about why there's an #if here? i.e. x86-32 has __NR_ipc instead. Or just merge this with the following function, or do "#elif defined(__i386__)" below. Why not put the #if inside the function, so that we don't need an #if later? https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:56: case __NR_accept: It's used by native_client/src/trusted/debug_stub/transport_common.cc https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:61: case __NR_rt_sigtimedwait: Can you comment that this is used by the sigwait() call used by linux/thread_suspension.c? https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:72: case __NR_ioctl: I don't think NaCl uses ioctl(). Does it work to remove this? Maybe this is used for TCP socket settings by the GDB debug stub. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:81: case __NR_sched_get_priority_max: I don't know whether any of these sched_* calls are required. How did you arrive at this list? https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:94: // TODO(jln): look into getting rid of System V shared memory. Does Chromium have the renderer use X's SysV shared memory segments these days? I thought Chromium just does a copy these days instead. If so, we don't need to allow SysV IPC.
Thanks Mark, PTAL! https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... File chrome/nacl/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:22: #if defined(__x86_64__) || defined(__arm__) On 2013/07/22 23:45:24, Mark Seaborn wrote: > Can you add a comment about why there's an #if here? i.e. x86-32 has __NR_ipc > instead. Or just merge this with the following function, or do "#elif > defined(__i386__)" below. > > Why not put the #if inside the function, so that we don't need an #if later? Added a comment. This looks quite ugly indeed, but since it's duplicating a tiny part of code inside content/, I wanted to keep it the same. Upcoming refactors should bring more sanity to this. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:56: case __NR_accept: On 2013/07/22 23:45:24, Mark Seaborn wrote: > It's used by native_client/src/trusted/debug_stub/transport_common.cc Done. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:61: case __NR_rt_sigtimedwait: On 2013/07/22 23:45:24, Mark Seaborn wrote: > Can you comment that this is used by the sigwait() call used by > linux/thread_suspension.c? Done. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:72: case __NR_ioctl: On 2013/07/22 23:45:24, Mark Seaborn wrote: > I don't think NaCl uses ioctl(). Does it work to remove this? Maybe this is > used for TCP socket settings by the GDB debug stub. This needs to be restricted (via parameters) like we did for renderers. TCGETS and FIONREAD are used by glibc I think. I think NaCl may need something more, but I didn't investigate yet. I'll definitely execute on the above TODO, but I would like to get something working soon. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:81: case __NR_sched_get_priority_max: On 2013/07/22 23:45:24, Mark Seaborn wrote: > I don't know whether any of these sched_* calls are required. How did you > arrive at this list? Not sure what needs it (it's something in glibc IIRC), but they are definitely required. https://chromiumcodereview.appspot.com/19980003/diff/5001/chrome/nacl/nacl_sa... chrome/nacl/nacl_sandbox_linux.cc:94: // TODO(jln): look into getting rid of System V shared memory. On 2013/07/22 23:45:24, Mark Seaborn wrote: > Does Chromium have the renderer use X's SysV shared memory segments these days? > I thought Chromium just does a copy these days instead. If so, we don't need to > allow SysV IPC. We don't need Sys V shm in Chromium on Aura, but we still need it on GTK. However, NaCl needs it unconditionally because of platform_qualify/linux/sysv_shm_and_mmap.c I suspect we can get rid of that, but again, I would rather land something soon and improve on it.
LGTM https://codereview.chromium.org/19980003/diff/5001/chrome/nacl/nacl_sandbox_l... File chrome/nacl/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/19980003/diff/5001/chrome/nacl/nacl_sandbox_l... chrome/nacl/nacl_sandbox_linux.cc:94: // TODO(jln): look into getting rid of System V shared memory. On 2013/07/23 00:18:16, Julien Tinnes wrote: > On 2013/07/22 23:45:24, Mark Seaborn wrote: > > Does Chromium have the renderer use X's SysV shared memory segments > > these days? I thought Chromium just does a copy these days instead. > > If so, we don't need to allow SysV IPC. > > We don't need Sys V shm in Chromium on Aura, but we still need it on GTK. > > However, NaCl needs it unconditionally because of > platform_qualify/linux/sysv_shm_and_mmap.c OK, can you put that information into a comment, please? https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/OWNERS File chrome/nacl/OWNERS (right): https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/OWNERS#newcode9 chrome/nacl/OWNERS:9: per-file nacl_sandbox_linux.*=cevans@chromium.org I'm not sure what the protocol is here -- I've heard it's considered rude not to accept someone's request to be added to OWNERS. :-) I haven't reviewed any changes by cevans or had changes reviewed by him. I assume both of you will get a code review from a NaCl person if you're changing what the sandbox accepts. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... File chrome/nacl/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:22: // On arm and x86_64, System V shared memory calls have each their own system Nit: 'arm' -> 'ARM' https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:55: // TODO(jln): NaClGdbDebugStubTest.Breakpoint needs the following 4 system Can you change this to: "NaCl's GDB debug stub uses the following socket system calls". Saying "the following 4" is very likely to get out of date as the code is changed. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:64: // trusted/service_runtime/linux/thread_suspension.cc needs this. Make this "trusted/service_runtime/linux/thread_suspension.c needs sigwait() and is used by NaCl's GDB debug stub". (Note that it's ".c" not ".cc".) https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:70: // NaClAddrSpaceBeforeAlloc needs this. "this" -> "prlimit64"
Thanks Mark! Please, take a sanity check look if you don't mind. I'm struggling to do the proper testing on ARM (and we don't have bots), so I disabled the seccomp-bpf sandbox on ARM for now. I'll continue the testing on ARM while this lands and hopefully enable ARM very soon. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/OWNERS File chrome/nacl/OWNERS (right): https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/OWNERS#newcode9 chrome/nacl/OWNERS:9: per-file nacl_sandbox_linux.*=cevans@chromium.org On 2013/07/23 17:13:03, Mark Seaborn wrote: > I'm not sure what the protocol is here -- I've heard it's considered rude not to > accept someone's request to be added to OWNERS. :-) I haven't reviewed any > changes by cevans or had changes reviewed by him. I assume both of you will get > a code review from a NaCl person if you're changing what the sandbox accepts. I wanted to have two OWNERS here, but how about I add you instead ? (Even though you're already in the general owners it would make it clearer who to ask about this file). I added you instead, let me know if that works. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... File chrome/nacl/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:22: // On arm and x86_64, System V shared memory calls have each their own system On 2013/07/23 17:13:03, Mark Seaborn wrote: > Nit: 'arm' -> 'ARM' Done. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:55: // TODO(jln): NaClGdbDebugStubTest.Breakpoint needs the following 4 system On 2013/07/23 17:13:03, Mark Seaborn wrote: > Can you change this to: "NaCl's GDB debug stub uses the following socket system > calls". Saying "the following 4" is very likely to get out of date as the code > is changed. Done. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:64: // trusted/service_runtime/linux/thread_suspension.cc needs this. On 2013/07/23 17:13:03, Mark Seaborn wrote: > Make this "trusted/service_runtime/linux/thread_suspension.c needs sigwait() and > is used by NaCl's GDB debug stub". (Note that it's ".c" not ".cc".) Done. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:64: // trusted/service_runtime/linux/thread_suspension.cc needs this. On 2013/07/23 17:13:03, Mark Seaborn wrote: > Make this "trusted/service_runtime/linux/thread_suspension.c needs sigwait() and > is used by NaCl's GDB debug stub". (Note that it's ".c" not ".cc".) Done. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:70: // NaClAddrSpaceBeforeAlloc needs this. On 2013/07/23 17:13:03, Mark Seaborn wrote: > "this" -> "prlimit64" Done. https://codereview.chromium.org/19980003/diff/15001/chrome/nacl/nacl_sandbox_... chrome/nacl/nacl_sandbox_linux.cc:70: // NaClAddrSpaceBeforeAlloc needs this. On 2013/07/23 17:13:03, Mark Seaborn wrote: > "this" -> "prlimit64" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/19980003/31001
https://chromiumcodereview.appspot.com/19980003/diff/31001/chrome/nacl/nacl_s... File chrome/nacl/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/19980003/diff/31001/chrome/nacl/nacl_s... chrome/nacl/nacl_sandbox_linux.cc:74: // used by NaCl's GDB debug stub." Remove closing quote here.
https://chromiumcodereview.appspot.com/19980003/diff/31001/chrome/nacl/nacl_s... File chrome/nacl/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/19980003/diff/31001/chrome/nacl/nacl_s... chrome/nacl/nacl_sandbox_linux.cc:74: // used by NaCl's GDB debug stub." On 2013/07/23 23:02:17, Mark Seaborn wrote: > Remove closing quote here. Oops, done, thanks!
Message was sent while issue was closed.
Committed patchset #4 manually as r213268 (presubmit successful). |
