1430756 - Sandbox prevents start of debug Firefox builds in Janitor containers (Assertion failure: false, at security/sandbox/linux/SandboxInfo.cpp:174)

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20180115220459

I was trying to run a debug build of Firefox as created inside a Janitor container, but it doesn't start at all and fails with the following assertion:

> Assertion failure: false, at /home/user/firefox/security/sandbox/linux/SandboxInfo.cpp:174

I have to note that this only happens for debug but not opt builds. So not sure what specifically blocks us here.

The full build (not artifact) was done based on the following changeset:

https://hg.mozilla.org/mozilla-central/rev/8c19564d609a

Comment 1

[Gian-Carlo Pascutto [:gcp]]

https://searchfox.org/mozilla-central/source/security/sandbox/linux/SandboxInfo.cpp#168

Comment 2

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

The history here is that we'd try to detect support for user namespaces by calling clone() with CLONE_NEWUSER, but we'd actually use the feature via unshare(), and this caused problems with valgrind (or at least potential problems that we got a heads-up about from Chromium) because valgrind would allow the clone but didn't understand unshare() at all.  Thus the probe with the no-op unshare(0) and the `return false` in that block of code.

At the time, as the code comments indicate, there was concern that anything else interfering with those system calls might need to be treated differently somehow and shouldn't just be silently ignored, and this is why the assertion is there[1].

What's happening in this case, however, is a sandbox[2] that disallows namespace manipulation due to (ironically) security concerns.  And I think this is the first time we've had a bug report about this assertion, so we can probably just remove it.

However, it's now 2018, and I'm about to land patches (in bug 1401062) that switch the actual sandboxing from unshare() to clone(), so this entire test can go away.


[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653/security/sandbox/linux/SandboxInfo.cpp#172-174
[2] https://docs.docker.com/engine/security/seccomp/

Comment 3

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

Is this going to be a problem for beta 59, or is it good enough to land a fix in 60 early next week after the branch?

Comment 4

[Jan Keromnes [:janx]]

I believe that what was preventing the sandbox from working in your case was a missing capability in the Docker container you used on https://janitor.technology. Containers don't have CAP_SYS_ADMIN by default, for security reasons (because this allows breaking out of a container to become root on the Docker host). This prevents things like sandboxes from working correctly (and also unfortunately `rr` from recording process executions it seems).

See also: https://github.com/JanitorTechnology/janitor/issues/232

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't care about when it's being landed. I was playing with Janitor to test something, which can wait another week. No problem.

Comment 6

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

Attachment: Bug 1430756 - Remove check for unshare(), which we're no longer using.

This also removes an assertion that was failing under external sandboxes
that deny unshare() even when it's a no-op.

Review commit: https://reviewboard.mozilla.org/r/214774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214774/

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8944628 [details]
Bug 1430756 - Remove check for unshare(), which we're no longer using.

https://reviewboard.mozilla.org/r/214774/#review220578

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22ce3b9ca9af
Remove check for unshare(), which we're no longer using. r=gcp

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/22ce3b9ca9af

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This works great now. Thanks!