Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
670603002:
Linux sandbox: Tighten up the NaCl sandbox policy. (Closed)
|
Created:
6 years, 2 months ago by rickyz (no longer on Chrome) Modified:
6 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionLinux sandbox: Tighten up the NaCl sandbox policy.
Previously, we allowed socket syscalls which were only needed by the
NaCl gdb stub. Now, we only allow these syscalls when the
--enable-nacl-debug flag is present.
Also restricts cross-process interaction for sched_* syscalls now that
non-crashing SIGSYS handlers are allowed under NaCl.
BUG=270914, 413855
Committed: https://crrev.com/3638a21d79c71ed0b18c1591ce2caea86cfb1aba
Cr-Commit-Position: refs/heads/master@{#301982}
Patch Set 1 #Patch Set 2 : Also restrict sched_* #Patch Set 3 : Disable the sandbox for debug stub tests. #Patch Set 4 : Add missing include. #
Total comments: 6
Patch Set 5 : Allow syscalls when nacl debug stub is enabled. #Patch Set 6 : Forward the enable-nacl-debug flag through. #
Total comments: 5
Patch Set 7 : Make NaClBPFSandboxPolicy subclass sandbox::BaselinePolicy. #Patch Set 8 : Have a pid member instead. #
Total comments: 3
Patch Set 9 : Respond to comments #Patch Set 10 : Rebase #
Total comments: 10
Patch Set 11 : Respond to more comments. #Patch Set 12 : Add check on policy_pid #
Total comments: 1
Patch Set 13 : Switch to DCHECK #Patch Set 14 : Rebase #
Messages
Total messages: 28 (5 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org, mseaborn@chromium.org
Here's a change to block syscalls that were previously only enabled for testing the gdb stub (and also to restrict sched_* calls now that we support non-crashing SIGSYS). I noticed that the need to use --no-sandbox was considered a bug on windows (crbug.com/265624), which I don't really understand - isn't it good to not loosen the sandbox for a debugging feature?
https://codereview.chromium.org/670603002/diff/60001/chrome/browser/nacl_host... File chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc (right): https://codereview.chromium.org/670603002/diff/60001/chrome/browser/nacl_host... chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc:34: // The debug stub requires --no-sandbox. Well, we don't *want* using the debug stub to require --no-sandbox. That makes it less secure, and more awkward to use, and it would require documenting. https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (left): https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:60: case __NR_accept: Could we conditionalise these based on whether the GDB debug stub is enabled? https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:65: // trusted/service_runtime/linux/thread_suspension.c needs sigwait() and is We may end up using thread suspension outside of the debug stub. e.g. For dynamic code unloading (see https://code.google.com/p/nativeclient/issues/detail?id=2817).
https://codereview.chromium.org/670603002/diff/60001/chrome/browser/nacl_host... File chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc (right): https://codereview.chromium.org/670603002/diff/60001/chrome/browser/nacl_host... chrome/browser/nacl_host/test/gdb_debug_stub_browsertest.cc:34: // The debug stub requires --no-sandbox. On 2014/10/21 22:08:40, Mark Seaborn wrote: > Well, we don't *want* using the debug stub to require --no-sandbox. That makes > it less secure, and more awkward to use, and it would require documenting. Ah, upon further inspection, it looks like the recommendation at http://www.chromium.org/nativeclient/how-tos/debugging-documentation/debuggin... applies to windows only. Agreed that allowing those syscalls if --enable-nacl-debug is enabled is better - I switched to doing that instead. https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (left): https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:60: case __NR_accept: On 2014/10/21 22:08:40, Mark Seaborn wrote: > Could we conditionalise these based on whether the GDB debug stub is enabled? Yeah, this sounds like a better approach, done. https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:65: // trusted/service_runtime/linux/thread_suspension.c needs sigwait() and is On 2014/10/21 22:08:40, Mark Seaborn wrote: > We may end up using thread suspension outside of the debug stub. e.g. For > dynamic code unloading (see > https://code.google.com/p/nativeclient/issues/detail?id=2817). Ah, good to know - I left this one enabled by default, even when the debug stub is not enabled.
Excellent Ricky, thanks! Just a couple of nits. https://chromiumcodereview.appspot.com/670603002/diff/100001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/670603002/diff/100001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:61: if (command_line->HasSwitch(switches::kEnableNaClDebug)) { Could you instead have a class variable to record this property and check this when we instantiate the class instead? as-is, we'll repeat this for every syscall and also it go against guaranteeing that calling EvaluateSyscall() twice with the same parameters will yield the same result. https://chromiumcodereview.appspot.com/670603002/diff/100001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:117: return sandbox::RestrictSchedTarget(getpid(), sysno); Why not policy_pid() ? (As a general rule, we should not query "the environment" in EvaluateSyscall() as it runs the risk that calling EvaluateSyscall() twice with the same parameter would not yield the same result).
https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:61: if (command_line->HasSwitch(switches::kEnableNaClDebug)) { On 2014/10/23 17:51:55, jln wrote: > Could you instead have a class variable to record this property and check this > when we instantiate the class instead? > > as-is, we'll repeat this for every syscall and also it go against guaranteeing > that calling EvaluateSyscall() twice with the same parameters will yield the > same result. Done. https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:117: return sandbox::RestrictSchedTarget(getpid(), sysno); On 2014/10/23 17:51:55, jln wrote: > Why not policy_pid() ? > > (As a general rule, we should not query "the environment" in EvaluateSyscall() > as it runs the risk that calling EvaluateSyscall() twice with the same parameter > would not yield the same result). policy_pid isn't exposed via sandbox::bpf_dsl::SandboxBPFDSLPolicy, so I changed NaClBPFSandboxPolicy to subclass public sandbox::BaselinePolicy instead of it as a member and forwarding the functions explicitly. What do you think of this?
https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:42: : baseline_policy_(content::GetBPFSandboxBaselinePolicy()) {} NaCl's sandbox policy was based on content/'s baseline policy. The fact that in practice it matches the //sandbox's baseline policy is an implementation detail. I could be convinced to get rid of this model, but I suspect that allowing for content's baseline policy to diverge from //sandbox's baseline policy will be useful flexibility especially as we get more users such as Mojo. Could you instead simply create a new current_pid_ member that gets initialized at construction time?
On 2014/10/23 22:37:20, jln wrote: > https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... > File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): > > https://codereview.chromium.org/670603002/diff/100001/components/nacl/loader/... > components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:42: : > baseline_policy_(content::GetBPFSandboxBaselinePolicy()) {} > NaCl's sandbox policy was based on content/'s baseline policy. The fact that in > practice it matches the //sandbox's baseline policy is an implementation detail. > > I could be convinced to get rid of this model, but I suspect that allowing for > content's baseline policy to diverge from //sandbox's baseline policy will be > useful flexibility especially as we get more users such as Mojo. > > Could you instead simply create a new current_pid_ member that gets initialized > at construction time? Done, how's this?
lgtm (please make sure Mark approves it as well). https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:48: current_pid_ = getpid(); Nit: how about putting this in the initializer list? https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:60: pid_t current_pid_; policy_pid_ to be consistent ?
https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:80: #endif style: add "default:" with a // Fallthrough comment.
On 2014/10/24 20:48:11, jln wrote: > https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... > File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): > > https://codereview.chromium.org/670603002/diff/140001/components/nacl/loader/... > components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:80: #endif > style: add "default:" with a // Fallthrough comment. Responded to the rest of your comments, but wasn't quite sure what you mean by the fallthrough comment - I added a default: break to the switch. Thanks, Ricky
> Responded to the rest of your comments, but wasn't quite sure what you mean by > the fallthrough comment - I added a default: break to the switch. Yeah, that's actually better, thanks!
LGTM. Will Bugdroid recognise "BUG=270914, 413855", BTW, or do these need to be on two separate BUG= lines? https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (left): https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:67: // used by NaCl's GDB debug stub. I think it would be worth keeping this comment and disabling rt_sigtimedwait(), to reduce the attack surface. We can always change this policy if it turns out we do need thread suspension for other reasons. My earlier comment (https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s...) was just for context, really. https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:49: enable_nacl_debug_ = command_line->HasSwitch(switches::kEnableNaClDebug); Maybe add a comment like: // nacl_process_host.cc doesn't always enable the debug stub when // kEnableNaClDebug is passed, but it's OK to enable the extra syscalls // whenever kEnableNaClDebug is passed. https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:70: // allow then when --enable-nacl-debug is specified. "then" -> "them" https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:127: return sandbox::RestrictSchedTarget(policy_pid_, sysno); Why not call getpid() here? I'm not sure why you're caching it on the object. Maybe add a comment if there's a reason for caching it?
https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:127: return sandbox::RestrictSchedTarget(policy_pid_, sysno); On 2014/10/28 00:30:14, Mark Seaborn wrote: > Why not call getpid() here? I'm not sure why you're caching it on the object. > Maybe add a comment if there's a reason for caching it? I actually requested that (see previous comments). The main reason is that EvaluateSyscall(X) needs to be guaranteed to always return the same thing. It is good practice to not call functions that depend on the environment from there. The policy is computed as being "bound" to a pid (which we happen to choose as the current one). This is much better expressed by a member variable for the policy itself. Moreover, and for some historical context, getpid() is very problematic because of pid caching (for processes created with clone). We solved this once and for all in the "baseline" policy by using syscall(SYS_getppid); and putting that in a member variable. We can unfortunately not use it here, due to the fact that we have to request that policy from content/ and the interface I created for that doesn't have that notion.
On 2014/10/28 00:30:14, Mark Seaborn wrote: > LGTM. Will Bugdroid recognise "BUG=270914, 413855", BTW, or do these need to be > on two separate BUG= lines? Yes, I'm pretty sure it does (it's a common pattern).
https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/670603002/diff/180001/components/nacl/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:61: pid_t policy_pid_; const
Thanks for the comments - will CQ once try bots pass https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (left): https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:67: // used by NaCl's GDB debug stub. On 2014/10/28 00:30:14, Mark Seaborn wrote: > I think it would be worth keeping this comment and disabling rt_sigtimedwait(), > to reduce the attack surface. We can always change this policy if it turns out > we do need thread suspension for other reasons. My earlier comment > (https://codereview.chromium.org/670603002/diff/60001/components/nacl/loader/s...) > was just for context, really. Ah, OK - moved it into the list of syscalls only allowed under kEnableNaClDebug. https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:49: enable_nacl_debug_ = command_line->HasSwitch(switches::kEnableNaClDebug); On 2014/10/28 00:30:14, Mark Seaborn wrote: > Maybe add a comment like: > > // nacl_process_host.cc doesn't always enable the debug stub when > // kEnableNaClDebug is passed, but it's OK to enable the extra syscalls > // whenever kEnableNaClDebug is passed. Done. https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:61: pid_t policy_pid_; On 2014/10/29 18:44:45, jln (OOO til 29th) wrote: > const Done. https://codereview.chromium.org/670603002/diff/180001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:127: return sandbox::RestrictSchedTarget(policy_pid_, sysno); On 2014/10/29 18:40:16, jln (OOO til 29th) wrote: > On 2014/10/28 00:30:14, Mark Seaborn wrote: > > Why not call getpid() here? I'm not sure why you're caching it on the object. > > > Maybe add a comment if there's a reason for caching it? > > I actually requested that (see previous comments). The main reason is that > EvaluateSyscall(X) needs to be guaranteed to always return the same thing. It is > good practice to not call functions that depend on the environment from there. > The policy is computed as being "bound" to a pid (which we happen to choose as > the current one). This is much better expressed by a member variable for the > policy itself. > > Moreover, and for some historical context, getpid() is very problematic because > of pid caching (for processes created with clone). We solved this once and for > all in the "baseline" policy by using syscall(SYS_getppid); and putting that in > a member variable. We can unfortunately not use it here, due to the fact that we > have to request that policy from content/ and the interface I created for that > doesn't have that notion. I added a CHECK in EvaluateSyscall - hopefully that documents the the requirement (and also somewhat justifies the member variable).
https://codereview.chromium.org/670603002/diff/220001/components/nacl/loader/... File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): https://codereview.chromium.org/670603002/diff/220001/components/nacl/loader/... components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:74: CHECK_EQ(policy_pid_, syscall(__NR_getpid)); If you would rather keep this, could you make it a DCHECK ? (This is going to be called thousand of times). You can also remove it. There are plenty of checks / tests to make sure that policies are instantiated and used in the same process: see the DCHECK in BaselinePolicy.
On 2014/10/29 21:40:07, jln (OOO til 29th) wrote: > https://codereview.chromium.org/670603002/diff/220001/components/nacl/loader/... > File components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc (right): > > https://codereview.chromium.org/670603002/diff/220001/components/nacl/loader/... > components/nacl/loader/sandbox_linux/nacl_bpf_sandbox_linux.cc:74: > CHECK_EQ(policy_pid_, syscall(__NR_getpid)); > If you would rather keep this, could you make it a DCHECK ? (This is going to be > called thousand of times). > > You can also remove it. There are plenty of checks / tests to make sure that > policies are instantiated and used in the same process: see the DCHECK in > BaselinePolicy. Aha, switched to a DCHECK.
The CQ bit was checked by rickyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/670603002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75479) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/80506) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rickyz@chromium.org
The CQ bit was checked by rickyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/670603002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3638a21d79c71ed0b18c1591ce2caea86cfb1aba Cr-Commit-Position: refs/heads/master@{#301982} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
