1405599 - (CVE-2018-5109) Audio capture can start under wrong origin

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Andreas Pehrson [:pehrsons]]

Attachment: test.html

Found while investigating bug 1403186.

STR:
1 host test.html locally with e.g., `python -m SimpleHTTPServer`
2 `MOZ_LOG=timestamp,MediaManager:5 ./mach run localhost:8000/test.hml`

Expected:
No gUM permission prompt

Actual:
gUM permission prompt that asks for permission for audio capture for "mozilla.github.io". Also, clicking "Allow" results in starting audio capture.


I have not been able to circumvent the permission prompt. So storing a persistent permission for https://mozilla.github.io still results in a prompt.

I have not been able to start video capture this way. There is a guard at [1] which is probably what is catching this.

I am not sure whether one can do anything with the audio. It cannot be ruled out.

The captured audio stops after a while, presumable because GC collects the track. I have not investigated persisting the capture for longer.


[1] http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/dom/media/systemservices/CamerasParent.cpp#640

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Guard mediaDevices APIs from an invalid window

This adds similar guards to `navigator.mediaDevices.getUserMedia` and `navigator.mediaDevices.enumerateDevices` as are present on `navigator.mozGetUserMedia` and the `navigator.mediaDevices` getter.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8915067 [details] [diff] [review]
Guard mediaDevices APIs from an invalid window

Is there a spec saying to do these checks?  If so, what exception does it say to throw, and what actual check does it say to do?  I doubt it says to throw NS_ERROR_NOT_AVAILABLE!

If there's no spec saying to do this, have you raised a spec issue?  The spec should presumably say something here.

Comment 3

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

Andreas, you've verified this patch fixes the problem?

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #2)
> Is there a spec saying to do these checks?

There's no mention of anything like this in the mediacapture spec [1][2].

Note that this situation is the result of fuzzing with window.location.href = newurl; From attachment 8915061 [details]:

      try { o1 = new XMLHttpRequest({mozAnon: true}) } catch(e) { }
      try { o2 = window.navigator } catch(e) { }
      try { o3 = o2.plugins } catch(e) { }
      try { o4 = o2.mediaDevices } catch(e) { }
      window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html"
      try { o1.open("T", "",false) } catch(e) { }
      try { o1.send("") } catch(e) { }
      try { o4.getUserMedia({audio: true, video: false, fake: false}) } catch(e) { }

I don't know enough about how window.location.href works here, but once this run of the event loop has finished, will there even any code left to observe a returned promise from o4.getUserMedia()?

> If there's no spec saying to do this, have you raised a spec issue?  The spec should presumably say something here.

Is this something individual domain specs are expected to handle? Is there a common pattern, or existing spec prose you could point me to as an example?

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-navigatorusermedia
[2] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-navigatorusermedia-getusermedia()

Comment 4

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

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #3)
> will there even any code left to observe a returned promise from o4.getUserMedia()?

I mean in the case where this patch would reject with NS_ERROR_NOT_AVAILABLE.

Comment 5

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

I guess the microtask queue would still run, so nm. Would it be better to return a Promise that never resolves here?

Comment 6

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

Patch aside, I'm having trouble wrapping my head around what code, if anything, can still be alive from the original test.html page once 

    window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html"

has gotten far enough that we're seeing that origin in the permission prompt, and users had moused over to click "Accept"? - Is there an vector here for an attacker to record the user?

Comment 7

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

Also, can we add a test for this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Why does code have to be alive from the test.html page?

You can load a page in a subframe (or window.open it), then grab the relevant objects from it, then navigate the page to a new origin, then call methods on the objects you grabbed, no?

> There's no mention of anything like this in the mediacapture spec [1][2].

Ok, that presumably needs fixing.

> Is this something individual domain specs are expected to handle?

Yes, depending on what "this" is.  In general, specs are expected to think about what happens if their APIs are invoked in the context of a document that is not "active" or "fully active" in the sense of https://html.spec.whatwg.org/multipage/browsers.html#active-document and <https://html.spec.whatwg.org/multipage/browsers.html#fully-active>.  For many APIs it's not a problem.  For some it could be.

In any case, it seems like the problem on our end is that the prompt's origin is derived from a WindowProxy somehow, not from a Window.  It's entirely possible that the spec simply assumes people won't do that (and indeed, they should not!).

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Ok, that presumably needs fixing.

That's _if_ the spec assumes you go through windowproxies at some point.

But even if it doesn't.  I suspect that having permission prompts coming up for non-fully-active documents is weird and should not happen.  Perhaps this should be handled by https://w3c.github.io/permissions/#request-permission-to-use

Comment 10

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #3)
> Andreas, you've verified this patch fixes the problem?

Yes, as in with the test case the permission prompt no longer appears. Just as getting navigator.mediaDevices for the first time would throw too.

FWIW, This is the relevant bit I find in mediacapture-main [1]:

> When the getUserMedia() method is called, the User Agent MUST run the following steps:
> ...
> 6. Run the following steps in parallel:
> ...
>   4. Let originIdentifier be the [HTML52] top-level browsing context's origin.
> 
>   5. If the current [HTML52] browsing context is a [HTML52] nested browsing context whose origin is different from originIdentifier, let originIdentifier be the result of combining originIdentifier and the current browsing context's origin.
> 
>   6. For the origin identified by originIdentifier, request permission for use of the devices, while considering all devices attached to a live MediaStreamTrack in the current browsing context to have permission status "granted", resulting in a set of provided media.
> ...
>      If the result of the request is "granted", then for each device that is sourcing the provided media, using the device's deviceId, deviceId, set [[devicesLiveMap]][deviceId] to true, if it isn’t already true, and set the [[devicesAccessibleMap]][deviceId] to true, if it isn’t already true.
> 
>      If the result is "denied", jump to the step labeled Permission Failure below. If the user never responds, this algorithm stalls on this step.
> 
>      If the user grants permission but a hardware error such as an OS/program/webpage lock prevents access, reject p with a new DOMException object whose name attribute has the value NotReadableError and abort these steps.
> 
>      If the result is "granted" but device access fails for any reason other than those listed above, reject p with a new DOMException object whose name attribute has the value AbortError and abort these steps.

This is outside my comfort zone, but on a hunch, does it mean anything that "Let originIdentifier be the [HTML52] top-level browsing context's origin." happens in parallel here (e.g., after `window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html"`)?

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia

Comment 11

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

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8)
> Why does code have to be alive from the test.html page?
>
> You can load a page in a subframe (or window.open it), then grab the
> relevant objects from it, then navigate the page to a new origin, then call
> methods on the objects you grabbed, no?

Good idea. I can repro that (get the misleading prompt) by hitting "Run" on https://jsfiddle.net/jib1/aLbn85ku/8/ roughly ~1/20th of the time, sometimes much more frequently. But the promise never resolves when I hit "Allow", so maybe no recording risk.

More good news, even when I have persistent permission already granted for the good site (webrtc-landing), I still get prompted (even though we haven't implemented bug 1299577 which is step 5 in the prose mentioned in comment 10). In the prompt I still get, if I check the "Remember this decision" box, the prompt UX says "Your connection is not secure...". Not sure how it figured that out since everything is https here, but go principals! Same for camera (s/audio: true/video: true/).

> Yes, depending on what "this" is.  In general, specs are expected to think
> about what happens if their APIs are invoked in the context of a document
> that is not "active" or "fully active" in the sense of
> https://html.spec.whatwg.org/multipage/browsers.html#active-document and
> <https://html.spec.whatwg.org/multipage/browsers.html#fully-active>.  For
> many APIs it's not a problem.  For some it could be.

Good point, though from a quick search I see very few specs test for "fully active" on initial invocation. For those that do, I see InvalidStateError, TypeError, SecurityError. I guess we can take our pick?

> In any case, it seems like the problem on our end is that the prompt's
> origin is derived from a WindowProxy somehow, not from a Window.  It's
> entirely possible that the spec simply assumes people won't do that (and
> indeed, they should not!).

How do we tell if it's a proxy?

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> I suspect that having permission prompts coming up for non-fully-active documents is weird and should not happen.
> Perhaps this should be handled by
> https://w3c.github.io/permissions/#request-permission-to-use

I like that idea. Then we could throw NotAllowedError here.

Comment 12

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

Cc'ing Florian to see if he thinks comment 9 is an issue that extends to permissions outside of getUserMedia, or it's not if there's precedence on how to handle this.

Comment 13

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

(In reply to Andreas Pehrson [:pehrsons] from comment #10)
> > 6. Run the following steps in parallel:
> > ...
> >   4. Let originIdentifier be the [HTML52] top-level browsing context's origin.

Step 4 should probably be lifted outside of the "in parallel" steps here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

>   4. Let originIdentifier be the [HTML52] top-level browsing context's origin.

Oh, boy.

1)  A browsing context does not have a concept of "origin".
2)  Touching a browsing context in an "in parallel" section is not OK.
3)  The top-level browsing context may not have anything to do with the API being invoked,
    as in the testcases in this bug.

Basically, this algorithm needs to capture the origin from a sane place before going parallel.  Right now it's not doing any of those things.

> does it mean anything that "Let originIdentifier be the [HTML52] top-level browsing
> context's origin." happens in parallel

Yes, it means that it's bogus spec text (or a threadsafety violation in implementation terms).

> But the promise never resolves when I hit "Allow", so maybe no recording risk.

Right, behavior of promises in navigated-away-from documents is ... complicated.  Also not specified anywhere; the ES spec which defines promises has no concept of navigation.

> I guess we can take our pick?

Works for me.  Just don't use Gecko-internal XPConnect error types like NS_ERROR_NOT_AVAILABLE.  ;)

> How do we tell if it's a proxy?

I'm not sure what you mean.  In spec terms, a WindowProxy is the JS object that more or less corresponds to a browsing context, effectively.  It's the _only_ JS object for windows exposed to script.  It generally forwards access on it to the current Window in the browsing context.  A Window is the thing that's actually loaded at any given time.

In implementation terms in Firefox, if you're in JS, you have a WindowProxy.  If you're in C++, and have a docshell, that's a browsing context.  If you have an nsGlobalWindow, it could be an "inner" (spec Window) or an "outer" (spec WindowProxy).

Looks to me like the spec quoted above is looking at browsing contexts (though it doesn't define how it does that, exactly).  So yes, it's going to get results that may have nothing to do with anything.  It needs to not do that unless it's enforcing that those browsing contexts are somehow related to the API being called.  If it checks "fully active", that's good enough; then it could examine the origins of the active documents in the relevant browsing contexts.

Comment 15

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #12)
> Cc'ing Florian to see if he thinks comment 9 is an issue that extends to
> permissions outside of getUserMedia

I have not actually verified, but yes I think this would not be specific to getUserMedia.

Comment 16

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

I've filed
https://github.com/w3c/permissions/issues/162
https://github.com/w3c/mediacapture-main/pull/490
https://github.com/w3c/mediacapture-main/pull/491

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: GetUserMedia should reject when document not fully active

This should implement the guard per [1]. Let me know if the fully active check is missing something, it's the same as the old mozGetUserMedia one.

[1] https://github.com/w3c/mediacapture-main/pull/491/files

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8918342 [details] [diff] [review]
GetUserMedia should reject when document not fully active

Sorry, this doesn't work. I was trigger happy here.

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: GetUserMedia should reject when document not fully active

This is a MediaStreamError for now, since our OnError() path doesn't support DOMException. We should turn the MediaStreamErrors into DOMExceptions some time in the future.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8918778 [details] [diff] [review]
GetUserMedia should reject when document not fully active

Please don't reinvent nsPIDOMWindowInner::IsCurrentInnerWindow.

But also, this is not checking for "fully active".  It's checking for "active".  Kind of.  It's possible for aWindow to not be current but its document to be active (e.g. in a document.open()) case.  That's why we have nsIDocument::IsCurrentActiveDocument.  And again, that doesn't test "fully active".  We don't actually have a convenient way of testing "fully active" right now...

Comment 21

[Andreas Pehrson [:pehrsons]]

Attachment: GetUserMedia should reject when document not fully active

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922806 [details] [diff] [review]
GetUserMedia should reject when document not fully active

The commit message still doesn't match the code, per comment 20.  Which one are you actually trying to do?

Comment 23

[Andreas Pehrson [:pehrsons]]

Attachment: GetUserMedia should reject when document not fully active

Best would be to follow the spec and check for fully active. Does this work?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8923462 [details] [diff] [review]
GetUserMedia should reject when document not fully active

>+  auto isFullyActive = [](nsPIDOMWindowInner* win)

Is there a good reason for this to be a lambda instead of a static function?

>+      if (!win->IsCurrentInnerWindow()) {

Again, this is _not_ the right test for the spec concept of "active document".  Please use the nsIDocument API I pointed to above.

>+      nsCOMPtr<nsIDOMElement> domFrameElement = context->GetFrameElement();

If we know we have an outer, we should really be able to use nsGlobalWindow::GetRealFrameElementOuter.

>+      nsIDocument* owner = frameElement->GetOwnerDocument();

frameElement->OwnerDoc(), which is guaranteed to never be null.

Comment 25

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

Comment on attachment 8923462 [details] [diff] [review]
GetUserMedia should reject when document not fully active

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

r=me once you get bz's r+

::: dom/media/MediaManager.cpp
@@ +2179,5 @@
>      onFailure->OnError(error);
>      return NS_OK;
>    }
> +
> +  auto isFullyActive = [](nsPIDOMWindowInner* win)

wow, is there really no common helper function for this?

@@ +2212,5 @@
> +    }
> +  };
> +  if (!isFullyActive(aWindow)) {
> +    RefPtr<MediaStreamError> error =
> +        new MediaStreamError(aWindow, NS_LITERAL_STRING("InvalidStateError"));

Error looks good.

Comment 26

[Andreas Pehrson [:pehrsons]]

Attachment: GetUserMedia should reject when document not fully active

Hopefully this is it. Manual test is still good.

Comment 27

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8924930 [details] [diff] [review]
GetUserMedia should reject when document not fully active

Carries forward r=jib.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8924930 [details] [diff] [review]
GetUserMedia should reject when document not fully active

r=me.  Sorry for all this back-and-forth...

Comment 29

[Andreas Pehrson [:pehrsons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/574820f4c220065da36bbc6b78ac2b9b4adb676f

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/574820f4c220

Comment 31

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this goes back to the Fx42 timeframe? Doesn't seem to meet the uplift criteria for ESR52, but feel free to set the status to affected and request approval if you feel strongly otherwise.