Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707763002:
Fix unprivileged user namespace regression (Closed)
|
Created:
3 years, 10 months ago by Kevin Cernekee Modified:
3 years, 8 months ago Reviewers:
sky, Tom (Use chromium acct), jschuh, msw, Jorge Lucangeli Obes, jln (very slow on Chromium), Tom Anderson, mdempsky CC:
chromium-reviews, tfarina, dcheng Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix unprivileged user namespace regression
Prior to crrev.com/357c17552fb353ea9f3de6eca8a47b2d009067c8 ("Namespace
sandbox: add check for unprivileged use of CLONE_NEWUSER") it was
possible to run the entire browser inside an unprivileged user namespace.
This is useful because it lets normal desktop users create separate
network namespaces for different browser instances. It is often used
to implement per-app VPN support on Linux[1], as an alternative to
having the VPN tunnel all traffic for the entire system.
Add a new check to differentiate the "bad" case of "Chrome running as
global UID 0" from the "good" case of "Chrome running as UID 0 inside
an unprivileged namespace."
Also, add missing includes for std::move and geteuid().
[1] https://github.com/cernekee/ocproxy#vpnns-experimental
BUG=638180
TEST=test by hand using vpnns
Review-Url: https://codereview.chromium.org/2707763002
Cr-Commit-Position: refs/heads/master@{#463532}
Committed: https://chromium.googlesource.com/chromium/src/+/7bc960609bc49b21be3240970b449cd1146af615
Patch Set 1 #
Total comments: 4
Patch Set 2 : incorporate code review feedback #
Total comments: 4
Patch Set 3 : incorporate code review feedback #Messages
Total messages: 53 (27 generated)
The CQ bit was checked by cernekee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cernekee@chromium.org changed reviewers: + msw@chromium.org, thomasanderson@chromium.org
Currently untested, would appreciate your feedback on the approach.
thomasanderson@google.com changed reviewers: + mdempsky@chromium.org, thomasanderson@google.com
+mdempsky for security review https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:7: #if defined(OS_LINUX) Move ifdef'ed includes to the bottom https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:88: // Allow running inside an unprivileged user namespace. In that case, / remove / ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:7: #if defined(OS_LINUX) On 2017/02/19 19:16:06, Tom Anderson wrote: > Move ifdef'ed includes to the bottom Done. https://codereview.chromium.org/2707763002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:88: // Allow running inside an unprivileged user namespace. In that case, / On 2017/02/19 19:16:06, Tom Anderson wrote: > remove / ? I changed it to "the root directory" to make the comment easier to follow.
The CQ bit was checked by cernekee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pinging mdempsky@ for security review this lgtm from a style pov
I think I'll need to defer to another c/b/ui[/views] owner, and do encourage security review here.
cernekee@chromium.org changed reviewers: + dcheng@chromium.org, jorgelo@chromium.org
Friendly ping, could we please complete the security review? I have tested this CL locally and verified that it fixes the regression. Thanks!
dcheng@chromium.org changed reviewers: - dcheng@chromium.org
Deferring to jorgelo for this, since he knows a lot more about this
thomasanderson@google.com changed reviewers: + jln@chromium.org
+jln for security review
Hi, could we please wrap up the security review in time for the M59 branch? Thanks!
https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:87: // root directory will be owned by an unmapped UID and GID. This is not true all the time, is it? It's only true in the case where you enter a user namespace without chrooting. Maybe update the comment to make it clearer?
https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:89: if (stat("/", &st) == 0 && st.st_uid != 0) Does this need to be HANDLE_EINTR?
https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:87: // root directory will be owned by an unmapped UID and GID. On 2017/04/10 13:39:47, Jorge Lucangeli Obes wrote: > This is not true all the time, is it? It's only true in the case where you enter > a user namespace without chrooting. > > Maybe update the comment to make it clearer? Done. https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:89: if (stat("/", &st) == 0 && st.st_uid != 0) On 2017/04/10 13:41:18, Jorge Lucangeli Obes wrote: > Does this need to be HANDLE_EINTR? I don't think it is strictly necessary, although I can add it. stat(2) (Linux and OSX) and signal(7) (Linux) do not say that stat() can be interrupted. However I do see a couple of HANDLE_EINTR(fstat(...)) invocations in existing Chrome code. And this isn't necessarily portable to other OSes, like Solaris: http://docs.oracle.com/cd/E19253-01/816-5167/fstat-2/index.html By contrast: other syscalls, such as open(2) and statfs(2), do explicitly list EINTR as a possible error.
The CQ bit was checked by cernekee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/10 21:40:22, Kevin Cernekee wrote: > https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc (right): > > https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:87: // root > directory will be owned by an unmapped UID and GID. > On 2017/04/10 13:39:47, Jorge Lucangeli Obes wrote: > > This is not true all the time, is it? It's only true in the case where you > enter > > a user namespace without chrooting. > > > > Maybe update the comment to make it clearer? > > Done. > > https://codereview.chromium.org/2707763002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc:89: if > (stat("/", &st) == 0 && st.st_uid != 0) > On 2017/04/10 13:41:18, Jorge Lucangeli Obes wrote: > > Does this need to be HANDLE_EINTR? > > I don't think it is strictly necessary, although I can add it. stat(2) (Linux > and OSX) and signal(7) (Linux) do not say that stat() can be interrupted. > However I do see a couple of HANDLE_EINTR(fstat(...)) invocations in existing > Chrome code. And this isn't necessarily portable to other OSes, like Solaris: > > http://docs.oracle.com/cd/E19253-01/816-5167/fstat-2/index.html > > By contrast: other syscalls, such as open(2) and statfs(2), do explicitly list > EINTR as a possible error. That's a fair point. Have you tested this locally running as plain root and running as root inside a container?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/10 22:52:06, Jorge Lucangeli Obes wrote: > Have you tested this locally running as plain root and running as root inside a > container? Plain root -> crashes before this function even executes: https://pastebin.com/sz7UhJkV fakeroot -> crashes when setting up sandbox via /proc (may be normal) root inside a container -> starts up with no problem normal user -> starts up with no problem
On 2017/04/11 00:29:01, Kevin Cernekee wrote: > On 2017/04/10 22:52:06, Jorge Lucangeli Obes wrote: > > Have you tested this locally running as plain root and running as root inside > a > > container? > > Plain root -> crashes before this function even executes: > https://pastebin.com/sz7UhJkV > > fakeroot -> crashes when setting up sandbox via /proc (may be normal) > > root inside a container -> starts up with no problem > > normal user -> starts up with no problem It definitely shouldn't crash as root. I think you need to do a real install. $ GYP_DEFINES="buildtype=Official branding=Chrome" IGNORE_DEPS_CHANGES=1 ninja -C out/Release -j 5000 -k 1000000 chrome/installer/linux:stable_deb $ sudo dpkg -i out/Release/google-chrome-stable<version info>.deb
On 2017/04/11 00:36:16, Tom Anderson wrote: > It definitely shouldn't crash as root. I think you need to do a real install. Yeah, didn't realize that the out/Default directory was mode 0700. When I changed it to 0755, `sudo ./out/Default/chrome` displays the error dialog as expected, and no longer crashes.
lgtm since functionality works as expected.
The CQ bit was checked by cernekee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thomasanderson@google.com Link to the patchset: https://codereview.chromium.org/2707763002/#ps40001 (title: "incorporate code review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cernekee@chromium.org changed reviewers: + sky@chromium.org
Hi sky, could you please provide OWNERS approval for this file?
LGTM
The CQ bit was checked by cernekee@chromium.org
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": 40001, "attempt_start_ts": 1491883811549420,
"parent_rev": "392bf0c8ce6d9ab9b2fca45f8b645f14ac19813f", "commit_rev":
"7bc960609bc49b21be3240970b449cd1146af615"}
sky@chromium.org changed reviewers: + jschuh@chromium.org
I'm not entirely sure I understand the ramifications of this. NOT LGTM until Justin looks this over. +jschuh
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: sky@chromium.org. Please make sure to get positive review before another CQ attempt.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491883811549420,
"parent_rev": "392bf0c8ce6d9ab9b2fca45f8b645f14ac19813f", "commit_rev":
"7bc960609bc49b21be3240970b449cd1146af615"}
Message was sent while issue was closed.
Description was changed from
==========
Fix unprivileged user namespace regression
Prior to crrev.com/357c17552fb353ea9f3de6eca8a47b2d009067c8 ("Namespace
sandbox: add check for unprivileged use of CLONE_NEWUSER") it was
possible to run the entire browser inside an unprivileged user namespace.
This is useful because it lets normal desktop users create separate
network namespaces for different browser instances. It is often used
to implement per-app VPN support on Linux[1], as an alternative to
having the VPN tunnel all traffic for the entire system.
Add a new check to differentiate the "bad" case of "Chrome running as
global UID 0" from the "good" case of "Chrome running as UID 0 inside
an unprivileged namespace."
Also, add missing includes for std::move and geteuid().
[1] https://github.com/cernekee/ocproxy#vpnns-experimental
BUG=638180
TEST=test by hand using vpnns
==========
to
==========
Fix unprivileged user namespace regression
Prior to crrev.com/357c17552fb353ea9f3de6eca8a47b2d009067c8 ("Namespace
sandbox: add check for unprivileged use of CLONE_NEWUSER") it was
possible to run the entire browser inside an unprivileged user namespace.
This is useful because it lets normal desktop users create separate
network namespaces for different browser instances. It is often used
to implement per-app VPN support on Linux[1], as an alternative to
having the VPN tunnel all traffic for the entire system.
Add a new check to differentiate the "bad" case of "Chrome running as
global UID 0" from the "good" case of "Chrome running as UID 0 inside
an unprivileged namespace."
Also, add missing includes for std::move and geteuid().
[1] https://github.com/cernekee/ocproxy#vpnns-experimental
BUG=638180
TEST=test by hand using vpnns
Review-Url: https://codereview.chromium.org/2707763002
Cr-Commit-Position: refs/heads/master@{#463532}
Committed:
https://chromium.googlesource.com/chromium/src/+/7bc960609bc49b21be3240970b44...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7bc960609bc49b21be3240970b44...
Message was sent while issue was closed.
It looks like jorgelo is on the security team, so LGTM. Sorry for the run around. |
