Closed
Bug 1299581
Opened 8 years ago
Closed 7 years ago
Remove wait4/waitpid from content process syscall whitelist
Categories
(Core :: Security: Process Sandboxing, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla57
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.
Updated•8 years ago
|
Whiteboard: sb+
Assignee | ||
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Should probably just be a MOZ_RELEASE_ASSERT(!XRE_IsContentProcess());
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Assignee: nobody → jld
You need to log in
before you can comment on or make changes to this bug.
Description
•