912807 - Use --enable-content-sandbox-reporter on b2g "eng" builds

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

Currently there are two reasons to want --enable-content-sandbox-reporter on b2g:
1) bug 907006, where processes that are supposed to be killed by seccomp instead get stuck in zombie state (possibly due to a bug in our backport of the kernel support from 3.5 to 3.0.x), and
2) the simple fact that we haven't yet caught all of our sandbox program's false positives, so people trying to develop or dogfood on seccomp-enabled devices like keon will encounter issues and should be given some clue as to what broke.

I gather that we'd prefer to not have the sandbox reporter enabled when we ship, in keeping with the principle of minimizing attack surface (killing the process immediately vs. letting it continue running in a presumably compromised state), and under the assumption that all false positives will have been discovered by then — but, for now, I feel that this security/usability tradeoff should favor usability.

Comment 1

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

Attachment: Enable sandbox reporter by default on b2g.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We should do this only for ENG builds, not for all builds.

Comment 3

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

Comment on attachment 799898 [details] [diff] [review]
Enable sandbox reporter by default on b2g.

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

As :briansmith says, I think this would make sense to have on ENG builds only instead of any build. Can you modify the patch for that?

Comment 4

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

(In reply to Guillaume Destuynder [:kang] from comment #3)
> As :briansmith says, I think this would make sense to have on ENG builds
> only instead of any build. Can you modify the patch for that?

Does that belong in gecko/configure.in rather than gonk-misc/default-gecko-config, at that point?  (Or maybe elsewhere in the mozconfig machinery.)

I don't particularly like the idea of using SECCOMP_RET_KILL at all as long as we're running on kernels where it's known to be broken, but on further thought that might want to be a separate bug.

Comment 5

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

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-misc/pull/114

Pointer to Github pull-request

Comment 6

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

Comment on attachment 800502 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-misc/pull/114

Talked to Brian at the MozSummit; he says r=kang is enough.

Comment 7

[Julien Wajsberg [:julienw]]

Jed, you marked "affected" for "1.2", does it mean you'd like it uplifted to the 1.2 branch ? If yes I think we should ask koi+ on this (or we can say it's not part of the build because it's only done for eng builds ?)

Comment 8

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

I'd like for this to be uplifted, if that's allowed.  I doubt we'd consider it koi+, if I understand correctly, because it wouldn't block the 1.2 release.

It is definitely Not Part Of The User Build (or any other build whose variant isn't "eng"), so if that's enough I can make another pull request.

Comment 9

[Julien Wajsberg [:julienw]]

Yep, I agree with this.

If that's not done before I'll land and uplift monday.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://github.com/mozilla-b2g/gonk-misc/commit/5283859444a13b05cb3ce584236022c56a6d726c

Comment 11

[Julien Wajsberg [:julienw]]

a=npotb
gonk-misc/v1.2: 610051dd78e520363d3a3cd88df6ce86a6e59274