1376653 - firefox seccomp-bpf sandbox is broken on musl libc

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Szabolcs Nagy]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170626211010

Steps to reproduce:

start firefox 54.0 on alpine linux


Actual results:

instead of correct page load it shows "Gah. Your tab just crashed."

background process with seccomp-bpf filter crashed because pthread_create of musl uses different clone flags than glibc so the syscall failed and this lead to a null pointer dereference.

thread creation fails at ProcessHangMonitor ctor
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessHangMonitor.cpp#1108
null deref is at CreateHangMonitorChild
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessHangMonitor.cpp#1228
because mThread is 0.

the sandbox policy only allows the glibc clone flags and it fails to whitelist certain syscalls used by musl that are reasonable to allow, details are in the alpine bug report:
https://bugs.alpinelinux.org/issues/7454



Expected results:

allow thread creation, not crash.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Bill might be able to offer some thoughts ...

Comment 2

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

To summarize what I'm seeing in the downstream bug report, and what to do about it:

1. musl libc is passing CLONE_DETACHED, of which the man page says: “This flag is still defined, but has no effect.” (as of 2.6.2, which predates seccomp-bpf); the kernel doesn't seem to have done anything with the flag other than #define it since the beginning of the Git history.  We can just mask it off in the flag comparison.

2. musl uses tkill to target threads in the current process; the workaround we did for Bionic in bug 1093893 can have its conditional expanded.

3. getdents vs. getdents64; normally amd64 doesn't have separate "64" versions of file-related syscalls, but apparently this is an exception.  Trivial to fix.

4. ALSA is trying to fork/exec to… run a shell to expand a glob?  There seems to be a workaround to make ALSA not do that, but if there weren't:

  a. ALSA support is Tier 3 (in the sense of https://developer.mozilla.org/en-US/docs/Supported_build_configurations), and plugins other than hw and dmix may already be broken by content sandboxing.  Up to a point we'd take patches that allow more things under #ifdef MOZ_ALSA, but:

  b. fork/exec wouldn't work even if we allowed the syscalls, because the child process would inherit the seccomp-bpf policy, and *not* the SIGSYS handler.  So the first time the dynamic linker calls open() or stat(), the process is immediately killed.  (Also, once we get content processes chrooted, /bin/sh won't exist because / will be an empty deleted directory.)

Comment 3

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

Attachment: Bug 1376653 - Loosen restrictions on clone flags for musl.

I've made this non-ifdef'ed, and removed currently unused ifdef'ed cases
for old Android versions, because I'd rather have less code that we're
not even compile-testing than save a few cycles on a non-critical path.

Review commit: https://reviewboard.mozilla.org/r/158008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158008/

Comment 4

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

Attachment: Bug 1376653 - Unconditionalize the tkill() polyfill.

Review commit: https://reviewboard.mozilla.org/r/158010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158010/

Comment 5

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

Attachment: Bug 1376653 - Fix handling of architecture differences for getdents.

Review commit: https://reviewboard.mozilla.org/r/158012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158012/

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8887219 [details]
Bug 1376653 - Loosen restrictions on clone flags for musl.

https://reviewboard.mozilla.org/r/158008/#review163708

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8887220 [details]
Bug 1376653 - Unconditionalize the tkill() polyfill.

https://reviewboard.mozilla.org/r/158010/#review163712

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8887221 [details]
Bug 1376653 - Fix handling of architecture differences for getdents.

https://reviewboard.mozilla.org/r/158012/#review163714

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8f06d32af31
Loosen restrictions on clone flags for musl. r=gcp
https://hg.mozilla.org/integration/autoland/rev/9b5bb669d128
Unconditionalize the tkill() polyfill. r=gcp
https://hg.mozilla.org/integration/autoland/rev/f68747fe8a15
Fix handling of architecture differences for getdents. r=gcp

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8f06d32af31
https://hg.mozilla.org/mozilla-central/rev/9b5bb669d128
https://hg.mozilla.org/mozilla-central/rev/f68747fe8a15