Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
888903004:
Non-SFI mode:Suid sandbox. (Closed)
|
Created:
5 years, 10 months ago by hidehiko Modified:
5 years, 9 months ago Reviewers:
tzik, jam, dmichael (off chromium), rickyz (no longer on Chrome), jln (very slow on Chromium), Mark Seaborn, Nick Bray (chromium), mdempsky CC:
chromium-reviews, rickyz+watch_chromium.org, hamaji, Junichi Uekawa, mazda, satorux1, rickyz (no longer on Chrome), Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNon-SFI mode: Suid sandbox.
This CL enables suid sandbox on nacl_helper_nonsfi.
BUG=358465
TEST=Ran trybots. Ran Non-SFI NaCl app with nacl_helper_nonsfi.
Committed: https://crrev.com/7d7dcec900cc0c148237307a79b9471a6459f2e5
Cr-Commit-Position: refs/heads/master@{#319845}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Rebase. #Patch Set 3 : Add --disable-namespace-sandbox flag to browser tests temporarily. #
Total comments: 8
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : Rebase #
Messages
Total messages: 47 (17 generated)
Patchset #1 (id:1) has been deleted
hidehiko@chromium.org changed reviewers: + jln@chromium.org, mdempsky@chromium.org, mseaborn@chromium.org, ncbray@chromium.org
PTAL. I know we still have discussion about the future plan of the sandbox/ implementation (especially libcap related stuff), but I believe it is closer enough to make an agreement. Also, this CL is enough straightforward and small enough. So please let me start the code review in parallel to save the time. Please feel free to hold on LGTM if you'd like to complete the discussion beforehand (and I'm actually planning to wait submitting for the agreement), but I'd really appreciate if you have any concerns in advance, too. Thanks! - hidehiko
I didn't look yet, but wanted to add rickyz@. Note that we want to ship the namespace sandbox in M42, see: https://chromiumcodereview.appspot.com/897723005/. We want to eventually kill the setuid sandbox. The setuid sandbox has one advantage: it requires very little code in the client (it just needs to IPC a separate process), which is not shared by the NS sandbox. I don't think the hurdle will be huge to make the NS sandbox build with newlib, but it's a task to think about.
OK, this is a good test case for "expanding the reviewer pool". With multiple people listed as reviewers, I do not know what you expect from me. Please be explicit about what you expect from various reviewers, particularly if you don't (yet) have a review history with one of them. I will assume you expect this is an FYI / getting to know the code review for me, unless you say otherwise. Mechanically it looks good, but it also points out a lot of missing context I do not have. This is good and bad. There's a lot of knowledge that needs to be shared, but it is now more clear what is not known. In terms of bringing multiple reviewers up to speed, it is now clear we need to talk about scope. What are all the subcomponents that we need to know about? Are there any pieces a particular reviewer could specialize in? (Hard to say how to distribute the load without knowing much about the load.) I'll try to talk to Mark on Friday (Mark?) to figure as much of this out as possible locally, and then we can email or VC to figure out the remaining questions.
lgtm Do we need a BUILD.gn for sandbox_nacl_nonsfi? Looks like NaCl is currently gyp-only?
On 2015/02/06 03:48:10, mdempsky wrote: > lgtm > > Do we need a BUILD.gn for sandbox_nacl_nonsfi? Looks like NaCl is currently > gyp-only? dpranke@ is working on the GN+Chrome+NaCl build. He is focusing on x64 desktop Linux, so I suspect the CL is not a concern... at least right now. cced him as FYI.
On 2015/02/06 05:35:45, Nick Bray (chromium) wrote: > On 2015/02/06 03:48:10, mdempsky wrote: > > lgtm > > > > Do we need a BUILD.gn for sandbox_nacl_nonsfi? Looks like NaCl is currently > > gyp-only? > > dpranke@ is working on the GN+Chrome+NaCl build. He is focusing on x64 desktop > Linux, so I suspect the CL is not a concern... at least right now. cced him as > FYI. Thank you for review, and comments. To jln@, rickyz@: Oh, good to know. I didn't think NS sandbox is enabled so soon. Anyway, we'll look into the NS sandbox incl. libcap. I'll keep you and rickyz@ updated about it. BTW, though, even if we can go with libcap, I'd like to enable suid sandbox separately. From library building perspective, building the suid sandbox and building the NS sandbox look separate/independent tasks. I prefer to make a progress step by step. To ncbray@: Thank you for your comment, and sorry for my poor explanation. Please feel free to think this is a kind of FYI for now. Actually, as NS sandbox seems to be enabled soon, I needed to take a look into it a bit more details, for all members including Sandbox team, NaCl team and ARC team on the same page, before moving forward. So, I very appreciated your offer to help, but please feel free to suspend the review, for now. Sorry for bothering you about the confusion. However, at the same time, I believe this is a good chance to share what the actual BMM work is with (not only Mark but also other) NaCl team members. I'd appreciate if you could find a time to have a brief chat with Mark about the background and the code review process, etc. To mdempsky@: Thank you for quick review! As for .gn, as Nick said, gyp is only supported in newlib-linked nacl_helper for Non-SFI. I'll update the CL somehow and let you know, when we can move forward. Again, thank you for your review! - hidehiko
Hidehiko, please proceed if mdempsky@ reviewed it, no need to wait for me!
LGTM https://chromiumcodereview.appspot.com/888903004/diff/20001/components/nacl/l... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/888903004/diff/20001/components/nacl/l... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:102: void NaClSandbox::CheckForExpectedNumberOfOpenFds() { Technically, CheckForExpectedNumberOfOpenFds() isn't part of the layer-two (seccomp) sandbox. This check is useful when we only have the layer-one (SUID) sandbox enabled too. Does CheckForExpectedNumberOfOpenFds() fail at run-time in nacl_helper_nonsfi, or does it fail to link? Maybe you could say in the comment which of the two applies. https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... File sandbox/sandbox_nacl_nonsfi.gyp (left): https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... sandbox/sandbox_nacl_nonsfi.gyp:30: # TODO(hidehiko): Add sandbox code. So Git regards this file as copied from content/content_nacl_nonsfi.gyp. Should you remove this TODO comment from there? I'm guessing that the new sandbox_nacl_nonsfi.gyp covers the sandbox code that this TODO comment referred to.
https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... File sandbox/sandbox_nacl_nonsfi.gyp (left): https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... sandbox/sandbox_nacl_nonsfi.gyp:30: # TODO(hidehiko): Add sandbox code. On 2015/02/06 20:15:03, Mark Seaborn wrote: > So Git regards this file as copied from content/content_nacl_nonsfi.gyp. Should > you remove this TODO comment from there? I'm guessing that the new > sandbox_nacl_nonsfi.gyp covers the sandbox code that this TODO comment referred > to. I think the comment may be referring to content/common/sandbox_linux, some of which will be needed for setting up the layer-2 sandbox (seccomp-bpf).
So, now we are on the same page, I'd like to move forward, and would like to check this in. Rebased on ToT. Mark, PTAL, as components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc needed to be edited? Thanks, - hidehiko https://chromiumcodereview.appspot.com/888903004/diff/20001/components/nacl/l... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/888903004/diff/20001/components/nacl/l... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:102: void NaClSandbox::CheckForExpectedNumberOfOpenFds() { On 2015/02/06 20:15:03, Mark Seaborn wrote: > Technically, CheckForExpectedNumberOfOpenFds() isn't part of the layer-two > (seccomp) sandbox. This check is useful when we only have the layer-one (SUID) > sandbox enabled too. > > Does CheckForExpectedNumberOfOpenFds() fail at run-time in nacl_helper_nonsfi, > or does it fail to link? Maybe you could say in the comment which of the two > applies. No, but it is only called from InitializeLayerTwoSandbox() so I exclude it. Ditto for above methods. (Note that some of above methods contains PNaCl toolchian unsupported code. I'll address it later). https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... File sandbox/sandbox_nacl_nonsfi.gyp (left): https://chromiumcodereview.appspot.com/888903004/diff/20001/sandbox/sandbox_n... sandbox/sandbox_nacl_nonsfi.gyp:30: # TODO(hidehiko): Add sandbox code. On 2015/02/09 06:12:39, mdempsky wrote: > On 2015/02/06 20:15:03, Mark Seaborn wrote: > > So Git regards this file as copied from content/content_nacl_nonsfi.gyp. > Should > > you remove this TODO comment from there? I'm guessing that the new > > sandbox_nacl_nonsfi.gyp covers the sandbox code that this TODO comment > referred > > to. > > I think the comment may be referring to content/common/sandbox_linux, some of > which will be needed for setting up the layer-2 sandbox (seccomp-bpf). Yes, and some content/public/common files, too.
hidehiko@chromium.org changed reviewers: + rickyz@chromium.org
Thanks to Ricky, I fixed the browser_tests failures. Mark, PTAL for the changes?
LGTM https://chromiumcodereview.appspot.com/888903004/diff/60001/chrome/test/nacl/... File chrome/test/nacl/nacl_browsertest_util.cc (right): https://chromiumcodereview.appspot.com/888903004/diff/60001/chrome/test/nacl/... chrome/test/nacl/nacl_browsertest_util.cc:300: // TODO(hidehiko): Remove this flag, when namespace sandbox is supported. "...supported by nacl_helper_nonsfi"? (Same below.) https://chromiumcodereview.appspot.com/888903004/diff/60001/components/nacl/l... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/888903004/diff/60001/components/nacl/l... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:149: // Currently namespace sandbox is not yet supported on nacl_helper_nonsfi. It's odd putting the comment inside the previous if() block. Would this be cleaner? -- if (...) { ... } // comment... #if !defined(OS_NACL_NONSFI) else { ... } #endif https://chromiumcodereview.appspot.com/888903004/diff/60001/components/nacl/l... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:167: // InitializeLayerTwoSandbox(). Enable them at once. Nit: "Enable them at once" -> "We can enable them together"? "At once" can mean "immediately" as well as "together", which isn't what you mean here, I think. https://chromiumcodereview.appspot.com/888903004/diff/60001/sandbox/sandbox_n... File sandbox/sandbox_nacl_nonsfi.gyp (right): https://chromiumcodereview.appspot.com/888903004/diff/60001/sandbox/sandbox_n... sandbox/sandbox_nacl_nonsfi.gyp:28: 'linux/services/proc_util.cc', If this is a subset of a native-Linux target, maybe comment that here?
hidehiko@chromium.org changed reviewers: + dmichael@chromium.org, jam@chromium.org
Thank you for review, Mark. +jam@, dmichael@. jam@, could you review content/content_nacl_nonsfi.gyp as an OWNER? Also, is it ok to add the file to the OWNER-review-skip-whitelist for this kind of edit, similar to what we allow for *.gypi files in content/OWNERS? dmichael@, could you review chrome/test/ppapi/... as an OWNER? Thanks. - hidehiko https://codereview.chromium.org/888903004/diff/60001/chrome/test/nacl/nacl_br... File chrome/test/nacl/nacl_browsertest_util.cc (right): https://codereview.chromium.org/888903004/diff/60001/chrome/test/nacl/nacl_br... chrome/test/nacl/nacl_browsertest_util.cc:300: // TODO(hidehiko): Remove this flag, when namespace sandbox is supported. On 2015/03/04 19:32:50, Mark Seaborn wrote: > "...supported by nacl_helper_nonsfi"? (Same below.) Done. https://codereview.chromium.org/888903004/diff/60001/components/nacl/loader/s... File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/888903004/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:149: // Currently namespace sandbox is not yet supported on nacl_helper_nonsfi. On 2015/03/04 19:32:50, Mark Seaborn wrote: > It's odd putting the comment inside the previous if() block. > > Would this be cleaner? -- > > if (...) { > ... > } > // comment... > #if !defined(OS_NACL_NONSFI) > else { > ... > } > #endif Done. https://codereview.chromium.org/888903004/diff/60001/components/nacl/loader/s... components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:167: // InitializeLayerTwoSandbox(). Enable them at once. On 2015/03/04 19:32:50, Mark Seaborn wrote: > Nit: "Enable them at once" -> "We can enable them together"? > > "At once" can mean "immediately" as well as "together", which isn't what you > mean here, I think. Great to know. Thanks! https://codereview.chromium.org/888903004/diff/60001/sandbox/sandbox_nacl_non... File sandbox/sandbox_nacl_nonsfi.gyp (right): https://codereview.chromium.org/888903004/diff/60001/sandbox/sandbox_nacl_non... sandbox/sandbox_nacl_nonsfi.gyp:28: 'linux/services/proc_util.cc', On 2015/03/04 19:32:50, Mark Seaborn wrote: > If this is a subset of a native-Linux target, maybe comment that here? Done.
chrome/test/ppapi lgtm with comment https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:1354: // by nacl_helper_nonsfi. Do you have a bug you could reference, please? https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.cc:448: // by nacl_helper_nonsfi. bug reference, please here and below.
On 2015/03/05 05:26:53, hidehiko wrote: > Thank you for review, Mark. > > +jam@, dmichael@. > > jam@, could you review content/content_nacl_nonsfi.gyp as an OWNER? Also, is it > ok to add the file to the OWNER-review-skip-whitelist for this kind of edit, > similar to what we allow for *.gypi files in content/OWNERS? sure > > dmichael@, could you review chrome/test/ppapi/... as an OWNER? > > Thanks. > - hidehiko lgtm with comment https://codereview.chromium.org/888903004/diff/80001/content/content_nacl_non... File content/content_nacl_nonsfi.gyp (left): https://codereview.chromium.org/888903004/diff/80001/content/content_nacl_non... content/content_nacl_nonsfi.gyp:29: 'public/common/send_zygote_child_ping_linux.h', why are you removing this? we need headers to be listed if they're depended on for analyze (the step on trybots that figures out which targets to build/teest) to work correctly
Thank you for review. Submitting. jam@, as for adding white-list to OWNERS file, please let me send another CL for that. https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_browsertest.cc:1354: // by nacl_helper_nonsfi. On 2015/03/05 18:17:01, dmichael wrote: > Do you have a bug you could reference, please? Done. https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/888903004/diff/80001/chrome/test/ppapi/ppapi_... chrome/test/ppapi/ppapi_test.cc:448: // by nacl_helper_nonsfi. On 2015/03/05 18:17:01, dmichael wrote: > bug reference, please here and below. Done. https://codereview.chromium.org/888903004/diff/80001/content/content_nacl_non... File content/content_nacl_nonsfi.gyp (left): https://codereview.chromium.org/888903004/diff/80001/content/content_nacl_non... content/content_nacl_nonsfi.gyp:29: 'public/common/send_zygote_child_ping_linux.h', On 2015/03/05 18:30:22, jam wrote: > why are you removing this? we need headers to be listed if they're depended on > for analyze (the step on trybots that figures out which targets to build/teest) > to work correctly This is input for the build_nexe.py rather than host toolchain. So, according to mark, header files are actually just filtered. I just followed Mark suggestion to remove header files for this kind of cases.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdempsky@chromium.org, mseaborn@chromium.org, jam@chromium.org, dmichael@chromium.org Link to the patchset: https://codereview.chromium.org/888903004/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was unchecked by hidehiko@chromium.org
FYI: I updated commit message. Now Layer-one sandbox means both suid/namespace sandbox. So, to be clear, I used "Suid sandbox". Also, precise32 build bot is replaced by trusty32. Changed the CQ_EXTRA_TRYBOTS. Submitting, again. On 2015/03/06 09:15:57, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_trusty32_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_trusty32_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_trusty32_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
hidehiko@chromium.org changed reviewers: + tzik@chromium.org
Removed: CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_trusty32_rel,linux_arm from commit message. Unrelated tests in linux_chromium_trusty32_rel look failing for a while (already 4 days). As it is not the CQ blocker, it'd take a bit more time for them to be fixed. I asked tzik@ (the expert around the failing tests) to fix them. Again sending to CQ.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888903004/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7d7dcec900cc0c148237307a79b9471a6459f2e5 Cr-Commit-Position: refs/heads/master@{#319845} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
