1004832 - minidumps not generated for content processes that __android_log_assert() w/ seccomp enabled

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

__android_log_assert() uses tgkill to terminate the offending process.  With seccomp enabled, tgkill is only whitelisted when MOZ_PROFILING is defined.  In non-profiling builds, this prevents the minidump exception handler code from running and capturing the backtrace from the content process crash.

Here's one example of a crash that would not be visible on seccomp-enabled devices: https://crash-stats.mozilla.com/report/index/5bcec7f6-5ec3-4dc9-a0a0-ba3762140425


----
Program received signal SIGSYS, Bad system call.
[Switching to Thread 1958.2163]
tgkill () at bionic/libc/arch-arm/bionic/tgkill.S:46
46          ldmfd   sp!, {r4-r7, ip, lr}
(gdb) bt
#0  tgkill () at bionic/libc/arch-arm/bionic/tgkill.S:46
#1  0xb6e60134 in pthread_kill (t=, sig=6) at bionic/libc/bionic/pthread_kill.cpp:49
#2  0xb6e60348 in raise (sig=6) at bionic/libc/bionic/raise.cpp:32
#3  0xb6e5f07e in __libc_android_abort () at bionic/libc/bionic/abort.cpp:55
#4  0xb6e6e994 in abort () at bionic/libc/arch-arm/bionic/abort_arm.S:41
#5  0xb508b91e in __android_log_bwrite (tag=1958, payload=0x656d2f76, len=6) at system/core/liblog/logd_write.c:270
...
----

Comment 1

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #0)
> __android_log_assert() uses tgkill to terminate the offending process.  With
> seccomp enabled, tgkill is only whitelisted when MOZ_PROFILING is defined. 
> In non-profiling builds, this prevents the minidump exception handler code
> from running and capturing the backtrace from the content process crash.

Shouldn't we have crash dumps for SIGSYS crashes as of bug 945498?

Comment 2

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

But also, we don't gain much by forbidding tgkill, given that we allow fcntl with any arguments, and F_SETFL + F_SETSIG + F_SETOWN_EX lets you send any signal to any process or thread you'd be able to signal with kill/tgkill.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

(In reply to Jed Davis [:jld] from comment #1)
> Shouldn't we have crash dumps for SIGSYS crashes as of bug 945498?

Looks like it from that patch.  That bug isn't uplifted to v1.4 (yet?) though

Comment 4

[Michael Vines [:m1] [:evilmachines]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #3)
> Looks like it from that patch.  That bug isn't uplifted to v1.4 (yet?) though

Hmmm, well that's wrong since bug 945498 landed before v1.4 forked off m-c.  Doesn't seem to be helping in this case though.  

STR for this bug should be to just throw CHECK(false) into a content process somewhere.

Comment 5

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #0)
> __android_log_assert() uses tgkill to terminate the offending process.

But only on KitKat (or newer): https://android.googlesource.com/platform/bionic/+/7e6ce1a3c52d8533fed92c143419fedb0c93988a

Comment 6

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

Also not on x86, where the __builtin_trap() compiles to a UD2 instruction instead of a call to __libc_android_abort.

Comment 7

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

Here's what goes wrong: Bionic's abort() blocks all signals except SIGABRT, rather than simply unblocking SIGABRT and leaving other signals unchanged.  This quirk appears to be inherited from the original 4.4BSD sources, but it is not part of the relevant standards.

The second part of this is that the SIGSYS from seccomp-bpf has some special handling, such that if SIGSYS is blocked or ignored, then it is both unblocked and restored to SIG_DFL before raising it.  This uninstalls the signal handler that would otherwise invoke Breakpad.

Comment 8

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

Attachment: bug1004832-whitelist-tgkill-hg0.diff

Comment 9

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

Comment on attachment 8416790 [details] [diff] [review]
bug1004832-whitelist-tgkill-hg0.diff

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

I don't really like opening this.
Sure, fcntl should also be filtered.

Mainly on desktop childs run as the same uid and you could signal other processes with signals they'll process.

meanwhile setting + to unblock as it doesnt really degrade the current state.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for the b2g30_v1_4 uplift.

Comment 13

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

Attachment: bug1004832-uplift14-hg0.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We would fail to collect crash dumps for certain types of crash on certain devices.
Testing completed (on m-c, etc.): Tested locally by causing a suitable crash with and without the patch.  Landed on m-c in comment #11.
Risk to taking this patch (and alternatives if risky): No risk; it expands the system call whitelist (so won't add any sandbox violations), and as previously discussed here it doesn't meaningfully affect the attack surface.
String or IDL/UUID changes made by this patch: None

https://tbpl.mozilla.org/?tree=Try&rev=98ba3fefea44

Carrying over r+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8418455 [details] [diff] [review]
bug1004832-uplift14-hg0.diff

B2G v1.4 is on the b2g30_v1_4 branch as of last week's merge. But 1.4+ blockers have auto-approval at this stage still anyway :)

Thanks for rebasing!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ff823e835ed5

Comment 16

[Robert Mead [:RobertM]]

No STR is present to create test case to address bug.