Closed
Bug 1199481
Opened 9 years ago
Closed 9 years ago
Complain more when entering namespace sandboxing code as root
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
kang
:
review+
|
Details | Diff | Splinter Review |
Breaking this out from bug 1189892 comment #30: unsharing the user namespace inherently drops all capabilities in the outer namespace. If we had any superuser capabilities at that point it's probably a mistake (running Firefox as root is unsupported but not outright forbidden; for other Gecko embeddings there could be reasons for it, but it's probably still a bad idea), and things can happen when capabilities are dropped that are unexpected and difficult to debug.
So it makes sense to have a loud all-caps warning for that case; it would have saved us a bunch of time over in bug 1189892 — and, as a side effect, could have picked up bug 1199379 much earlier.
Also, it probably makes sense to drop capabilities even if user namespaces aren't available — I considered doing this in the first place bug 1151607 but decided that it wasn't worth worrying about an unsupported case like running as root. This won't increase security much, but it does mean that any resulting breakage will be inflicted on all systems, which will make debugging these things easier.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8654319 -
Flags: review?(gdestuynder)
Assignee | ||
Comment 2•9 years ago
|
||
Trivial update: add const.
Attachment #8654319 -
Attachment is obsolete: true
Attachment #8654319 -
Flags: review?(gdestuynder)
Attachment #8654329 -
Flags: review?(gdestuynder)
Comment on attachment 8654329 [details] [diff] [review]
bug1199481-sandbox-complaint-hg1.diff
Review of attachment 8654329 [details] [diff] [review]:
-----------------------------------------------------------------
::: security/sandbox/linux/Sandbox.cpp
@@ +561,5 @@
> }
>
> + {
> + LinuxCapabilities existingCaps;
> + if (existingCaps.GetCurrent() && existingCaps.AnyEffective()) {
just making sure - this wont interfere/trigger log with the potential CAP_SYS_CHROOT (or actual root) thread right?
else r+
Attachment #8654329 -
Flags: review?(gdestuynder) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Guillaume Destuynder [:kang] from comment #3)
> ::: security/sandbox/linux/Sandbox.cpp
> @@ +561,5 @@
> > }
> >
> > + {
> > + LinuxCapabilities existingCaps;
> > + if (existingCaps.GetCurrent() && existingCaps.AnyEffective()) {
>
> just making sure - this wont interfere/trigger log with the potential
> CAP_SYS_CHROOT (or actual root) thread right?
This is before the UnshareUserNamespace(), which is before the chroot thread is started, so that won't be a problem. On B2G we're already non-root at this point, but it doesn't matter because this is (currently) reachable only if MOZ_GMP_SANDBOX.
Assignee | ||
Comment 5•9 years ago
|
||
checkin-needed: This might need to be applied after bug 1199413. Shouldn't affect our tests, but I ran https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46e26a64beb and the intermittent oranges look like not my fault.
Keywords: checkin-needed
Keywords: checkin-needed
Comment 7•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•