Closed Bug 1299581 Opened 4 years ago Closed 3 years ago

Remove wait4/waitpid from content process syscall whitelist

Categories

(Core :: Security: Process Sandboxing, defect, P3)

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox51 --- affected
firefox57 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(2 files)

In principle there's no need to permit sandboxed processes to use the wait/wait3/wait4/waitpid/waitid family of interfaces — it can't create its own child processes, so it shouldn't have any need to wait for children to terminate.

But, in practice, we are seeing wait4() being used; see bug 1296309 comment #6.  Currently live crash reports: bp-887f40ba-a17e-41b1-99af-3cc342160828, bp-9501d838-cb83-4f94-adb2-a16442160828, bp-bc6d5827-81ab-4c04-9cf0-d2b592160828, bp-4c02e07e-9316-47ff-8f54-5d23d2160828, bp-482d20c9-54eb-4ab0-bdcc-50c062160828, bp-44521c4f-2d67-469d-834d-46fc62160825.

Looking at the registers (in the Raw Dump tab), the calls are always wait4(-1, &status, WNOHANG, nullptr) — waiting for any child process, but returning immediately if none.  I suspect that this is the call from WaitPidDaemonThread in NSPR[*], which has caused other problems in the past (see bug 227246 and associated bugs).

[*] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/nsprpub/pr/src/md/unix/uxproces.c#648

This would be consistent with the reported stacks: frame #1 was found via the "frame_pointer" tactic, and this is a Nightly build so it uses --enable-profiling and has frame pointers, but the waitpid (etc.) wrappers in libc/libpthread don't.  So the sequence is: _pt_root calls WaitPidDaemonThread via function pointer, WaitPidDaemonThread's prologue links a stack frame pointing at _pt_root onto %rbp, then calls waitpid which doesn't does %rbp and then crashes.

(Ironically, the more problematic "scan" tactic (which I'm normally complaining about because it can have false positives) would've worked better here — it would've seen the return address in WaitPidDaemonThread (or whatever it is) on the stack instead of being misled by the frame pointer.)



So, the root cause of this might be something using NSPR (either directly or via nsIProcess) to start a child process — and launching the waiter thread as a side-effect, even if it fails to actually create a process (see also bug 1286324).

One thing we could do: alter the seccomp-bpf policy to make anything like waitpid(-1, _, WNOHANG) fail with ECHILD, and resume crashing on anything else.

Another thing we could do: make the nsIProcess implementation assert and/or return an error if it's used in a content process.
Whiteboard: sb+
See Also: → 1310314
Should probably just be a MOZ_RELEASE_ASSERT(!XRE_IsContentProcess());
Comment on attachment 8803882 [details]
Bug 1299581 - Crash immediately if we try to fork()/CreateProcess() in content.

This can't crash as long as the drag-and-drop code is actually using it (bug 1310116).  Returning an error would be less dramatic, but would probably still break the UI.

Also, this needs an XPCOM peer.
Attachment #8803882 - Flags: review?(jld)
(In reply to Jed Davis [:jld] {⏰UTC-6} from comment #4)
> This can't crash as long as the drag-and-drop code is actually using it (bug
> 1310116).  

It already crashes (in 32 bit), this just gives us a workable stack :-)

Does it make sense to whitelist waitpid (to match wait4 in 32-bit mode) so the crashes are gone, and make this code a debug warning? (To be made a release assert when the underlying bugs causing the calls are gone)
Flags: needinfo?(jld)
We'll do allow+warn in bug 1310116, so adding removal of waitpid to this bug.
Flags: needinfo?(jld)
Summary: Remove wait4 from content process syscall whitelist → Remove wait4/waitpid from content process syscall whitelist
Priority: -- → P3
Comment on attachment 8905681 [details]
Bug 1299581 - Fail waitpid et al. with ECHILD in sandboxed content processes.

https://reviewboard.mozilla.org/r/177472/#review184306
Attachment #8905681 - Flags: review?(gpascutto) → review+
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e6bfbf7e58e
Fail waitpid et al. with ECHILD in sandboxed content processes. r=gcp
https://hg.mozilla.org/mozilla-central/rev/2e6bfbf7e58e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → jld
You need to log in before you can comment on or make changes to this bug.