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.

Pointer to Github pull-request

Pointer to Github pull-request

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

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.

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

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

Here's the __android_log_print approach. Thoughts?

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 on attachment 800529 [details] [diff] [review] bug912822-sandbox-log-hg0.diff Talked to Brian at the MozSummit; he says r=kang is enough.

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.