1009995 - Require seccomp-bpf support on KitKat-based B2G

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

The original plan in bug 790923 was to require seccomp-bpf support on all B2G devices, but that wasn't feasible at the time, and as a result it's silently disabled if the kernel doesn't support it.  However, we should be able to revive that for devices based on Android KitKat (and future versions).

Currently we have:

* Emulator: almost done; see bug 997469.
* Nexus 4: no bug yet, but various people (myself included) have patched/rebuilt locally for the JB version, so shouldn't be hard.
* Nexus 5: ought to be the same difficulty level as N4, but I haven't investigated yet.
* Dolphin: based on 3.10.17, so it has the original non-backported seccomp-bpf patches, and it's not a TARGET_NO_KERNEL device, so it's just a kernel config change.

And:

* Future KK-based Qualcomm devices, including Flame (currently JB, but should be KK+ at some future point): should pick up the changes from CodeAurora, so we shouldn't have to do anything.


In any case, we'll want to have a configure option to allow overriding this default.

Comment 1

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

Attachment: bug1009995-require-seccomp-hg0.diff

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 8448936 [details] [diff] [review]
bug1009995-require-seccomp-hg0.diff

Review of attachment 8448936 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the OnSandboxUnsupported function.

Comment 3

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

Comment on attachment 8448936 [details] [diff] [review]
bug1009995-require-seccomp-hg0.diff

Review of attachment 8448936 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Adding paul in feedback just to make sure everyone is OK with that behavior. (of course, I'm all for this behavior ;)

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just to make sure I understand this correctly: this makes seccomp mandatory on kitkat based FxOS builds, by introducing a crash when seccomp is not enabled. 

This sounds fine (very good even!), so long as the overide (export MOZ_DISABLE_CONTENT_SANDBOX=1) still works. 

Maybe we want to include instructions in the error message to help developers here, so they don't need to come to this bug to find this out ? Just a thought.

Comment 5

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

My patch for this is going to depend on the patch for bug 1043733.

Comment 6

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

Attachment: bug10099995-must-sandbox-hg0.diff

New patch, based on bug 1043733.  Similarly to that bug, disabling content process sandboxing with an env var counts as sandboxing being “supported” (because enabling it will always “succeed”) and bypasses the check, as :pauljt requested in comment #4.

Comment 7

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

Comment on attachment 8472745 [details] [diff] [review]
bug10099995-must-sandbox-hg0.diff

Review of attachment 8472745 [details] [diff] [review]:
-----------------------------------------------------------------

Just a nit for clarity about when the sandbox is enforced or not, otherwise r+

::: dom/ipc/ContentChild.cpp
@@ +922,5 @@
>    // at some point; see bug 880808.
>  #if defined(MOZ_CONTENT_SANDBOX)
>  #if defined(XP_LINUX)
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 19
> +    // Sandboxing is mandatory; see also ContentParent::StartUp().

CanSandboxContentProcess() only enforces the sandbox for MOZ_WIDGET_GONK - its not very clear when reading only this file (ContentChild.cpp)

I would either make it clear by doing it everywhere the same way, or make the comment clearer - just in case someone elses touches this code later

Comment 8

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

(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #7)
> Just a nit for clarity about when the sandbox is enforced or not, otherwise
> r+
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +922,5 @@
> >    // at some point; see bug 880808.
> >  #if defined(MOZ_CONTENT_SANDBOX)
> >  #if defined(XP_LINUX)
> > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 19
> > +    // Sandboxing is mandatory; see also ContentParent::StartUp().
> 
> CanSandboxContentProcess() only enforces the sandbox for MOZ_WIDGET_GONK -
> its not very clear when reading only this file (ContentChild.cpp)

I can reword the comment to make it clear that "mandatory" refers only to B2G >= Kitkat; is that what you mean?

Comment 9

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

Attachment: bug1009995-must-sandbox-hg1.diff

Improved some of the comments.

Comment 10

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

Comment on attachment 8473252 [details] [diff] [review]
bug1009995-must-sandbox-hg1.diff

Review of attachment 8473252 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds clear to me now :)

Comment 11

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b6c34f278918

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6c34f278918