Closed
Bug 1376910
Opened 7 years ago
Closed 6 years ago
Remove SysV IPC access from content processes
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
References
Details
(Whiteboard: sblc4)
Attachments
(1 file, 1 obsolete file)
This turns out to be easier than I thought: * SysV message queues were used only by the ESET antivirus injected library, and bug 1362601 took care of that. * SysV semaphores are used by ALSA (probably for dmix), which is disabled by default as configure time. * SysV shared memory is more complicated, but we don't seem to be using it in content processes anymore. The fix/workaround for bug 1271100 prevents GTK from using it, but we don't seem to be having GTK draw directly to the X server in any case. There's also nsShmImage, but that seems to be used only in the parent process. Running `ipcs -p` shows child processes as the “last operation pid” for shm segments, but this seems to be an artifact of the parent doing fork/exec with SHM mappings (i.e., the exec acts like a shmdt() or something like that). And, indeed, blocking all the syscalls and unsharing the IPC namespace yields a browser that still works in local testing.
Updated•7 years ago
|
Whiteboard: sblc4
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36a0edec49086814786679d47954994712326ebd
Assignee | ||
Comment 4•7 years ago
|
||
I should also mention that I tested this locally with both DRI-based and NVidia GL drivers, so hopefully we won't have too many surprised when this ships in Nightly.
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8887700 [details] Bug 1376910 - Remove SysV IPC access from Linux content sandbox when possible. https://reviewboard.mozilla.org/r/158598/#review163994
Attachment #8887700 -
Flags: review?(gpascutto) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8887701 [details] Bug 1376910 - Unshare the SysV IPC namespace in content processes. https://reviewboard.mozilla.org/r/158600/#review163996
Attachment #8887701 -
Flags: review?(gpascutto) → review+
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d11cd5c3fc6f Block syscalls for SysV IPC in content processes. r=gcp https://hg.mozilla.org/integration/autoland/rev/17e2e2aa8f56 Unshare the SysV IPC namespace in content processes. r=gcp
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d11cd5c3fc6f https://hg.mozilla.org/mozilla-central/rev/17e2e2aa8f56
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 9•7 years ago
|
||
Things that are broken and being fixed: * ALSA using --enable-alsa: bug 1384439. Things that seem to be broken and need more investigation: * fglrx (“AMD Catalyst”), the proprietary driver for ATi/AMD GPUs * Something about graphics that causes Cairo to be used to draw directly to X for GTk widgets, and doesn't happen in my testing or in Mozilla's automation. Additionally: * Using ALSA via a shim library that pretends to be PulseAudio was already broken by sandboxing but now it's even more broken. This isn't supported, and until bug 1362220 happens the workarounds are to disable sandboxing or build with --enable-alsa.
Depends on: 1384439
Assignee | ||
Comment 10•7 years ago
|
||
I've backed this out while I deal with the graphics regressions: https://hg.mozilla.org/integration/mozilla-inbound/rev/1efacc8c49ba
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 11•7 years ago
|
||
Do you have some info on the 2 graphics regressions? (other bugs etc?)
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8887700 [details] Bug 1376910 - Remove SysV IPC access from Linux content sandbox when possible. https://reviewboard.mozilla.org/r/158598/#review167100 ::: security/sandbox/linux/SandboxFilter.cpp (Diff revision 1) > - // access. But the graphics layer might not be using them > +#ifdef MOZ_ALSA > - // anymore; this needs to be studied. > - case SHMGET: > - case SHMCTL: > - case SHMAT: > - case SHMDT: Note to self: the ALSA `dmix` plugin uses shared memory as well as semaphores, so these need to be moved under `#ifdef MOZ_ALSA` rather than deleted.
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #9) > * Something about graphics that causes Cairo to be used to draw directly to > X for GTk widgets, and doesn't happen in my testing or in Mozilla's > automation. I have a theory: libXext is being preloaded, probably indirectly via something like libGL, which defeats the XShmQueryExtension shim in libmozgtk[1]. If I do this kind of preload locally, it causes crashes that look like the ones we saw before this was backed out. [1] https://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/widget/gtk/mozgtk/mozgtk.c#633
Assignee | ||
Comment 15•7 years ago
|
||
So we'll need: 1. Something to detect fglrx and communicate that to content processes in an environment variable so that the information is available early enough to skip unsharing the IPC namespace. And, of course, adjust the seccomp-bpf policy as well. (If this happens after bug 1151624 we can skip the env var dance steps, because I plan to move all the unsharing to when we clone() the child process.) 2. Either preload libmozgtk itself, or copy the shim into libmozsandbox, which is already preloaded. Fortunately, LD_PRELOAD is handled before /etc/ld.so.preload, so our preload will win — assuming that that's really what's going on. (Side note: bug 624422 suggests that libmozgtk might become unnecessary once NPAPI is gone. The other approach is to make semget() and shmget() non-fatal on Nightly, but that might still break things with fglrx.
Keywords: stale-bug
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8887701 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 17•6 years ago
|
||
Comment on attachment 8887700 [details] Bug 1376910 - Remove SysV IPC access from Linux content sandbox when possible. This is a completely different patch (and has a different MozReview ID, no less), but MozReview is confused again. It needs re-review.
Attachment #8887700 -
Flags: review+ → review?(gpascutto)
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8887700 [details] Bug 1376910 - Remove SysV IPC access from Linux content sandbox when possible. https://reviewboard.mozilla.org/r/158598/#review222792 ::: security/sandbox/linux/SandboxFilter.cpp:599 (Diff revision 2) > public: > ContentSandboxPolicy(SandboxBrokerClient* aBroker, > ContentProcessSandboxParams&& aParams) > : mBroker(aBroker) > , mParams(Move(aParams)) > + , mAllowSysV(PR_GetEnv("MOZ_SANDBOX_ALLOW_SYSV") != nullptr) Counterintuitive that MOZ_SANDBOX_ALLOW_SYSV=0 does not do what you'd think. (Of course no user is *supposed* to specify that, though...) ::: security/sandbox/linux/launch/SandboxLaunch.cpp:124 (Diff revision 2) > + &nsIGfxInfo::GetAdapterVendorID, > + &nsIGfxInfo::GetAdapterVendorID2, > + }; > + for (const auto method : kMethods) { > + if (NS_SUCCEEDED((gfxInfo->*method)(vendorID))) { > + if (vendorID.EqualsLiteral("ATI Technologies Inc.")) { Ok this is a clever way to not catch newer drivers :-)
Attachment #8887700 -
Flags: review?(gpascutto) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #18) > > + if (vendorID.EqualsLiteral("ATI Technologies Inc.")) { > > Ok this is a clever way to not catch newer drivers :-) I added a comment about this, because it's not quite what it looks like: Post-merger AMD-branded devices also have that ATI vendor string if they're using the Catalyst/fglrx drivers, but the open-source drivers integrated into Mesa identify their vendor as "X.Org".
Comment 21•6 years ago
|
||
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/74b5e036363f Remove SysV IPC access from Linux content sandbox when possible. r=gcp
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74b5e036363f
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla56 → mozilla60
Assignee | ||
Comment 23•6 years ago
|
||
Because I never actually wrote out the rationale for this in comment #0: SysV IPC authorization is all uid-based, so anything running as the right uid that can obtain or guess the correct small-integer “key” can get access. This is a problem for sandboxing, where we're trying to draw finer-grained security boundaries than “same uid” — it's like a mini-filesystem that we can't restrict or broker access to, only block completely.
Assignee | ||
Updated•6 years ago
|
status-firefox56:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•