1199481 - Complain more when entering namespace sandboxing code as root

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1199481-sandbox-complaint-hg0.diff

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1199481-sandbox-complaint-hg1.diff

Trivial update: add const.

Comment 3

[Guillaume Destuynder [:kang] [this account is no longer active]]

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+

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10e3f62dc8a6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10e3f62dc8a6