1212237 - WebRTC: Error recording remote media stream (Security Error: operation is insecure)

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect, P3)

Description

[AirMike23]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

Trying to automatically start Audio/Video recording of remote stream on stream flow.

1. on onaddstream callback of webrtc peer connection call function for checking if remote stream is available (e.g waitForRemote)
2. check if media element attached to remote stream is ready (check currentTime > 0)
3. on waitForRemote function callback initiate MediaRecorder and start recording remote stream


Actual results:

On trying to start remote stream approximately in 4 of 5 cases error is issued (Security Error: operation is insecure)

This error is not happening on recording of local stream, only on recording remote stream.

Additional check (media element is not paused and media element readyState >= HTMLMediaElement.HAVE_CURRENT_DATA) on media element attached to remote stream may solve the issue.



Expected results:

Recording of remote stream started.

Comment 1

[Maire Reavy [:mreavy]]

Andreas, Martin -- Are we compliant with the spec on setting the origin?

Comment 2

[Martin Thomson [:mt:]]

Ahh, I think that I see where this could happen.  When we establish a connection, prior to learning what the real status of the media is, we mark the stream with a NullPrincipal.  That ensures that we don't open access to media that might ultimately turn into an isolated stream.  The problem with MediaRecorder is that it makes a single check and then throws.  If that check is made immediately after receiving the stream (i.e., when the onaddstream event is fired), then the media is highly likely to be isolated.

The workaround is to wait for the DTLS connection to be established before starting MediaRecorder.  Unfortunately, we don't really have an event for that right now, at least until we implement the new stuff that is coming for state variables and events.

Comment 3

[Andreas Pehrson [:pehrsons]]

I agree with Martin's analysis on what's happening, I even saw the code for this the other day.

There's an issue noted in the spec elaborating a bit on this. See the bottom of 5.1.3 at [1].

The workaround Martin mentions is actually more of a proper fix IMO; MediaRecorder should just check the principal when it starts recording, i.e., when data is available on the stream and it raises the "start" event, see [2].

Our webidl for MediaRecorder has [Throws] on each method though the spec says none of them should throw. We should remove the throwing and fix this issue at the same time. MediaRecorder has an "error" event we can use instead of any throwing. Perhaps something you want to look at if you're doing MediaRecorder work Maire?


[1] https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#processing-remote-mediastreamtracks
[2] http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-MediaRecorder-start-void-long-timeslice

Comment 4

[Maire Reavy [:mreavy]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #3)
> Perhaps something you want to look at if you're doing MediaRecorder work Maire?

Yep, I'll take this.

Comment 5

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 8

[guest271314]

Has any progress been made towards fixing this bug?

Comment 9

[Byron Campen [:bwc]]

Do you have room on your plate for this? This is the cause of one of our more common intermittent test failures.

Comment 10

[Andreas Pehrson [:pehrsons]]

What do you suggest I do here?

The only known bug I'm aware of is that "unmute" may fire before the principal has propagated, but isn't that another bug?
Otherwise, we're to spec, no?

Comment 11

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] from comment #10)

What do you suggest I do here?

The only known bug I'm aware of is that "unmute" may fire before the principal has propagated, but isn't that another bug?
Otherwise, we're to spec, no?

Has the stuff you've described in comment 3 already been done, or ruled out?

Comment 12

[Andreas Pehrson [:pehrsons]]

(In reply to Byron Campen [:bwc] from comment #11)

(In reply to Andreas Pehrson [:pehrsons] from comment #10)

What do you suggest I do here?

The only known bug I'm aware of is that "unmute" may fire before the principal has propagated, but isn't that another bug?
Otherwise, we're to spec, no?

Has the stuff you've described in comment 3 already been done, or ruled out?

Inlining my answers into comment 3.

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

I agree with Martin's analysis on what's happening, I even saw the code for
this the other day.

There's an issue noted in the spec elaborating a bit on this. See the bottom
of 5.1.3 at [1].

This issue is gone from the latest webrtc-pc draft. jib or bwc, do you know what this was resolved as? (Since the issue ended with "For now, we simply make note of this issue, until it can be considered fully by the WG.")

The workaround Martin mentions is actually more of a proper fix IMO;
MediaRecorder should just check the principal when it starts recording,
i.e., when data is available on the stream and it raises the "start" event,
see [2].

The MediaRecorder spec defines what to do here now, and it will throw a SecurityError in start(). To me this means we're doing the right thing, modulo the potential "unmute" bug mentioned in comment 10, and our lack of the "isolationchange" event that's supposed to notify content that it's ok to use the tracks per bug 1475360. If we think the above is the right approach it would deserve some spec work IMO.

Our webidl for MediaRecorder has [Throws] on each method though the spec
says none of them should throw. We should remove the throwing and fix this
issue at the same time. MediaRecorder has an "error" event we can use
instead of any throwing. Perhaps something you want to look at if you're
doing MediaRecorder work Maire?

[1]
https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html#processing-
remote-mediastreamtracks
[2]
http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-
MediaRecorder-start-void-long-timeslice

Comment 13

[Byron Campen [:bwc]]

Ok, so I guess the "right" way of fixing this is to fix bug 1475360, and then modify our test code to pay attention to that.

We might be able to work around by adding a short delay.

Comment 14

[Andreas Pehrson [:pehrsons]]

For now making sure "unmute" fires after the principal has been fully updated would also work. If we modify our tests to look for that.

Comment 15

[Byron Campen [:bwc]]

There's nothing in the spec that says anything about the timing of "unmute" vs the update of the principal, unfortunately.

Comment 16

[guest271314]

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Comment 17

[Byron Campen [:bwc]]

(In reply to guest271314 from comment #16)

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Because the PeerConnection API is not designed to disable this checking if both ends happen to be running in the same tab. This is also why the media being passed between the two PeerConnections is encrypted, regardless of where those PeerConnections actually are. PeerConnection is, at its heart, an API that starts with the assumption that peer-to-peer media is being exchanged between two different browser instances (or, a browser instance and some external media entity like a gateway).

Comment 18

[guest271314]

(In reply to Byron Campen [:bwc] from comment #17)

(In reply to guest271314 from comment #16)

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Because the PeerConnection API is not designed to disable this checking if both ends happen to be running in the same tab.

Can that be changed to reflect the case where the sole purpose of using PeerConnection is for the functionality of RTCRtpSender.replaceTrack() to overcome the currently specified limitations of MediaRecorder?

This is also why the media being passed between the two PeerConnections is encrypted, regardless of where those PeerConnections actually are. PeerConnection is, at its heart, an API that starts with the assumption that peer-to-peer media is being exchanged between two different browser instances (or, a browser instance and some external media entity like a gateway).

What are the security implications of two instances of RTCPeerConnection being used at the same tab in the same browser; that is, when the initial assumption that peer-to-peer media is being exchanged between two different browser instances, is not the case?

Comment 19

[Byron Campen [:bwc]]

(In reply to guest271314 from comment #18)

(In reply to Byron Campen [:bwc] from comment #17)

(In reply to guest271314 from comment #16)

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Because the PeerConnection API is not designed to disable this checking if both ends happen to be running in the same tab.

Can that be changed to reflect the case where the sole purpose of using PeerConnection is for the functionality of RTCRtpSender.replaceTrack() to overcome the currently specified limitations of MediaRecorder?

No, because doing so would violate the w3c specification. What you're doing is not a use-case that the PeerConnection API was designed for, so I'm afraid you're going to have to live with some friction, or find another way to do what you want.

Comment 20

[guest271314]

(In reply to Byron Campen [:bwc] from comment #19)

(In reply to guest271314 from comment #18)

(In reply to Byron Campen [:bwc] from comment #17)

(In reply to guest271314 from comment #16)

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Because the PeerConnection API is not designed to disable this checking if both ends happen to be running in the same tab.

Can that be changed to reflect the case where the sole purpose of using PeerConnection is for the functionality of RTCRtpSender.replaceTrack() to overcome the currently specified limitations of MediaRecorder?

No, because doing so would violate the w3c specification. What you're doing is not a use-case that the PeerConnection API was designed for, so I'm afraid you're going to have to live with some friction, or find another way to do what you want.

The W3C specification has been and is constantly being changed.

That does not answer the question as to what the security implications of RTCPeerConnection being used at the same tab at the same browser are.

Comment 21

[Byron Campen [:bwc]]

(In reply to guest271314 from comment #20)

(In reply to Byron Campen [:bwc] from comment #19)

(In reply to guest271314 from comment #18)

(In reply to Byron Campen [:bwc] from comment #17)

(In reply to guest271314 from comment #16)

When the "remote" stream is in fact a local PeerConnection why is there a "Security Error" thrown at all by MediaRecorder?

Because the PeerConnection API is not designed to disable this checking if both ends happen to be running in the same tab.

Can that be changed to reflect the case where the sole purpose of using PeerConnection is for the functionality of RTCRtpSender.replaceTrack() to overcome the currently specified limitations of MediaRecorder?

No, because doing so would violate the w3c specification. What you're doing is not a use-case that the PeerConnection API was designed for, so I'm afraid you're going to have to live with some friction, or find another way to do what you want.

The W3C specification has been and is constantly being changed.

That does not answer the question as to what the security implications of RTCPeerConnection being used at the same tab at the same browser are.

What you're trying to do here is very far outside the scope of what the webrtc-pc working group was chartered to do. You might have some luck trying to get a "seamless switching" feature incorporated into the MediaRecorder spec, however.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] from comment #12)

This issue is gone from the latest webrtc-pc draft. jib or bwc, do you know what this was resolved as?

The section was deleted here. The conclusion was that early media cannot happen. Specifically: packets cannot arrive before the DTLS fingerprint. Thus, listening to the mute event should suffice, according to webrtc-pc, which says to fire the event when it "receives data on an RTP source".

It sounds from bug 1542616 comment 17 that we fire unmute before the principal is ready (or set the principal too late).

I've verified this using this fiddle where I wait for unmute, and I still get SecurityError. That's the bug to fix.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Oddly, to work around this, I find I have to wait as much as 1000 ms! What can possibly take that long?

Comment 24

[Andreas Pehrson [:pehrsons]]

It must be content, see https://bugzilla.mozilla.org/show_bug.cgi?id=1542616#c17
If you want more details, MediaStreamTrack logs should shine some light on what's happening.

If early media cannot happen, would it be an option to start with a friendly principal and when dtls is conncected switch to the secure one if needed? Becoming secure would be immediate (on main thread) since a secure and friendly principal merge into a secure one.

A track can start with any principal, but a change later on requires verification in content to guarantee that no content leaks under the wrong principal.

Comment 25

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] from comment #24)

It must be content, see https://bugzilla.mozilla.org/show_bug.cgi?id=1542616#c17
If you want more details, MediaStreamTrack logs should shine some light on what's happening.

If early media cannot happen, would it be an option to start with a friendly principal and when dtls is conncected switch to the secure one if needed? Becoming secure would be immediate (on main thread) since a secure and friendly principal merge into a secure one.

Hmm. I guess that would work.

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yeah that might work. But if principal changes still have this 1 second delay, then we need to ensure we don't leak the initial frame(s) of media isolated by peerIdentity.

Comment 27

[Andreas Pehrson [:pehrsons]]

Like I said, making it more secure is immediate. The only trick is to set the principal on main thread before adding the first frame on whatever other thread, at least if that frame was meant to go with a secure principal.

Comment 28

[Andreas Pehrson [:pehrsons]]

Byron: do we have a way to test in automation the case where peerIdentity is not set but the ALPN negotiation still tells us that privacy is requested?

Comment 29

[Andreas Pehrson [:pehrsons]]

Apart from testing that case, this is getting ready: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5be6c6542f823104b07b606bd708527534d9bf

Comment 30

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] from comment #28)

Byron: do we have a way to test in automation the case where peerIdentity is not set but the ALPN negotiation still tells us that privacy is requested?

I don't see a way to do that right now, no.

Comment 31

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1212237 - Add mochitest to test that freshly received tracks are not isolated. r?jib

Comment 32

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1212237 - Use MediaRecorder to test peerIdentity track isolation. r?jib

Depends on D48944

Comment 33

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1212237 - Be explicit about principals for received tracks. r?bwc

This patch fixes two things:

1. A potential threading issue in setting and reading the PrincipalHandle for
   MediaPipelineReceiveVideo.
2. Plumbs the PrincipalHandle down to the receiving MediaPipelines right from
   the start. It hasn't been necessary in the past as initially the principal
   handed to a track's constructor is trusted, but it's good form and will help
   clarify the situation for the next patch which switches the initial
   principal from always-private to mostly-non-private. In most cases this
   principal does no longer get updated after the track's been created, so it
   helps to have a PrincipalHandle that matches the track's principal.

Depends on D48945

Comment 34

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1212237 - Use a content principal by default for received tracks. r?bwc

This swaps things around so that received tracks in peer connections have a
content principal instead of a null principal initially.

This puts an extra requirement on us to not output any frames under the content
principal after ALPN has been negotiated with requested privacy, but before this
private principal has been signaled to the MediaPipelines. The private principal
is signaled from the STS thread, via the main thread, whereas media is consumed
directly by MediaPipelines after being received on the STS thread.

This patch adds an extra signaling path to tell MediaPipelines to wait for a
private principal before adding data into the graph, that is signaled directly
from the STS thread.

This lets all MediaStream and MediaStreamTrack APIs consume received tracks
directly after getting exposed to JS without errors. In case privacy is later
requested, consumers that have already been set up must handle this on the fly.

Depends on D48946

Comment 35

[Martin Thomson [:mt:]]

Thanks for taking this on Andreas, this is the shape of the fix that I had always wanted to have, but didn't have the guts to attempt.

Comment 36

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1212237 - s/nsIEventTarget/nsISerialEventTarget/ in media/webrtc. r?bwc

nsISerialEventTarget is more semantically accurate for these uses, as the
dispatched runnables cannot run in parallel. It also allows us to use
InvokeAsync in future patches, as that function only takes nsISerialEventTarget.

Depends on D48946

Comment 37

[Byron Campen [:bwc]]

I'm going to wait until this lands before landing bug 1588588, since there are going to be conflicts.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4adcbfd99da0
Add mochitest to test that freshly received tracks are not isolated. r=jib
https://hg.mozilla.org/integration/autoland/rev/e406c203d2a9
Use MediaRecorder to test peerIdentity track isolation. r=jib
https://hg.mozilla.org/integration/autoland/rev/184b8485ae32
Be explicit about principals for received tracks. r=bwc
https://hg.mozilla.org/integration/autoland/rev/c23272ab755f
s/nsIEventTarget/nsISerialEventTarget/ in media/webrtc. r=bwc
https://hg.mozilla.org/integration/autoland/rev/c3f52449c92c
Use a content principal by default for received tracks. r=bwc

Comment 39

[Cristian Brindusan [:cbrindusan]]

Backed out 7 changesets (Bug 1212237, Bug 1588055) for mochitest failures at test_peerConnection_recordReceiveTrack.html.

Backout: https://hg.mozilla.org/integration/autoland/rev/37769ce983ece0ad9e7213b675dbfef5ee1c8b7d

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=15f2829db807a503a42e8d35ab5cbdf6f05d65fe

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=274727458&repo=autoland&lineNumber=3148

Comment 40

[Andreas Pehrson [:pehrsons]]

This might be because those devices are using hw decoders, whose frames the MediaRecorder cannot access the pixels of yet.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ded4b6c5fba
Add mochitest to test that freshly received tracks are not isolated. r=jib
https://hg.mozilla.org/integration/autoland/rev/14b3d64aeba9
Use MediaRecorder to test peerIdentity track isolation. r=jib
https://hg.mozilla.org/integration/autoland/rev/f828ded989ea
Be explicit about principals for received tracks. r=bwc
https://hg.mozilla.org/integration/autoland/rev/fafd810651ef
s/nsIEventTarget/nsISerialEventTarget/ in media/webrtc. r=bwc
https://hg.mozilla.org/integration/autoland/rev/28a50db0c006
Use a content principal by default for received tracks. r=bwc

Comment 42

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9ded4b6c5fba
https://hg.mozilla.org/mozilla-central/rev/14b3d64aeba9
https://hg.mozilla.org/mozilla-central/rev/f828ded989ea
https://hg.mozilla.org/mozilla-central/rev/fafd810651ef
https://hg.mozilla.org/mozilla-central/rev/28a50db0c006