Bug 942367 applies cross origin protections to media streams that are sourced from navigator.mozGetUserMedia and mozRTCPeerConnection. This should prevent access to the content of constrained streams. Webaudio cross origin protection only applies to media that is obtained using an HTML media element (<video> and <audio>). Media acquired using getUserMedia is - until bug 942367 - assumed to be part of the page origin and therefore accessible by default. With the WebRTC identity work, this is no longer true. Rather than make bug 942367 even larger, I've split this work out.

Since bug 942367 makes it possible for a stream principal to change, this provides stream consumers a way to learn of changes in a timely fashion.

Web audio currently assumes that any instance of MediaStream is accessible to content. This isn't true any more.

Removing webaudio workaround from tests added in Bug 942367.

RTCPeerConnection needs to pay attention to stream principals too.

Comment on attachment 8407099 [details] [diff] [review] 0004-Bug-966066-Adding-principal-observer-to-RTCPeerConne.patch Review of attachment 8407099 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1453,5 @@ > +void > +PeerConnectionImpl::PrincipalChanged(DOMMediaStream* aMediaStream) { > + nsIDocument* doc = GetWindow()->GetExtantDoc(); > + if (doc) { > + mMedia->UpdateSinkIdentity_m(doc->NodePrincipal(), mPeerIdentity); Having reviewed only half of these patches, I'm a little lost here. Can you help me with some context by explaining the 100-foot view of what transpires to have this called? A PeerConnection may have both audio and video streams attached. Can each of those have different principals? It seems here that PeerConnection reacts the same regardless of which stream has a change of principal. Why is that and what conflicts may happen here in practice? Where is the nullPrincipal in all of this, and what is the net effect? Thanks.

Comment on attachment 8407099 [details] [diff] [review] 0004-Bug-966066-Adding-principal-observer-to-RTCPeerConne.patch Review of attachment 8407099 [details] [diff] [review]: ----------------------------------------------------------------- The changes otherwise look benign, so in the sake of expediency let me r+ it. Then maybe we can take the education bit offline.

I'm sorry I didn't answer earlier, meetings... Let's see if I can't help you understand here, and then we don't have to worry about me having snuck one by you or something dodgy like that. The last thing I want is to rely on "trust me" here. If you aren't comfortable with this, feel free to ask for more review. (In reply to Jan-Ivar Bruaroey [:jib] from comment #5) > Can you help me with some context by explaining the 100-foot view of what > transpires to have this called? Apologies if this is far too high level. So, the basic premise is that a DOMMediaStream has an associated actor, or a principal. A principal is, at a high level, a web origin [RFC6454], basically, the scheme, host and port from a URI. In all cases bar one (this one, sadly), it is necessary for the MediaStream principal to be "subsumed" by the principal of the running script (i.e., the document) if the script is consuming content. Again, at a high level, principals with the same origin "subsume" each other. And when I say "consuming content", this is any case where the script might be able to read the stream contents, rendering to a canvas, webaudio, or sending over RTCPeerConnection. We use nsNullPrincipal to force isolation. nsNullPrincipal subsumes nothing and is subsumed by nothing (except the uber-nsSystemPrincipal, which subsumes all). This means that no page can read the contents of the stream. The stream can only be rendered to a user. The exception here is that we want to allow streams to be sent to a specific remote user. Then, what happens is that we tag the stream with that user's identity (here played by the PeerIdentity class). Then, when RTCPeerConnection starts sending, rather than saying "if script.subsumes(stream) then send", it has a second clause: "if script.subsumes(stream) OR script.peerIdentity == my.peerIdentity then send". Now this piece of code relates to the receiving end of RTCPeerConnection. Again, we use nsNullPrincipal to isolate streams that the peer is requesting that we isolate. But there are two triggers for that isolation: The first trigger is easy: set the peerIdentity constraint on RTCPeerConnection. The second trigger is tricky, it's when the remote peer asks for isolation. This specific piece of code deals with part of that. It's tricky because RTCPeerConnection is required to produce MediaStream instances *before* the peer has connected. And the only way we have for signaling a requirement for isolation is bound to the DTLS handshake. The signaling might lie about isolation just to get access to isolated stream. So, prior to a connection being established, we force streams to be isolated. Then, once we are connected and we discover that we don't need isolation, we can switch the principals to be the script principal and make content accessible. That's what UpdateRemoteStreamPrincipals_m does. UpdateSinkIdentity_m is on the other end of that principal-changing process. Say you have a stream that is initially isolated but subsequently becomes accessible. You need to learn about that change so that you can start sending media. Otherwise, if we were to relay remote streams to another RTCPeerConnection (as we theoretically can), streams would be stuck sending black or silence, unless the script was very careful to wait until content was accessible, and that turns out to be hard to test for. So, when peerIdentity changes (because we just authenticated our peer, for example), or a stream principal changes, we check to see if the isolation properties of streams have changed and switch to sending black/silence or video/audio as appropriate. > A PeerConnection may have both audio and video streams attached. Can each of > those have different principals? Yes, but this uses a very broad rule, similar to what is used in canvas. If anyone wants to mix non-isolated media with isolated media, then it all becomes isolated. With the existing APIs, to do anything else would be a complexity nightmare, in our code and in application code. > It seems here that PeerConnection reacts the same regardless of which stream > has a change of principal. Why is that and what conflicts may happen here in > practice? Where is the nullPrincipal in all of this, and what is the net > effect? Yes, any change to any stream causes all streams to be re-evaluated. It's certainly suboptimal, but the event is rare enough that this is unlikely to hurt that much. the usual procedure for principals is to undergo one transition: from script-accessible to nsNullPrincipal, from which return is usually not possible; here we have the opposite, so in the worst case streams change principal twice.

Thanks! A good explanation is never too high level. I stand by my r+.

Comment on attachment 8407096 [details] [diff] [review] 0002-Bug-966066-Adding-principal-observer-to-web-audio-Me.patch Review of attachment 8407096 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeExternalInputStream.cpp @@ +15,4 @@ > AudioNodeExternalInputStream::AudioNodeExternalInputStream(AudioNodeEngine* aEngine, TrackRate aSampleRate) > : AudioNodeStream(aEngine, MediaStreamGraph::INTERNAL_STREAM, aSampleRate) > , mCurrentOutputPosition(0) > + , mEnabled(false) There's a race condition here. SetEnabled sets mEnabled on the main thread and ProcessInput reads it on the MSG thread. You should send a message to the MSG thread to accomplish this instead.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > Comment on attachment 8407096 [details] [diff] [review] > 0002-Bug-966066-Adding-principal-observer-to-web-audio-Me.patch > > Review of attachment 8407096 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/AudioNodeExternalInputStream.cpp > @@ +15,4 @@ > > AudioNodeExternalInputStream::AudioNodeExternalInputStream(AudioNodeEngine* aEngine, TrackRate aSampleRate) > > : AudioNodeStream(aEngine, MediaStreamGraph::INTERNAL_STREAM, aSampleRate) > > , mCurrentOutputPosition(0) > > + , mEnabled(false) > > There's a race condition here. SetEnabled sets mEnabled on the main thread > and ProcessInput reads it on the MSG thread. You should send a message to > the MSG thread to accomplish this instead. mEnabled is Atomic<bool>. Maybe I've missed some nuance here (and that is highly likely). I thought that the memory barrier caused by the dispatch of CreateMessage [1] that puts the stream into the MSG would be suffient to ensure that the Atomic instance is found correctly. As long as that is true, then Atomic should work. As long as mCurrentOutputPosition is good, then this should be OK. If you prefer that I do a thread dispatch for this, then I'm happy to do that. Seems like more work though. [1] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#2853

The problem is that your SetEnabled call races with data actually being processed. I think this means, for example, that a PrincipalChanged call on the main thread could result in SetEnabled(true) being called while we're still processing data with the old principal, allowing a snippet of that data to be captured. I think the way to go here is to use AudioNodeStream::SetInt32Parameter to toggle the enabled state. That will delay the change taking effect so that it happens atomically with all other main-thread-initiated changes to the MediaStreamGraph taking place during this turn of the event loop. The question then is, is that sufficient to perfectly synchronize changes to the enabled state with the processing of data having the new principal? It is, if the new data is arriving due to graph configuration changes made by the main thread. It is not, if the new data is being fed into the MediaStream asynchronously by PeerConnection independent of the timing of the principal change on the main thread. I believe the latter is the case :-(. If the latter is the case, then I think we need to tag the actual audio blocks with some kind of principal identifier.

Thanks. Since all principal changes ultimately happen on the main thread, that means that the enacting of the change can be synchronized with other related changes on the MediaStreamGraph. (Unless there's magic in place to prevent a series of main-thread-initiated changes from being separated by a frame, the synchronization can't be perfect.) However, the risk of content "leaking" is worth highlighting. The model PeerConnection uses results in transitions from nsNullPrincipal to the script principal only. There are no returns from this state. There are no plans to change that. Thus, the risk, such as it is, is that streams are disabled slightly longer than they need to be. I'll make sure that the task is properly dispatched, and see what I can do about documenting the risks properly.

Try looks OK so far. About to upload revised patch set, including interdiff for the affected part. https://tbpl.mozilla.org/?tree=Try&rev=89f1b22a9a93

unrotting, r=roc

Including thread dispatch to change the MediaStreamAudioSourceNode to enabled. Still not convinced that this is currently needed for the current implementation, but it's probably best to stick to the discipline rather than risk slippage in later patches.

Interdiff for 0002, in case that helps. It's probably not useful.

Unrot; r=jib

Unrot, r=jib

https://hg.mozilla.org/integration/mozilla-inbound/rev/e343fc88dbff https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4c24365e3e https://hg.mozilla.org/integration/mozilla-inbound/rev/a913ba8e7e60 https://hg.mozilla.org/integration/mozilla-inbound/rev/1abbffdc82cc

https://hg.mozilla.org/mozilla-central/rev/e343fc88dbff https://hg.mozilla.org/mozilla-central/rev/bb4c24365e3e https://hg.mozilla.org/mozilla-central/rev/a913ba8e7e60 https://hg.mozilla.org/mozilla-central/rev/1abbffdc82cc