Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578483002:
Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER (Closed)
|
Created:
4 years ago by Tom (Use chromium acct) Modified:
3 years, 11 months ago CC:
chromium-reviews, jln+watch_chromium.org, Jorge Lucangeli Obes Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNamespace sandbox: add check for unprivileged use of CLONE_NEWUSER
Debian 8 restricts use of CLONE_NEWUSER to only processes with
CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysctl-to-disallow-unprivileged-CLONE_NEWUSER-by-default.patch)
Chrome was previously checking if the kernel supported CLONE_NEWUSER
by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome
was launched with. This leads to 2 scenarios:
1. If Chrome was run as root:
The check for CLONE_NEWUSER will succeed. Chrome will then set up
the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN.
Subsequent clone()'s with CLONE_NEWUSER will then fail.
2. If Chrome was run as a normal user:
The check for CLONE_NEWUSER will fail. Chrome will fallback to
using the setuid sandbox.
The solution is to simply drop CAP_SYS_ADMIN before the check.
In addition, this CL disallows running Chromium as root unless launched
with --no-sandbox.
BUG=638180
Review-Url: https://codereview.chromium.org/2578483002
Cr-Commit-Position: refs/heads/master@{#443062}
Committed: https://chromium.googlesource.com/chromium/src/+/357c17552fb353ea9f3de6eca8a47b2d009067c8
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor #Patch Set 3 : Disallow root even with --user-data-dir #
Total comments: 2
Patch Set 4 : getuid -> geteuid #
Total comments: 5
Patch Set 5 : Allow --no-sandbox #Patch Set 6 : Add comment #
Messages
Total messages: 35 (11 generated)
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. BUG=638180 R=jln@chromium.org ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. BUG=638180 ==========
thomasanderson@google.com changed reviewers: + mdempsky@chromium.org - jln@chromium.org
mdempsky@ PTAL. This finally fixes bug 638180, which has been a top browser-crasher since M53 https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD root_dir(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); Without this, when running as root, IsFileSystemAccessDenied returns false. This was originally /proc/self/exe but was changed to / in https://chromium.googlesource.com/chromium/src/+/10feab647ef8b96609d087f9936e... I'm not sure why this makes it work, but I figured I'd see if you knew before I spent a lot of time debugging it.
https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/services/cred... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/services/cred... sandbox/linux/services/credentials.cc:297: int ret = sys_unshare(CLONE_NEWUSER); I would probably do: PCHECK(sys_unshare(CLONE_NEWUSER)); _exit(kExitSuccess); and then below: return WIFEXITED(status) && WEXITSTATUS(status) == kExitSuccess; https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD root_dir(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); On 2016/12/14 04:13:48, Tom Anderson wrote: > I'm not sure why this makes it work, but I figured I'd see if you knew before I > spent a lot of time debugging it. Hm, so my best guess is that because with the setuid sandbox, we end up with filesystem access, but chroot'd into an empty no-permissions directory. And then because root bypasses filesystem ACLs, we would be able to open "/" successfully. Are you able to confirm what UID the sandboxed processes are running as when you test it? However, if that's the case (i.e., renderers are running as root), that seems really bad to me. I'm not sure we actually want to support that case.
https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/services/cred... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/services/cred... sandbox/linux/services/credentials.cc:297: int ret = sys_unshare(CLONE_NEWUSER); On 2016/12/14 05:11:11, mdempsky wrote: > I would probably do: > > PCHECK(sys_unshare(CLONE_NEWUSER)); > _exit(kExitSuccess); > > and then below: > > return WIFEXITED(status) && WEXITSTATUS(status) == kExitSuccess; Done. But when I run this as root on Debian, I get the CHECK stack trace printed to the console now. Also I needed to change sys_clone to base::ForkWithFlags. For some reason, when using sys_clone, the parent process would exit too. https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/1/sandbox/linux/suid/client/s... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD root_dir(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); On 2016/12/14 05:11:11, mdempsky wrote: > On 2016/12/14 04:13:48, Tom Anderson wrote: > > I'm not sure why this makes it work, but I figured I'd see if you knew before > I > > spent a lot of time debugging it. > > Hm, so my best guess is that because with the setuid sandbox, we end up with > filesystem access, but chroot'd into an empty no-permissions directory. And then > because root bypasses filesystem ACLs, we would be able to open "/" > successfully. > ok that makes sense. > Are you able to confirm what UID the sandboxed processes are running as when you > test it? > The renderers have uid and gid 0 > However, if that's the case (i.e., renderers are running as root), that seems > really bad to me. I'm not sure we actually want to support that case. Yes that does sound bad :( But we do have a check here: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... I like that check because it shows a dialog box rather than just exiting to the console so if chrome was launched using a gui, the user will know what's wrong. However the check lets you run chrome as root with the --user-data-dir flag. Maybe we could just unconditionally refuse to run as root? Also I'm not hitting that check when using the namespace sandbox, but I guess that's correct behavior?
ping
On 2016/12/14 21:10:27, Tom Anderson wrote: > But we do have a check here: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... > > I like that check because it shows a dialog box rather than just exiting to the > console so if chrome was launched using a gui, the user will know what's wrong. > However the check lets you run chrome as root with the --user-data-dir flag. > Maybe we could just unconditionally refuse to run as root? Yeah, I'm inclined to say we should unconditionally refuse to run as root, but maybe I'm overlooking some historical reason why we supported that. I suggest we determine that first so we can determine how to proceed with this CL.
thomasanderson@google.com changed reviewers: + sky@chromium.org
+sky@ for chrome/app/generated_resources.grd and chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc On 2016/12/15 17:28:21, mdempsky wrote: > On 2016/12/14 21:10:27, Tom Anderson wrote: > > But we do have a check here: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m... > > > > I like that check because it shows a dialog box rather than just exiting to > the > > console so if chrome was launched using a gui, the user will know what's > wrong. > > However the check lets you run chrome as root with the --user-data-dir flag. > > Maybe we could just unconditionally refuse to run as root? > > Yeah, I'm inclined to say we should unconditionally refuse to run as root, but > maybe I'm overlooking some historical reason why we supported that. I suggest we > determine that first so we can determine how to proceed with this CL. Banned root in latest patch set
https://codereview.chromium.org/2578483002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (left): https://codereview.chromium.org/2578483002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:79: const base::CommandLine& command_line = Can you update the description to indicate why we want to allow user-data-dir now?
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. BUG=638180 ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root when using the setuid sandbox. If this were allowed, renderer processes would run as root as well. This change does not prevent running as root when the user namespace sandbox is used instead because after the sandbox is set up, renderers won't run as root. BUG=638180 ==========
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root when using the setuid sandbox. If this were allowed, renderer processes would run as root as well. This change does not prevent running as root when the user namespace sandbox is used instead because after the sandbox is set up, renderers won't run as root. BUG=638180 ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root when using the setuid sandbox. Previously, we allowed this case only when the --user-data-dir flag was present. However, if this were allowed, renderer processes would run as root as well, which opens up security vulnerabilities. This change does not prevent running as root when the user namespace sandbox is used instead because after the sandbox is set up, renderers won't run as root. BUG=638180 ==========
https://codereview.chromium.org/2578483002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (left): https://codereview.chromium.org/2578483002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:79: const base::CommandLine& command_line = On 2017/01/05 23:21:59, sky wrote: > Can you update the description Updated the CL description. > to indicate why we want to allow user-data-dir now? This change disallows --user-data-dir
Thanks for updating description LGTM
Thanks for your work on this, Tom. I agree with Matthew that we should just refuse to run as root. Possibly in sandbox initialization code, so that users could run with --no-sandbox if they *really* want (and we already have a butter bar to make clear that this is unsupported). - I haven't spent a lot of time thinking about uid0 + nocaps escalation to full root in an empty chroot. But it's probably quite weak. Think of System V APIs for instance. We probably go around most of this through namespaces + seccomp, but I wouldn't want to rely on it (also the suid/NS sandbox is designed so that it is effective even without the second layer (seccomp)). - Distinguishing between the setuid sandbox and NS sandbox modes for whether or not we allow running as root feels artificial. I don't see a clear-cut security reason (we could always drop caps, if any, at sandbox initialization, rather than tie it to the NS sandbox path). But more importantly, it just makes for a weird product that works as root or not based on weird low level details. So, what do you think of the sandboxing initialization code aborting if uid == 0, and still allowing --no-sandbox as an extreme override?
On 2017/01/06 01:07:30, jln (very slow on Chromium) wrote: > Thanks for your work on this, Tom. > > I agree with Matthew that we should just refuse to run as root. Possibly in > sandbox initialization code, so that users could run with --no-sandbox if they > *really* want (and we already have a butter bar to make clear that this is > unsupported). > > - I haven't spent a lot of time thinking about uid0 + nocaps escalation to full > root in an empty chroot. But it's probably quite weak. Think of System V APIs > for instance. We probably go around most of this through namespaces + seccomp, > but I wouldn't want to rely on it (also the suid/NS sandbox is designed so that > it is effective even without the second layer (seccomp)). > > - Distinguishing between the setuid sandbox and NS sandbox modes for whether or > not we allow running as root feels artificial. I don't see a clear-cut security > reason (we could always drop caps, if any, at sandbox initialization, rather > than tie it to the NS sandbox path). But more importantly, it just makes for a > weird product that works as root or not based on weird low level details. > I would be fine with allowing uid0 and unconditionally dropping capabilities if this is approved by the security team. > So, what do you think of the sandboxing initialization code aborting if uid == > 0, and still allowing --no-sandbox as an extreme override? My main concern is if we have the check in sandbox code, chrome will just fail to launch. We could add a verbose error message, but users would only see this when running from a shell. If they launch chrome from a gui (eg. some desktop environments have "right click -> run as root"), they won't see the error and will just think Chrome is broken. The check in ChromeBrowserMainExtraPartsViews has a dialog for this.
On 2017/01/06 01:41:02, Tom Anderson wrote: > My main concern is if we have the check in sandbox code, chrome will just fail > to launch. We could add a verbose error message, but users would only see this > when running from a shell. If they launch chrome from a gui (eg. some desktop > environments have "right click -> run as root"), they won't see the error and > will just think Chrome is broken. The check in ChromeBrowserMainExtraPartsViews > has a dialog for this. I'm missing something I think. Isn't this a concern with your current patch of the user doesn't have unprivileged namespace support?
On 2017/01/06 01:45:53, jln (very slow on Chromium) wrote: > On 2017/01/06 01:41:02, Tom Anderson wrote: > > > My main concern is if we have the check in sandbox code, chrome will just fail > > to launch. We could add a verbose error message, but users would only see > this > > when running from a shell. If they launch chrome from a gui (eg. some desktop > > environments have "right click -> run as root"), they won't see the error and > > will just think Chrome is broken. The check in > ChromeBrowserMainExtraPartsViews > > has a dialog for this. > > I'm missing something I think. Isn't this a concern with your current patch of > the user doesn't have unprivileged namespace support? The behavior with the current patch is: - if the user is root and has unprivileged namespace support, then chrome launches (and renderers don't have uid0) - if the user is root and doesn't have unprivileged namespace support, then a dialog is shown that says you can't run as root and chrome doesn't launch
On 2017/01/06 02:00:21, Tom Anderson wrote: > On 2017/01/06 01:45:53, jln (very slow on Chromium) wrote: > > On 2017/01/06 01:41:02, Tom Anderson wrote: > > > > > My main concern is if we have the check in sandbox code, chrome will just > fail > > > to launch. We could add a verbose error message, but users would only see > > this > > > when running from a shell. If they launch chrome from a gui (eg. some > desktop > > > environments have "right click -> run as root"), they won't see the error > and > > > will just think Chrome is broken. The check in > > ChromeBrowserMainExtraPartsViews > > > has a dialog for this. > > > > I'm missing something I think. Isn't this a concern with your current patch of > > the user doesn't have unprivileged namespace support? > > The behavior with the current patch is: > - if the user is root and has unprivileged namespace support, > then chrome launches (and renderers don't have uid0) > - if the user is root and doesn't have unprivileged namespace support, > then a dialog is shown that says you can't run as root and chrome doesn't > launch Ok. So, could we always show this dialog when Chrome is launched as root, if --no-sandbox is not passed? That seems like a reasonable compromise to me.
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root when using the setuid sandbox. Previously, we allowed this case only when the --user-data-dir flag was present. However, if this were allowed, renderer processes would run as root as well, which opens up security vulnerabilities. This change does not prevent running as root when the user namespace sandbox is used instead because after the sandbox is set up, renderers won't run as root. BUG=638180 ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root in all cases. BUG=638180 ==========
On 2017/01/09 18:34:38, jln (very slow on Chromium) wrote: > On 2017/01/06 02:00:21, Tom Anderson wrote: > > On 2017/01/06 01:45:53, jln (very slow on Chromium) wrote: > > > On 2017/01/06 01:41:02, Tom Anderson wrote: > > > > > > > My main concern is if we have the check in sandbox code, chrome will just > > fail > > > > to launch. We could add a verbose error message, but users would only see > > > this > > > > when running from a shell. If they launch chrome from a gui (eg. some > > desktop > > > > environments have "right click -> run as root"), they won't see the error > > and > > > > will just think Chrome is broken. The check in > > > ChromeBrowserMainExtraPartsViews > > > > has a dialog for this. > > > > > > I'm missing something I think. Isn't this a concern with your current patch > of > > > the user doesn't have unprivileged namespace support? > > > > The behavior with the current patch is: > > - if the user is root and has unprivileged namespace support, > > then chrome launches (and renderers don't have uid0) > > - if the user is root and doesn't have unprivileged namespace support, > > then a dialog is shown that says you can't run as root and chrome doesn't > > launch > > > Ok. So, could we always show this dialog when Chrome is launched as root, if > --no-sandbox is not passed? > > That seems like a reasonable compromise to me. Ok, I've changed this CL so that root is banned in all cases (all I needed to do was change getuid() to geteuid()). Do you still think we should permit --no-sandbox?
On 2017/01/10 22:15:10, Tom Anderson wrote: > Do you still think we should permit --no-sandbox? You mean still permit --no-sandbox as root? If it used to work, and it's no more complex to keep working than a single 'if' condition, then we might as well. There's only so much we can do to prevent users from shooting themselves in the foot.
https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:291: CHECK(sandbox::Credentials::DropAllCapabilities(proc_fd.get())); nit: You can just use DropAllCapabilities(), which will call OpenProc internally. https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD proc_self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); Since we decided to not support root+sandbox, can we revert this back to "/"?
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root in all cases. BUG=638180 ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root unless launched with --no-sandbox. BUG=638180 ==========
On 2017/01/10 22:31:49, mdempsky wrote: > On 2017/01/10 22:15:10, Tom Anderson wrote: > > Do you still think we should permit --no-sandbox? > > You mean still permit --no-sandbox as root? > > If it used to work, and it's no more complex to keep working than a single 'if' > condition, then we might as well. There's only so much we can do to prevent > users from shooting themselves in the foot. Ok, I made an exception for --no-sandbox. https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/services/... File sandbox/linux/services/credentials.cc (right): https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/services/... sandbox/linux/services/credentials.cc:291: CHECK(sandbox::Credentials::DropAllCapabilities(proc_fd.get())); On 2017/01/10 22:48:23, mdempsky wrote: > nit: You can just use DropAllCapabilities(), which will call OpenProc > internally. Done. https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD proc_self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); On 2017/01/10 22:48:23, mdempsky wrote: > Since we decided to not support root+sandbox, can we revert this back to "/"? Unfortunately no, since the dialog box gets shown after the sandbox is set up
ping. should be ready to land imo :)
You have a bunch of reviewers listed, who are you pinging? On Wed, Jan 11, 2017 at 11:56 AM, thomasanderson via Chromium-reviews <chromium-reviews@chromium.org> wrote: > ping. should be ready to land imo :) > > https://codereview.chromium.org/2578483002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD proc_self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); On 2017/01/10 23:32:03, Tom Anderson wrote: > On 2017/01/10 22:48:23, mdempsky wrote: > > Since we decided to not support root+sandbox, can we revert this back to "/"? > > Unfortunately no, since the dialog box gets shown after the sandbox is set up That is unfortunate. Can you at least add a comment mentioning we would rather check "/", but that gives false positives when running as root and reference this CL?
On 2017/01/11 22:20:15, mdempsky wrote: > lgtm > > https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... > File sandbox/linux/suid/client/setuid_sandbox_client.cc (right): > > https://codereview.chromium.org/2578483002/diff/60001/sandbox/linux/suid/clie... > sandbox/linux/suid/client/setuid_sandbox_client.cc:25: base::ScopedFD > proc_self_exe(HANDLE_EINTR(open("/proc/self/exe", O_RDONLY))); > On 2017/01/10 23:32:03, Tom Anderson wrote: > > On 2017/01/10 22:48:23, mdempsky wrote: > > > Since we decided to not support root+sandbox, can we revert this back to > "/"? > > > > Unfortunately no, since the dialog box gets shown after the sandbox is set up > > That is unfortunate. Can you at least add a comment mentioning we would rather > check "/", but that gives false positives when running as root and reference > this CL? Added comment.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, mdempsky@chromium.org Link to the patchset: https://codereview.chromium.org/2578483002/#ps100001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484174087819020,
"parent_rev": "dc1892fc0639464d4418ccfaedfd1f95a5b947d0", "commit_rev":
"357c17552fb353ea9f3de6eca8a47b2d009067c8"}
Message was sent while issue was closed.
Description was changed from ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root unless launched with --no-sandbox. BUG=638180 ========== to ========== Namespace sandbox: add check for unprivileged use of CLONE_NEWUSER Debian 8 restricts use of CLONE_NEWUSER to only processes with CAP_SYS_ADMIN. (https://github.com/semplice/linux/blob/master/debian/patches/debian/add-sysct...) Chrome was previously checking if the kernel supported CLONE_NEWUSER by running clone(CLONE_NEWUSER, ...) with the same capabilities chrome was launched with. This leads to 2 scenarios: 1. If Chrome was run as root: The check for CLONE_NEWUSER will succeed. Chrome will then set up the namespace sandbox by clone()'ing and dropping CAP_SYS_ADMIN. Subsequent clone()'s with CLONE_NEWUSER will then fail. 2. If Chrome was run as a normal user: The check for CLONE_NEWUSER will fail. Chrome will fallback to using the setuid sandbox. The solution is to simply drop CAP_SYS_ADMIN before the check. In addition, this CL disallows running Chromium as root unless launched with --no-sandbox. BUG=638180 Review-Url: https://codereview.chromium.org/2578483002 Cr-Commit-Position: refs/heads/master@{#443062} Committed: https://chromium.googlesource.com/chromium/src/+/357c17552fb353ea9f3de6eca8a4... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/357c17552fb353ea9f3de6eca8a4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
