Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14954012:
Restrict mmap(2) flags for x64. (Closed)
|
Created:
7 years, 7 months ago by Chris Evans Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRestrict mmap(2) flags for x64.
BUG=241220
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 1
Messages
Total messages: 8 (0 generated)
lgtm with quite a few nits. Unless there is a rush, try to have someone do a last-look after addressing the nits. Also of course make sure you test on ARM (still no good test coverage there). I'm wondering if we shouldn't roll this a little bit more cautiously by putting the additional restriction in renderer at first (and leaving the baseline policy as-is). Moreover, the baseline policy's goal is to be a one-size-fits-all as much as possible, so even if you want to restrict other process types, it may be worth doing so in their respective policies. For instance, I think NaCl might need more flags and I would love the baseline to still apply there. https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:667: case __NR_mmap: Nit: if we keep it this way, let's move mmap and mmap2 down to keep alphabetical ordering! https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1243: uint32_t mask = ~(MAP_SHARED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK | want to rename that variable denied_mask to make things more clear? https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1245: return sandbox->Cond(3, ErrorCode::TP_32BIT, ErrorCode::OP_HAS_ANY_BITS, Nit: I guess we should have provided a OP_HAS_NO_BITS helper, it's a bit confusing that we're inverting the order of what's allowed / denied in comparison to the other similar functions. Can you try to make things a little be more clear by having denied_mask on the previous line and the two Trap and ERR_ALLOWED each on their own line ? https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1260: switch (sysno) { If you want to make this a switch, make sure you start the switch on top. Then you'd need a default: case as per style guide requirements. I wonder if it's worth it though, I found the if () return logic fairly readable. Your call.
https://codereview.chromium.org/14954012/diff/4001/content/common/sandbox_sec... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/14954012/diff/4001/content/common/sandbox_sec... content/common/sandbox_seccomp_bpf_linux.cc:1266: #if defined(__x86_64__) Arg, had missed one important nit: #ifdef are really only used in this file for when things can't compile. Not to define the policy. Use IsArchitectureArm() etc to define the policy instead. Currently, this is very confusing!
https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... content/common/sandbox_seccomp_bpf_linux.cc:1280: if (IsBaselinePolicyAllowed(sysno)) { Another remark: The goal of putting things *above* IsBaseLinePolicyAllowed() is that we do deny thing that would otherwise be allowed. You can put all of these (which are now defined by default in the baseline policy) below.
On 2013/05/15 20:10:13, jln wrote: > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1280: if > (IsBaselinePolicyAllowed(sysno)) { > Another remark: > > The goal of putting things *above* IsBaseLinePolicyAllowed() is that we do deny > thing that would otherwise be allowed. You can put all of these (which are now > defined by default in the baseline policy) below. Fixed, thanks.
On 2013/05/15 20:03:52, Julien Tinnes wrote: > https://codereview.chromium.org/14954012/diff/4001/content/common/sandbox_sec... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://codereview.chromium.org/14954012/diff/4001/content/common/sandbox_sec... > content/common/sandbox_seccomp_bpf_linux.cc:1266: #if defined(__x86_64__) > Arg, had missed one important nit: > > #ifdef are really only used in this file for when things can't compile. Not to > define the policy. Use IsArchitectureArm() etc to define the policy instead. > > Currently, this is very confusing! Ah yes, good catch. Got it, I think. Next PS upcoming.
On 2013/05/15 20:01:05, Julien Tinnes wrote: > lgtm with quite a few nits. Unless there is a rush, try to have someone do a > last-look after addressing the nits. > > Also of course make sure you test on ARM (still no good test coverage there). > > I'm wondering if we shouldn't roll this a little bit more cautiously by putting > the additional restriction in renderer at first (and leaving the baseline policy > as-is). > > Moreover, the baseline policy's goal is to be a one-size-fits-all as much as > possible, so even if you want to restrict other process types, it may be worth > doing so in their respective policies. For instance, I think NaCl might need > more flags and I would love the baseline to still apply there. I don't think NaCl needs more flags. Which one did you have in mind? I've done pretty extensive searching for the denied flags, all of which are pretty esoteric and unused. > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:667: case __NR_mmap: > Nit: if we keep it this way, let's move mmap and mmap2 down to keep alphabetical > ordering! > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1243: uint32_t mask = ~(MAP_SHARED | > MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK | > want to rename that variable denied_mask to make things more clear? Done. > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1245: return sandbox->Cond(3, > ErrorCode::TP_32BIT, ErrorCode::OP_HAS_ANY_BITS, > Nit: > > I guess we should have provided a OP_HAS_NO_BITS helper, it's a bit confusing > that we're inverting the order of what's allowed / denied in comparison to the > other similar functions. Agree. > > Can you try to make things a little be more clear by having denied_mask on the > previous line and the two Trap and ERR_ALLOWED each on their own line ? Done. > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1260: switch (sysno) { > If you want to make this a switch, make sure you start the switch on top. > > Then you'd need a default: case as per style guide requirements. > > I wonder if it's worth it though, I found the if () return logic fairly > readable. Your call. Agree. Reverted to if() logic.
On 2013/05/15 20:01:05, Julien Tinnes wrote: > lgtm with quite a few nits. Unless there is a rush, try to have someone do a > last-look after addressing the nits. > > Also of course make sure you test on ARM (still no good test coverage there). > > I'm wondering if we shouldn't roll this a little bit more cautiously by putting > the additional restriction in renderer at first (and leaving the baseline policy > as-is). > > Moreover, the baseline policy's goal is to be a one-size-fits-all as much as > possible, so even if you want to restrict other process types, it may be worth > doing so in their respective policies. For instance, I think NaCl might need > more flags and I would love the baseline to still apply there. > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > File content/common/sandbox_seccomp_bpf_linux.cc (right): > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:667: case __NR_mmap: > Nit: if we keep it this way, let's move mmap and mmap2 down to keep alphabetical > ordering! > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1243: uint32_t mask = ~(MAP_SHARED | > MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK | > want to rename that variable denied_mask to make things more clear? > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1245: return sandbox->Cond(3, > ErrorCode::TP_32BIT, ErrorCode::OP_HAS_ANY_BITS, > Nit: > > I guess we should have provided a OP_HAS_NO_BITS helper, it's a bit confusing > that we're inverting the order of what's allowed / denied in comparison to the > other similar functions. > > Can you try to make things a little be more clear by having denied_mask on the > previous line and the two Trap and ERR_ALLOWED each on their own line ? > > https://codereview.chromium.org/14954012/diff/1/content/common/sandbox_seccom... > content/common/sandbox_seccomp_bpf_linux.cc:1260: switch (sysno) { > If you want to make this a switch, make sure you start the switch on top. > > Then you'd need a default: case as per style guide requirements. > > I wonder if it's worth it though, I found the if () return logic fairly > readable. Your call. Sorry, I've made an error, we'll continue on https://codereview.chromium.org/15112008 |
