912822 - Enable log messages from content sandbox reporter by default on b2g.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

As a companion to bug 912807, if we're enabling the seccomp sandbox reporter with (as part of the rationale) the goal of letting developers and dogfooders find the bugs, it would help to actually log the message that gives information on what broke.

That uses NSPR logging, and as far as I can tell the only way to make NSPR log anything is to ask for it in the enviroment.  This means changing b2g.sh in gonk-misc.

Comment 1

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

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-misc/pull/113#attch-to-bugzilla

Pointer to Github pull-request

Comment 2

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

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

Pointer to Github pull-request

Comment 3

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

Comment on attachment 799920 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-misc/pull/113#attch-to-bugzilla

(The PR-to-attachment addon got confused.)

Comment 4

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

As I mentioned in other bugs, I think we should do this only for ENG builds. The seccomp code seems like the most problematic NSPR logging enabled by default in release builds. We assume that malicious code is what trips the seccomp filter, and we don't want NSPR logging to be an avenue for the malicious code to use to attack us.

Comment 5

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

If MOZ_CONTENT_SANDBOX_REPORTER isn't defined, then the only remaining log message in the SeccompSandbox module is the one for failing to initialize seccomp (if the kernel doesn't support it), which should be less worrisome.

In any case, if we're going to make this conditional on build variant, setting env vars in b2g.sh is no longer the least annoying way to make that happen.  In particular, I notice that a significant number of Gecko source files have ifdef'ed calls to __android_log_print for things like this.  If people won't complain too loudly, I'll just do that instead (and, again, no effect ifndef MOZ_CONTENT_SANDBOX_REPORTER).

Comment 6

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

(In reply to Jed Davis [:jld] from comment #5)
> In any case, if we're going to make this conditional on build variant,
> setting env vars in b2g.sh is no longer the least annoying way to make that
> happen.  In particular, I notice that a significant number of Gecko source
> files have ifdef'ed calls to __android_log_print for things like this.  If
> people won't complain too loudly, I'll just do that instead (and, again, no
> effect ifndef MOZ_CONTENT_SANDBOX_REPORTER).

On second thought, this isn't particularly relevant to the concern raised in comment #4 — in either case a MOZ_CONTENT_SANDBOX_REPORTER will log the faults and non-reporter build won't, and I assume we're applying a similar level of security pessimism whether that's through NSPR or directly to the Android libraries.

However, now that I've seen how many existing things make their logging happen with __android_log_print (lots) and how many set env vars in b2g.sh (none), I might change the patch for consistency/least-surprise.

Comment 7

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

Attachment: bug912822-sandbox-log-hg0.diff

Here's the __android_log_print approach.  Thoughts?

Comment 8

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

Comment on attachment 800529 [details] [diff] [review]
bug912822-sandbox-log-hg0.diff

From my relatively narrow point of view ('sandbox works as expected, security isn't degraded') its ok - however I don't know what the rationale for using NSPR or other logging systems are on B2G. I'm also not sure what the rationale is for environment variables. For example, child process debug involves quite a few steps: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Debugging/Debugging_B2G_using_gdb (afaik the env variable in this page also works when starting b2g "manually", i've done that quite often in the past).

Comment 9

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

Comment on attachment 800529 [details] [diff] [review]
bug912822-sandbox-log-hg0.diff

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

Comment 10

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

Comment on attachment 800529 [details] [diff] [review]
bug912822-sandbox-log-hg0.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: This patch is useful for developers who trip over a deficiency in the seccomp whitelist: when they look in logcat they'll see a message which indicates why the app just mysteriously crashed, and will be able to usefully report it or investigate instead of just being confused.
Testing completed (on m-c, etc.): I have used this patch, applied to mozilla-aurora, while confirming that bug 919090 affected that branch.
Risk to taking this patch (and alternatives if risky): Effectively none.  This patch does mean that, on a non-seccomp-enabled b2g device (which at the moment is everything except keon/peak) the message during startup about failing to install the syscall filter will be logged, where before it was not logged.  But the system log is already quite noisy, and nobody will notice in any case unless they enable adb and look through logcat.
String or IDL/UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c5fe339bcd4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3194ce08765