Closed Bug 1199481 Opened 4 years ago Closed 4 years ago

Complain more when entering namespace sandboxing code as root

Categories

(Core :: Security: Process Sandboxing, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8654319 - Flags: review?(gdestuynder)
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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/10e3f62dc8a6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.