1405877 - Cubeb audioipc requires a named Unix-domain socket

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, enhancement, P2)

Description

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

I noticed that the audioipc client connects to a named Unix-domain socket, which means we'll need to retain the workaround from bug 1259508 to pre-load cubeb before starting sandboxing on Linux.

Pre-acquiring resources like this isn't necessarily a bad thing — for example, the media plugin container on Linux pre-opens various files, including the plugin itself, so that the sandbox can completely remove filesystem access (vs. using a broker and increasing attack surface).

The alternative is to pass down one end of a socketpair over IPDL and then run the dedicated protocol over that.  This might be nontrivial to implement in this case, since the content process would be initiating the request for audio services.  The main benefit would be delaying audio initialization until when/if content requests it; I don't know whether this would have a significant effect on startup performance metrics.

In any case, this doesn't affect the strength of the sandbox, so from a sandboxing point of view I'd consider it lower priority than getting AudioIPC shipped and enabled by default.

Comment 1

[Matthew Gregan [:kinetik]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #0)
> The alternative is to pass down one end of a socketpair over IPDL and then
> run the dedicated protocol over that.  This might be nontrivial to implement
> in this case, since the content process would be initiating the request for
> audio services.  The main benefit would be delaying audio initialization
> until when/if content requests it; I don't know whether this would have a
> significant effect on startup performance metrics.

Would it be possible to pass one end of a socketpair into the content process preemptively at process creation time, which the client can then use later when trying to create the libcubeb context?  It seems like it'd be much simpler to implement, but it does waste an fd in every content process that ends up not using audio.

Comment 2

[Matthew Gregan [:kinetik]]

Forgot to mention: I ask because I was trying to get audioipc running in Firefox on macOS, which should work more-or-less right now, but I'm hitting issues with the sandbox that I suspect are related to the tempdir sandboxing the macOS sandbox enforces (special per-instance temp path which we don't use in audioipc plus a restriction to "regular" files only).

Comment 3

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

(In reply to Matthew Gregan [:kinetik] from comment #1)
> Would it be possible to pass one end of a socketpair into the content
> process preemptively at process creation time, which the client can then use
> later when trying to create the libcubeb context?  It seems like it'd be
> much simpler to implement, but it does waste an fd in every content process
> that ends up not using audio.

Yes; this is already how we pass down the descriptors for Gecko IPC and the crash reporter (and, on Linux, the rejected syscall reporter).  There are some examples at various places in GeckoChildProcessHost::PerformAsyncLaunchInternal that modify mFileMap (which bug 1401786 will change to mLaunchOptions).

Setting up file descriptors like this isn't supported on Windows, but there might be something else that could be used there.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> Forgot to mention: I ask because I was trying to get audioipc running in
> Firefox on macOS, which should work more-or-less right now, but I'm hitting
> issues with the sandbox that I suspect are related to the tempdir sandboxing
> the macOS sandbox enforces (special per-instance temp path which we don't
> use in audioipc plus a restriction to "regular" files only).

Perhaps not entirely surprisingly, this bug is now blocking similar sandbox improvements on Linux:
https://bugzilla.mozilla.org/show_bug.cgi?id=1386404

If the TMPDIR is changed before the child process launch, to point to the content-process-specific temporary directory, audioipc will fail to locate it's socket and fail.

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to Jed Davis [:jld] (⏰UTC-7) from comment #3)
> Setting up file descriptors like this isn't supported on Windows, but there
> might be something else that could be used there.

Good point, and given that we want this to work on Windows soonish, I'm taking an approach along the lines of paragraph 3 of comment 0.  I'll post patches soon.

Comment 7

[Matthew Gregan [:kinetik]]

https://github.com/djg/audioipc-2/pull/24

Comment 8

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4afb4d3c339652c35315e28ba4b71d6c318abb0&group_state=expanded

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: p1: Update media/audioipc with fd passing changes. r?kamidphish

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: p2: Update Rust deps for media/audioipc. r?kamidphish

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: p3: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process. r?

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: p3: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process. r?jld,kamidphish

Comment 13

[u480271]

Comment on attachment 8937385 [details] [diff] [review]
p1: Update media/audioipc with fd passing changes.  r?kamidphish

Review of attachment 8937385 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/audioipc/client/src/context.rs
@@ +196,5 @@
>              }
>          }
> +        unsafe {
> +            if super::G_SERVER_FD.is_some() {
> +            libc::close(super::G_SERVER_FD.take().unwrap());

nit: indentation

Comment 14

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

Comment on attachment 8937388 [details] [diff] [review]
p3: Bootstrap AudioIPC from Gecko IPC by passing server fd to content process.  r?jld,kamidphish

Review of attachment 8937388 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall.  Two minor things:

::: dom/media/CubebUtils.cpp
@@ +397,5 @@
> +                  MOZ_ASSERT(!sIPCConnection.IsValid());
> +                  sIPCConnection = aFD;
> +                },
> +                [](mozilla::ipc::ResponseRejectReason aReason) {
> +                  // handle send failure

Should there be something here to at least log the error?

@@ +437,5 @@
> +    // TODO: Don't use audio IPC when within the same process.
> +    MOZ_ASSERT(!sIPCConnection.IsValid());
> +    sIPCConnection = CreateAudioIPCConnection();
> +  } else {
> +    MOZ_ASSERT(sIPCConnection.IsValid());

How certain is it that the SendCreateAudioIPCConnection response will arrive before content can try to use WebAudio?  If we're not entirely sure about that, maybe this should be a MOZ_DIAGNOSTIC_ASSERT to get some coverage on non-debug builds where the timing might be different.

Comment 15

[Matthew Gregan [:kinetik]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #14)
> Should there be something here to at least log the error?

Good point, I'll add something when I land.

> @@ +437,5 @@
> > +    // TODO: Don't use audio IPC when within the same process.
> > +    MOZ_ASSERT(!sIPCConnection.IsValid());
> > +    sIPCConnection = CreateAudioIPCConnection();
> > +  } else {
> > +    MOZ_ASSERT(sIPCConnection.IsValid());
> 
> How certain is it that the SendCreateAudioIPCConnection response will arrive
> before content can try to use WebAudio?  If we're not entirely sure about
> that, maybe this should be a MOZ_DIAGNOSTIC_ASSERT to get some coverage on
> non-debug builds where the timing might be different.

I'll switch it to MOZ_DIAGNOSTIC_ASSERT; that's the behaviour I wanted anyway.  I think it'll be fine, but if it turns out to be too late in some situations we can wait on the SendCreateAudioIPCConnection response instead.

Comment 16

[Matthew Gregan [:kinetik]]

(In reply to Dan Glastonbury :kamidphish from comment #13)
> ::: media/audioipc/client/src/context.rs
> @@ +196,5 @@
> >              }
> >          }
> > +        unsafe {
> > +            if super::G_SERVER_FD.is_some() {
> > +            libc::close(super::G_SERVER_FD.take().unwrap());
> 
> nit: indentation

Fixed upstream and rebased patch.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de3a554033e6
Update media/audioipc with fd passing changes (2565ddfcacbc14e1ac0d66af1a4154c07050bba2).  r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fcb75943aa
Update Rust deps for media/audioipc.  r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/502337e7c6ae
Bootstrap AudioIPC from Gecko IPC by passing server fd to content process.  r=jld,kamidphish

Comment 18

[u480271]

Jed,

Is this enough for you to start locking down the linux sandbox, or are you still waiting on some more pieces?

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/de3a554033e6
https://hg.mozilla.org/mozilla-central/rev/a4fcb75943aa
https://hg.mozilla.org/mozilla-central/rev/502337e7c6ae

Comment 20

[Gian-Carlo Pascutto [:gcp]]

I'll test whether this allows tmpdir redirection now.

Comment 21

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

In local testing, this (+ bug 1394163) allows WebAudio and WebRTC to work with all of my sandboxing work-in-progress turned on (including bug 1401053: it doesn't quite pass Try yet, but the remaining failures don't seem to be media-related).

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Tmpdir redirection landed and didn't have any issues in media tests for a while.