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)

Unspecified
Linux
enhancement

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.
Whiteboard: sblc4
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 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 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
https://hg.mozilla.org/mozilla-central/rev/d11cd5c3fc6f
https://hg.mozilla.org/mozilla-central/rev/17e2e2aa8f56
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
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 → ---
Do you have some info on the 2 graphics regressions? (other bugs etc?)
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.
Priority: -- → P1
(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
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
Priority: P1 → P2
Attachment #8887701 - Attachment is obsolete: true
Priority: P2 → P1
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 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+
(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".
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
https://hg.mozilla.org/mozilla-central/rev/74b5e036363f
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla56 → mozilla60
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.
Regressions: 1571541
No longer regressions: 1571541
You need to log in before you can comment on or make changes to this bug.