Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
868233011:
Start all children in their own PID namespace. (Closed)
|
Created:
5 years, 10 months ago by rickyz (no longer on Chrome) Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, rickyz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart all children in their own PID namespace.
BUG=460972
Committed: https://crrev.com/4557699fbb30704f76c68b906d2656d2e322572c
Cr-Commit-Position: refs/heads/master@{#322660}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Respond to comments. #Patch Set 3 : Changes for the new DropAllCapabilities API. #Patch Set 4 : Only drop capabilities if we have any. #
Total comments: 17
Patch Set 5 : Rebase. #Patch Set 6 : Respond to some comments. #
Total comments: 13
Patch Set 7 : Respond to more comments. #
Total comments: 9
Patch Set 8 : Add CHECK test/stuff. #Patch Set 9 : Oops, remove extra period. #Patch Set 10 : Use push_back instead of confusing syntax. #
Total comments: 2
Patch Set 11 : Move Capability type inside Credentials. #Patch Set 12 : Add InstallDefaultTerminationSignalHandlers. #
Total comments: 25
Patch Set 13 : Respond to comments. #Patch Set 14 : More comments. #Messages
Total messages: 36 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
rickyz@chromium.org changed reviewers: + jln@chromium.org, mdempsky@chromium.org
Here's a change to put all children in their own PID namespaces. It should be simple to do this for NaCl as well if desired. https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... File content/zygote/zygote_main_linux.cc (left): https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... content/zygote/zygote_main_linux.cc:468: CHECK(sandbox::Credentials::DropAllCapabilities()); Like I mentioned in that last change, we need CAP_SYS_ADMIN in order to start children in their own PID namespaces. We should probably discuss whether the gains of isolating renderers is worth the additional attack surface of the syscalls exposed by these capabilities on the zygote.
I like it. I'm worried if it will have any performance impact though: I seem to recall nested PID namespaces were much slower in older Linux kernels. E.g., I think UnixDomainSocketTest.DoubleNamespace took about 500ms longer on Ubuntu Precise than it does on Ubuntu Trusty. https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... File content/zygote/zygote_main_linux.cc (left): https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... content/zygote/zygote_main_linux.cc:468: CHECK(sandbox::Credentials::DropAllCapabilities()); On 2015/02/07 05:14:03, rickyz wrote: > Like I mentioned in that last change, we need CAP_SYS_ADMIN in order to start > children in their own PID namespaces. We should probably discuss whether the > gains of isolating renderers is worth the additional attack surface of the > syscalls exposed by these capabilities on the zygote. Couple thoughts I have: 1. Would it be worthwhile to drop everything else except for CAP_SYS_ADMIN if that's all we need? (Do we even have other capabilities?) 2. I think the init process reaper now will have unneeded capabilities too; that's probably not a risk, but might as well drop the capabilities there too so only the actual zygote process needs to reap?
https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... File content/zygote/zygote_main_linux.cc (left): https://codereview.chromium.org/868233011/diff/80001/content/zygote/zygote_ma... content/zygote/zygote_main_linux.cc:468: CHECK(sandbox::Credentials::DropAllCapabilities()); On 2015/02/09 06:28:12, mdempsky wrote: > On 2015/02/07 05:14:03, rickyz wrote: > > Like I mentioned in that last change, we need CAP_SYS_ADMIN in order to start > > children in their own PID namespaces. We should probably discuss whether the > > gains of isolating renderers is worth the additional attack surface of the > > syscalls exposed by these capabilities on the zygote. > > Couple thoughts I have: > > 1. Would it be worthwhile to drop everything else except for CAP_SYS_ADMIN if > that's all we need? (Do we even have other capabilities?) Done - I didn't look too much into how much attack surface this takes away - CAP_SYS_ADMIN seems to be used as a catch-all in a lot of places. > 2. I think the init process reaper now will have unneeded capabilities too; > that's probably not a risk, but might as well drop the capabilities there too so > only the actual zygote process needs to reap? Done.
Exciting times! Let's move as much stuff as possible into reasonable-looking API in sandbox/ As you know, I'm also trying to unify everything into a new generic sandbox class in sandbox::, so in general, that's the direction we should be taking. https://chromiumcodereview.appspot.com/868233011/diff/140001/content/common/s... File content/common/sandbox_linux/sandbox_linux.h (right): https://chromiumcodereview.appspot.com/868233011/diff/140001/content/common/s... content/common/sandbox_linux/sandbox_linux.h:123: int proc_fd() { return proc_fd_; } const https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... File content/zygote/zygote_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_linux.cc:399: clone_flags |= CLONE_NEWPID; I know there was some back and forth on this, but how about not supporting kernel that don't enable PID namespace? It's probably better to be forceful now while everything is new rather than drag a ton of complexity down the line... https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_linux.cc:406: CHECK(sandbox::Credentials::DropAllCapabilities( Why not always drop capabilities? Is it because proc_fd_ may not be available if --no-sandbox? I wanted to send a change to initialize proc_fd_ in the constructor (and make it a ScopedFD). Would that be sufficient here? Even without that, I would rather have something like: if (proc_fd >= 0) { sandbox::Credentials::DropAllCapabilities(proc_fd); } else { sandbox::Credentials::DropAllCapabilities(); } https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_linux.cc:410: // If the process is the init process inside a PID namespace, it must have (I'm trying to figure out how we can re-factor the whole sandbox business into a nice generic sandbox class) But even before that, this part should be exposed as an API, probably in NamespaceSandbox. 1. Something along the lines of AssumePidNameSpaceOwnership() (with clear documentation and some unit testing). It would: - DCHECK(1 == getpid); - Drop all capabilities - Install signal handlers for all signals that can be caught and have a default "TERM" action. Note: you should handle SIGHUP, SIGUSER1, SIGUSER2, SIGPIPE... 2. Provide a small, trivial ForkInNewPidNameSpace() wrapper. (It's useful because it's a good place for documenting that children should either use CreateInitProcessReaper() or AssumePidNameSpaceOwnership() and not fork again). 3. Provide an API there along the line of GetTerminationStatus() that can be used to retreive termination status for processes started with ForkInNewPidNameSpace(). https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... File content/zygote/zygote_main_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_main_linux.cc:410: static void DropAllCapabilities(int proc_fd, base::Closure* done) { DropAllCapabilitiesAndRun() https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_main_linux.cc:410: static void DropAllCapabilities(int proc_fd, base::Closure* done) { I don't think there is a way in base:: to make a closure from a list of Closures, even though that's kind of what we want to do here. Maybe just make a RunTwoClosures(Closure* one, Closure*, two) {one.Run(); two.Run();} and make a closure from that? https://chromiumcodereview.appspot.com/868233011/diff/140001/content/zygote/z... content/zygote/zygote_main_linux.cc:411: LOG(ERROR) << "proc_fd: " << proc_fd; oops https://chromiumcodereview.appspot.com/868233011/diff/140001/sandbox/linux/se... File sandbox/linux/services/credentials.h (right): https://chromiumcodereview.appspot.com/868233011/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials.h:8: #include <sys/capability.h> :( https://chromiumcodereview.appspot.com/868233011/diff/140001/sandbox/linux/se... sandbox/linux/services/credentials.h:42: static bool SetCapabilities(int proc_fd, Not great to expose that. Probably ok for now, but we may want to go to the trouble of defining our own enum class of capability and wrapping the whole low level API. It will make it easier to migrate out of libcap. It looks like we'll need it anymore.
https://codereview.chromium.org/868233011/diff/140001/content/common/sandbox_... File content/common/sandbox_linux/sandbox_linux.h (right): https://codereview.chromium.org/868233011/diff/140001/content/common/sandbox_... content/common/sandbox_linux/sandbox_linux.h:123: int proc_fd() { return proc_fd_; } On 2015/02/25 21:32:48, jln wrote: > const Done. https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_l... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:399: clone_flags |= CLONE_NEWPID; On 2015/02/25 21:32:48, jln wrote: > I know there was some back and forth on this, but how about not supporting > kernel that don't enable PID namespace? > > It's probably better to be forceful now while everything is new rather than drag > a ton of complexity down the line... I'm happy to not go out of my way to support people with userns and no pidns, but I don't think it really adds much complexity in this case (I think it'll probably work fine with this code, and we won't have to bend over backwards to make it work). https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:406: CHECK(sandbox::Credentials::DropAllCapabilities( On 2015/02/25 21:32:48, jln wrote: > Why not always drop capabilities? > > Is it because proc_fd_ may not be available if --no-sandbox? > > I wanted to send a change to initialize proc_fd_ in the constructor (and make it > a ScopedFD). Would that be sufficient here? > > Even without that, I would rather have something like: > > if (proc_fd >= 0) { > sandbox::Credentials::DropAllCapabilities(proc_fd); > } else { > sandbox::Credentials::DropAllCapabilities(); > } Yeah, that's exactly the reason. I haven't addressed this comment yet, but will in the next patchset. https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:410: // If the process is the init process inside a PID namespace, it must have On 2015/02/25 21:32:48, jln wrote: > (I'm trying to figure out how we can re-factor the whole sandbox business into a > nice generic sandbox class) > > But even before that, this part should be exposed as an API, probably in > NamespaceSandbox. > > 1. Something along the lines of AssumePidNameSpaceOwnership() (with clear > documentation and some unit testing). > > It would: > > - DCHECK(1 == getpid); > - Drop all capabilities > - Install signal handlers for all signals that can be caught and have a default > "TERM" action. > > Note: you should handle SIGHUP, SIGUSER1, SIGUSER2, SIGPIPE... > > 2. Provide a small, trivial ForkInNewPidNameSpace() wrapper. > > (It's useful because it's a good place for documenting that children should > either use CreateInitProcessReaper() or AssumePidNameSpaceOwnership() and not > fork again). > > 3. Provide an API there along the line of GetTerminationStatus() that can be > used to retreive termination status for processes started with > ForkInNewPidNameSpace(). Ugh, this signal handling stuff is so horrible. Here is an attempt at one way to expose the right API. I didn't drop capabilities in ForkWithNewPidNamespace in case the caller wants to fork a new pid namespace inside. Also, I made it the caller's responsibility to call InstallTerminationSignalHandler for any signals that they want to terminate so that they can register different exit codes for different signals. I'm really not happy with the state of this though, so any additional suggestions on how to make this nicer are more than appreciated! https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_m... File content/zygote/zygote_main_linux.cc (right): https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_m... content/zygote/zygote_main_linux.cc:410: static void DropAllCapabilities(int proc_fd, base::Closure* done) { On 2015/02/25 21:32:49, jln wrote: > I don't think there is a way in base:: to make a closure from a list of > Closures, even though that's kind of what we want to do here. > > Maybe just make a RunTwoClosures(Closure* one, Closure*, two) {one.Run(); > two.Run();} and make a closure from that? Done. https://codereview.chromium.org/868233011/diff/140001/content/zygote/zygote_m... content/zygote/zygote_main_linux.cc:411: LOG(ERROR) << "proc_fd: " << proc_fd; On 2015/02/25 21:32:48, jln wrote: > oops Done. https://codereview.chromium.org/868233011/diff/140001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/868233011/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.h:8: #include <sys/capability.h> On 2015/02/25 21:32:49, jln wrote: > :( Superseded by https://codereview.chromium.org/997463002/ :-) https://codereview.chromium.org/868233011/diff/140001/sandbox/linux/services/... sandbox/linux/services/credentials.h:42: static bool SetCapabilities(int proc_fd, On 2015/02/25 21:32:49, jln wrote: > Not great to expose that. Probably ok for now, but we may want to go to the > trouble of defining our own enum class of capability and wrapping the whole low > level API. > > It will make it easier to migrate out of libcap. It looks like we'll need it > anymore. Superseded by https://codereview.chromium.org/997463002/ :-)
It's so hard to build a reasonable-looking API with all these possible codepath and this complexity. I think I would love if ForkInNewPidNamespace() could automatically install the signal handlers. I'm worried about having "here's how to use the sandbox in 30 easy steps" syndrome. Let me know what you think! https://chromiumcodereview.appspot.com/868233011/diff/180001/content/common/s... File content/common/sandbox_linux/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/content/common/s... content/common/sandbox_linux/sandbox_linux.cc:193: 1, sandbox::LinuxCapability::kCapSysAdmin); Why the value 1? That's CAP_DAC_OVERRIDE, right? https://chromiumcodereview.appspot.com/868233011/diff/180001/content/zygote/z... File content/zygote/zygote_linux.cc (left): https://chromiumcodereview.appspot.com/868233011/diff/180001/content/zygote/z... content/zygote/zygote_linux.cc:283: NOTREACHED(); NOTREACHED() crash is debug-only. I think it could be ok to move to LOG(FATAL), but it should be done as a separate CL in this case. https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); I would like to make this API really easy to use for "new users" who don't really want to care about details. How about taking a const ref to a std::map<int, int> of signal to exit_code. The API would be: for every catchable signal for which the default signal disposition is to terminate the process, install a signal handler that exits with |exit_code|, unless overriden by |signal_to_exit_code|. That way, a caller that "doesn't" care can simply pass (std::map<int,int>(), 1). I would also merge this with ForkInNewPidNamespace(). It gets rid of having to do "if (getpid() == 1)" and makes it much easier to use. Again, we should think of new users of these API who don't want to care too much. I would love to be able to have simple steps like: - Either start a new pid namespace with LaunchProcess - Or start a new PID namespace with ForkInNewPidNamespace(). There are your two options (incidentally we will probably need the ForKInNewPidNamespace() version for Mojo soon). WDYT?
https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); Meh: we still need to support a separate InstallTerminationSignalHandler() API for processes that used the sandbox:: LaunchProcess. So keeping it separate seems good.
https://chromiumcodereview.appspot.com/868233011/diff/180001/content/zygote/z... File content/zygote/zygote_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/content/zygote/z... content/zygote/zygote_linux.cc:401: CHECK(sandbox::Credentials::DropAllCapabilities( We'll need an API to drop all capabilities only on the current thread only anyways. How about: Add: DropAllCapabilitiesOnCurrentThread() API, which doesn't check for threads in /proc. Add some warning about this being a subtle / advanced API. In ForkInNewPidNamespace(), call DropAllCapabilitiesOnCurrentThread() in the child (safe since we just forked). Of course, DropAllCapabilities() will internally simply perform the single-threaded check and then call DropAllCapabilitiesOnCurrentThread(). This feels a bit odd, but seems like the right thing to do. At least DropAllCapabilitiesOnCurrentThread() calls should be rare and easily auditable. https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox_unittest.cc:133: if (pid == 0) { We should also check that getpid() == 1
Patchset #7 (id:200001) has been deleted
Respond to more comments.
https://codereview.chromium.org/868233011/diff/180001/content/common/sandbox_... File content/common/sandbox_linux/sandbox_linux.cc (right): https://codereview.chromium.org/868233011/diff/180001/content/common/sandbox_... content/common/sandbox_linux/sandbox_linux.cc:193: 1, sandbox::LinuxCapability::kCapSysAdmin); On 2015/03/24 22:03:34, jln wrote: > Why the value 1? That's CAP_DAC_OVERRIDE, right? This is an ugly way to initialize the vector with a single element (the one is the number of times to repeat the value) - unfortunately {sandbox::LinuxCapability::kCapSysAdmin} doesn't work in the one-element case :-( https://codereview.chromium.org/868233011/diff/180001/content/zygote/zygote_l... File content/zygote/zygote_linux.cc (left): https://codereview.chromium.org/868233011/diff/180001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:283: NOTREACHED(); On 2015/03/24 22:03:34, jln wrote: > NOTREACHED() crash is debug-only. > > I think it could be ok to move to LOG(FATAL), but it should be done as a > separate CL in this case. Done. https://codereview.chromium.org/868233011/diff/180001/content/zygote/zygote_l... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/868233011/diff/180001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:401: CHECK(sandbox::Credentials::DropAllCapabilities( On 2015/03/25 02:09:13, jln wrote: > We'll need an API to drop all capabilities only on the current thread only > anyways. > > How about: > > Add: DropAllCapabilitiesOnCurrentThread() API, which doesn't check for threads > in /proc. Add some warning about this being a subtle / advanced API. > > In ForkInNewPidNamespace(), call DropAllCapabilitiesOnCurrentThread() in the > child (safe since we just forked). > > Of course, DropAllCapabilities() will internally simply perform the > single-threaded check and then call DropAllCapabilitiesOnCurrentThread(). > > This feels a bit odd, but seems like the right thing to do. At least > DropAllCapabilitiesOnCurrentThread() calls should be rare and easily auditable. Good idea, done. I made it drop capabilities based on an argument to keep in flexible (in case someone wants to create nested pid namespaces, for example). https://codereview.chromium.org/868233011/diff/180001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/180001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); On 2015/03/24 22:57:40, jln wrote: > Meh: we still need to support a separate InstallTerminationSignalHandler() API > for processes that used the sandbox:: LaunchProcess. > > So keeping it separate seems good. Any opinions on whether this should take a map or be more free-form like this? Ideally I'd have liked to have an option with some default values, but default values are tough when people might want to know what signal caused their program to die :-/ https://codereview.chromium.org/868233011/diff/180001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://codereview.chromium.org/868233011/diff/180001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:133: if (pid == 0) { On 2015/03/25 02:09:13, jln wrote: > We should also check that getpid() == 1 Done.
https://chromiumcodereview.appspot.com/868233011/diff/180001/content/common/s... File content/common/sandbox_linux/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/content/common/s... content/common/sandbox_linux/sandbox_linux.cc:193: 1, sandbox::LinuxCapability::kCapSysAdmin); On 2015/03/25 22:47:46, rickyz wrote: > On 2015/03/24 22:03:34, jln wrote: > > Why the value 1? That's CAP_DAC_OVERRIDE, right? > > This is an ugly way to initialize the vector with a single element (the one is > the number of times to repeat the value) - unfortunately > {sandbox::LinuxCapability::kCapSysAdmin} doesn't work in the one-element case > :-( Ooch, misread that. I can't understand why this wouldn't work. Did you do std::vector<X> caps ({sandbox::LinuxCapability::kCapSysAdmin}); "const std::vector<X> caps {sandbox::LinuxCapability::kCapSysAdmin};" should work as well, but is more ambiguous and problematic (uniform initialization syntax vs initialization lists). That being said it's currently disallowed for technically being a "library feature" :( (https://chromium-cpp.appspot.com/). What would you think about doing a horrible-ugly push_back() instead? I suspect I may not be the only one do mis-read this. Up to you. https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/868233011/diff/180001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); On 2015/03/25 22:47:46, rickyz wrote: > On 2015/03/24 22:57:40, jln wrote: > > Meh: we still need to support a separate InstallTerminationSignalHandler() API > > for processes that used the sandbox:: LaunchProcess. > > > > So keeping it separate seems good. > > Any opinions on whether this should take a map or be more free-form like this? > Ideally I'd have liked to have an option with some default values, but default > values are tough when people might want to know what signal caused their program > to die :-/ I was thinking this should take two arguments: a default value and a std::map from signals to exit code to override that default. It's a little ugly, but I don't have a better idea. https://chromiumcodereview.appspot.com/868233011/diff/220001/content/zygote/z... File content/zygote/zygote_linux.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/220001/content/zygote/z... content/zygote/zygote_linux.cc:395: pid = sandbox::NamespaceSandbox::ForkInNewPidNamespace(true); /* drop_capabilities_in_child */ https://chromiumcodereview.appspot.com/868233011/diff/220001/sandbox/linux/se... File sandbox/linux/services/credentials.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/220001/sandbox/linux/se... sandbox/linux/services/credentials.cc:17: #include "base/files/file_path.h" #include "build/build_config.h" https://chromiumcodereview.appspot.com/868233011/diff/220001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/220001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox_unittest.cc:134: CHECK_EQ(1, getpid()); I don't like the implicit dependency on kNormalExitCode being different from what CHECK would exit with. Maybe add a test for CHECK(false) exiting with something else than kNormalExitCode? https://chromiumcodereview.appspot.com/868233011/diff/220001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox_unittest.cc:151: const pid_t pid = NamespaceSandbox::ForkInNewPidNamespace(true); /* drop_capabilities_in_child */
Add CHECK test/stuff.
https://codereview.chromium.org/868233011/diff/220001/content/zygote/zygote_l... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/868233011/diff/220001/content/zygote/zygote_l... content/zygote/zygote_linux.cc:395: pid = sandbox::NamespaceSandbox::ForkInNewPidNamespace(true); On 2015/03/25 23:47:50, jln wrote: > /* drop_capabilities_in_child */ Done. https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:17: #include "base/files/file_path.h" On 2015/03/25 23:47:50, jln wrote: > #include "build/build_config.h" Is this actually needed? I think THREAD_SANITIZER is set via the command line (in common.gyp) https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:134: CHECK_EQ(1, getpid()); On 2015/03/25 23:47:50, jln wrote: > I don't like the implicit dependency on kNormalExitCode being different from > what CHECK would exit with. > > Maybe add a test for CHECK(false) exiting with something else than > kNormalExitCode? Sure, I added the test (and found out in the process that the stack trace from the CHECK failure would cause the test to fail if we hit it) https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:151: const pid_t pid = NamespaceSandbox::ForkInNewPidNamespace(true); On 2015/03/25 23:47:50, jln wrote: > /* drop_capabilities_in_child */ Done.
Oops, remove extra period.
Use push_back instead of confusing syntax.
https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://codereview.chromium.org/868233011/diff/220001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:134: CHECK_EQ(1, getpid()); On 2015/03/26 00:09:37, rickyz wrote: > On 2015/03/25 23:47:50, jln wrote: > > I don't like the implicit dependency on kNormalExitCode being different from > > what CHECK would exit with. > > > > Maybe add a test for CHECK(false) exiting with something else than > > kNormalExitCode? > > Sure, I added the test (and found out in the process that the stack trace from > the CHECK failure would cause the test to fail if we hit it) Indeed, but that's a SANDBOX_TEST feature FYI :) https://codereview.chromium.org/868233011/diff/280001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/280001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); I think that's the last remaining nit for this CL.
Move Capability type inside Credentials.
https://codereview.chromium.org/868233011/diff/280001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/280001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:74: static void InstallTerminationSignalHandler(int sig, int exit_code); On 2015/03/26 00:49:47, jln wrote: > I think that's the last remaining nit for this CL. I added a InstallDefaultTerminationSignalHandlers. Eventually, we might want to make two version of ForkInNewPidNamespace: One that calls InstallDefaultTerminationSignalHandlers and one that calls CreateInitProcessReaper, but I honestly couldn't think of a great name that conveys all the weird gotchas of both, so I just made the lower level pieces for now.
Only a few nits. https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... File sandbox/linux/services/credentials.h (right): https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/credentials.h:33: kCapSysChroot, Nit: you could drop the "Cap", now that it's qualified with ..::Capability::k.. Up to you. https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/credentials.h:54: // threads will not be changed. Maybe add something along the lines "This is dangerous, do not use unless you know what you are doing"? https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.cc:152: const int kDefaultTermSignals[] = { static https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.cc:179: DCHECK_LT(sig_idx, arraysize(g_signal_exit_codes)); Hmm, let's just CHECK? I'm warry of dangerous API contracts like this. https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.cc:187: memset(&action, 0, sizeof(action)); In C++, simply do: struct sigaction action = {}; In C you can do: struct sigaction action = {0}; https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox.h (right): https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:82: bool override_existing_handler); I don't think we need |override_existing_handler|, do we? I don't really see a case where we would want to override an existing handler. If an existing handler exists: great, the "init" oddity doesn't exist, nothing to fix. https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox.h:91: static const int kDefaultExitCode = 1; Style: static const members should go to top of the class. https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox_unittest.cc:128: if (!Credentials::CanCreateProcessInNewUserNS()) { I don't think you need any of this here. Also can't you use a ScopedProcess? https://chromiumcodereview.appspot.com/868233011/diff/320001/sandbox/linux/se... sandbox/linux/services/namespace_sandbox_unittest.cc:203: struct sigaction action; struct sigaction action = {};
(Just two small nits; jln seems to have covered the rest of the CL pretty well.) https://codereview.chromium.org/868233011/diff/320001/content/common/sandbox_... File content/common/sandbox_linux/sandbox_linux.h (right): https://codereview.chromium.org/868233011/diff/320001/content/common/sandbox_... content/common/sandbox_linux/sandbox_linux.h:122: // after the sandbox has been sealed. Can/should we CHECK/DCHECK for this? https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/credentials.h:33: kCapSysChroot, Also, Chromium style is to use SHOUTY_CASE for enums. :(
https://codereview.chromium.org/868233011/diff/320001/content/common/sandbox_... File content/common/sandbox_linux/sandbox_linux.h (right): https://codereview.chromium.org/868233011/diff/320001/content/common/sandbox_... content/common/sandbox_linux/sandbox_linux.h:122: // after the sandbox has been sealed. On 2015/03/27 20:32:11, mdempsky wrote: > Can/should we CHECK/DCHECK for this? Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/credentials.h (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/credentials.h:33: kCapSysChroot, On 2015/03/27 20:32:11, mdempsky wrote: > Also, Chromium style is to use SHOUTY_CASE for enums. :( Oop, missed that, changed it as well, but let me know if you'd prefer that to happen in another CL. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/credentials.h:54: // threads will not be changed. On 2015/03/26 22:01:33, jln wrote: > Maybe add something along the lines "This is dangerous, do not use unless you > know what you are doing"? Heh, sure, done https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.cc (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.cc:152: const int kDefaultTermSignals[] = { On 2015/03/26 22:01:33, jln wrote: > static Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.cc:179: DCHECK_LT(sig_idx, arraysize(g_signal_exit_codes)); On 2015/03/26 22:01:34, jln wrote: > Hmm, let's just CHECK? I'm warry of dangerous API contracts like this. Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.cc:187: memset(&action, 0, sizeof(action)); On 2015/03/26 22:01:33, jln wrote: > In C++, simply do: > > struct sigaction action = {}; > > In C you can do: > > struct sigaction action = {0}; Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:82: bool override_existing_handler); On 2015/03/26 22:01:34, jln wrote: > I don't think we need |override_existing_handler|, do we? > > I don't really see a case where we would want to override an existing handler. > If an existing handler exists: great, the "init" oddity doesn't exist, nothing > to fix. The use case I was thinking of was that a user might want to call InstallDefaultTerminationSignalHandlers and then override the exit code for specific signals with InstallTerminationSignalHandler. What do you think? https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:91: static const int kDefaultExitCode = 1; On 2015/03/26 22:01:34, jln wrote: > Style: static const members should go to top of the class. Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox_unittest.cc (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:128: if (!Credentials::CanCreateProcessInNewUserNS()) { On 2015/03/26 22:01:34, jln wrote: > I don't think you need any of this here. > > Also can't you use a ScopedProcess? I kept the user namespace stuff in order to test this inside an init process of a pid namespace (in case CHECK does ends up doing something like raising a signal instead of exiting). https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox_unittest.cc:203: struct sigaction action; On 2015/03/26 22:01:34, jln wrote: > struct sigaction action = {}; Done.
lgtm https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:74: static void InstallDefaultTerminationSignalHandlers(); Add to the doc that if a signal handler already exists, it won't get overridden. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:82: bool override_existing_handler); On 2015/03/27 21:08:03, rickyz wrote: > On 2015/03/26 22:01:34, jln wrote: > > I don't think we need |override_existing_handler|, do we? > > > > I don't really see a case where we would want to override an existing handler. > > If an existing handler exists: great, the "init" oddity doesn't exist, nothing > > to fix. > > The use case I was thinking of was that a user might want to call > InstallDefaultTerminationSignalHandlers and then override the exit code for > specific signals with InstallTerminationSignalHandler. What do you think? Well, in that case, one could simply install the specific handler with InstallTerminationSignalHandler and then InstallDefaultTerminationSignalHandlers. So I think we can get rid of the parameter.
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org Link to the patchset: https://codereview.chromium.org/868233011/#ps360001 (title: "More comments.")
https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... File sandbox/linux/services/namespace_sandbox.h (right): https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:74: static void InstallDefaultTerminationSignalHandlers(); On 2015/03/27 21:24:35, jln wrote: > Add to the doc that if a signal handler already exists, it won't get overridden. Done. https://codereview.chromium.org/868233011/diff/320001/sandbox/linux/services/... sandbox/linux/services/namespace_sandbox.h:82: bool override_existing_handler); On 2015/03/27 21:24:36, jln wrote: > On 2015/03/27 21:08:03, rickyz wrote: > > On 2015/03/26 22:01:34, jln wrote: > > > I don't think we need |override_existing_handler|, do we? > > > > > > I don't really see a case where we would want to override an existing > handler. > > > If an existing handler exists: great, the "init" oddity doesn't exist, > nothing > > > to fix. > > > > The use case I was thinking of was that a user might want to call > > InstallDefaultTerminationSignalHandlers and then override the exit code for > > specific signals with InstallTerminationSignalHandler. What do you think? > > Well, in that case, one could simply install the specific handler with > InstallTerminationSignalHandler and then > InstallDefaultTerminationSignalHandlers. So I think we can get rid of the > parameter. Ah, good point, removed.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/868233011/360001
Message was sent while issue was closed.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4557699fbb30704f76c68b906d2656d2e322572c Cr-Commit-Position: refs/heads/master@{#322660}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:360001) has been created in https://codereview.chromium.org/1041163003/ by spang@chromium.org. The reason for reverting is: Appears to cause system lockups on Chrome OS.. |
