961512 - (CVE-2014-1499) WebRTC permission prompt can show the wrong domain name, potentially making it possible for the page to spoof the domain name the access to the webcam/mic is requested from

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[(no longer active)]

Attachment: Screenshot

I was on hacker news and I clicked a link to rtc.io, went to their demo pages, clicked on http://hughsk.github.io/post-process/, and I think the browser janked for a moment and I pressed back to get back to the hacker news webpage, which is when the WebRTC prompt appeared, showing the wrong domain name as can be seen in the screenshot.  Now, the doorhanger prompt is attached to the hacker news page as if the request originally came from that page.

This seems to be a race condition which an attacker may be able to employ as one step to convince the user that a legit website is trying to access their webcam/mic.

Comment 1

[Daniel Veditz [:dveditz]]

We have a bunch of folks who like to earn bounties off this kind of navigation/timing location confusion :-(

Comment 2

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

I think this is Bug 949907, because of the async GetUserMediaDevices call (w/callback) to get the device list (off main thread) right before the permission prompt is stood up, and there are zero checks of windowIDs across that particular request.

If we agree they're related, should I promote 949907 to a sec bug or not touch it and put my patches here instead?

I have a patch that fixes 949907 but I'm unhappy with it (breaks b2g for one) so I plan to consolidate the discordant uses of windowIDs here in a way that doesn't break b2g.

---

On the upside, I'm almost positive that nothing would have happened if you pressed "Share" (did you try it?), because the navigated-away page's windowId would no longer be on the list of outstanding gUM requests, so the code should stop there.

Comment 3

[(no longer active)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> I think this is Bug 949907, because of the async GetUserMediaDevices call
> (w/callback) to get the device list (off main thread) right before the
> permission prompt is stood up, and there are zero checks of windowIDs across
> that particular request.

That is at least plausible.

> If we agree they're related, should I promote 949907 to a sec bug or not
> touch it and put my patches here instead?

Please post your patch here, and then we will test to see if they also fix bug 949907.

> I have a patch that fixes 949907 but I'm unhappy with it (breaks b2g for
> one) so I plan to consolidate the discordant uses of windowIDs here in a way
> that doesn't break b2g.
> 
> ---
> 
> On the upside, I'm almost positive that nothing would have happened if you
> pressed "Share" (did you try it?), because the navigated-away page's
> windowId would no longer be on the list of outstanding gUM requests, so the
> code should stop there.

I did try it, yes and nothing visible happened.

Comment 4

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

Attachment: Add checks to GetUserMediaDevices() + untie GetUserMediaRequest obj from DOM (WIP)

Fixes the dead object test-case in Bug 949907https://bugzilla.mozilla.org/attachment.cgi?id=8347060 for me.

It burns on b2g however https://tbpl.mozilla.org/?tree=Try&rev=e99a3f7ba00f because they apparently used GetParent() to get the innerWindow instead of the windowId passed. I hope to clean this up.

Comment 5

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

https://bugzilla.mozilla.org/attachment.cgi?id=8347060

Comment 6

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

Attachment: Add checks to GetUserMediaDevices() + untie GetUserMediaRequest obj from DOM - WIP (2)

Rebased.

Comment 7

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

Attachment: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2)

A cleaner patch.

- GetUserMediaDevices() now omits calling callbacks if the inner window the
  call originated from has disappeared/navigated away.

  Actually, as I'm writing this, that's not entirely what it does, since it goes
  through an outerWindowID, but it checks that the current innerWindow of that
  outerWindow is on the "outstanding gUM request-list", which is close, and should
  kill most critters. In theory, if the second window that was navigated to ALSO
  requested gUM, then I suppose it is still possible that this dud request double
  would still make it through. Let me know what you think, and I can take another
  pass at this.

- Redid calls to navigator.GetUserMediaDevices() to go through the
  contentWindow so it gets the window it needs without adding a new argument.
  Hopefully there's no reason it was done the other way.

- Changed the GetUserMediaRequest object to no longer be a child the content
  window (which AFAIK was the "dead object on line 73" in Bug 949907).
  Now that the other changes in this patch prevent line 73 from executing in the
  trouble case, this change may technically no longer be needed, but it was
  probably always wrong to have the lifespan of that request object - which is
  just passed from c++ chrome to js chrome - be tied to anything DOM.

- Schien: On the last part, if you could take a look at the change in
  MediaPermissionGonk.cpp to make sure it works, is compatible and makes sense
  to you that would be great. It compiled, but I'm not set up to flash at the
  moment. Thanks!

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> > If we agree they're related, should I promote 949907 to a sec bug or not
> > touch it and put my patches here instead?
> 
> Please post your patch here, and then we will test to see if they also fix
> bug 949907.

FWIW, if there is an existing public bug that can be used as a suitable place to post not-obviously-security-sensitive patches, it's generally better to put the patches in the public bug to avoid drawing attention to their security impact.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8364188 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2)

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

::: dom/media/MediaPermissionGonk.cpp
@@ +411,5 @@
> +  if (!outerWindow) {
> +    MOZ_ASSERT(false, "No outer window");
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsPIDOMWindow *innerWindow = outerWindow->GetCurrentInnerWindow();

Does this means we use the latest window instead of the requester window? I'm ok with this approach, however, I'm thinking if canceling GetUserMedia request is a better choice while we discover the current window is different from the requester window.

Comment 10

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #8)
> FWIW, if there is an existing public bug that can be used as a suitable
> place to post not-obviously-security-sensitive patches, it's generally
> better to put the patches in the public bug to avoid drawing attention to
> their security impact.

Good idea. How about we land the final patch from Bug 949907 (with a mock lgtm review) after we've done review here? That way we can discuss security in the review.

Comment 11

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

(In reply to Shih-Chiang Chien [:schien] from comment #9)
> > +  nsPIDOMWindow *innerWindow = outerWindow->GetCurrentInnerWindow();
> 
> Does this means we use the latest window instead of the requester window?
> I'm ok with this approach, however, I'm thinking if canceling GetUserMedia
> request is a better choice while we discover the current window is different
> from the requester window.

This change brings the Gonk version in line with how the desktop and android versions work. Those are written in JS and seem only capable of dealing in outer windows, which is why GetUserMediaRequest returns an outerWindowId. I need a solution for all three platforms.

If someone knows of a way to get an outer window from an innerWindowId in JS, let me know, as that would simplify things and let me pass out an innerWindowId here. If not, I'll look into passing both, or maybe using the existing callId to disambiguate the request from a more recent request (the remaining problem I mention in Comment 7).

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8364188 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2)

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

::: dom/media/MediaPermissionGonk.cpp
@@ +405,5 @@
>  {
>    nsString callID;
>    req->GetCallID(callID);
>  
> +  nsPIDOMWindow *outerWindow = static_cast<nsPIDOMWindow*>

Suppose we need a strong reference here, i.e. nsCOMPtr. The innerWindow below should also use nsCOMPtr, too.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8364188 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (2)

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

You also need to modify MediaPermissionRequest::GetWindow() and MediaPermissionRequest::GetOwner() because they both depend on GetUserMediaRequest::GetParentObject().
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaPermissionGonk.cpp#160
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaPermissionGonk.cpp#194

Comment 14

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

Attachment: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (3)

This patch uses the callID to make sure the innerWindow is the requesting window, and fixes nits. I was happy to solve it without adding any more arguments to GetUserMediaRequest.

However, looking at the Gonk code and the four other uses of GetParentObject() you point out, it really looks like the B2G code needs an innerWindow (I can't tell for sure, can you verify?) - If so, I may need to bite the bullet and just add an innerWindow id, otherwise I worry I'm adding a similar problem to B2G in exchange (even though there are no async calls there, it seems like window.location can change between processing of events on the main thread).

That's probably what I'll do, so stay tuned for yet another patch. :-/ Just wanted to upload this version for posterity in case we change our minds.

Comment 15

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

Attachment: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (4)

This patch adds a new innerWindowID parameter to the GetUserMediaRequest object. This turned out to work quite nicely, so I'm going with this solution.

Comment 16

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

Try - https://tbpl.mozilla.org/?tree=Try&rev=d32113cbce64

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8370305 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (4)

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

@jib, do we have a minimal reproduce steps for this bug? I'm a bit worried about that B2G is not fixed by this patch because no logical change in MediaPermissionGonk.cpp.

Keep r? as a reminder to myself.

::: dom/media/GetUserMediaRequest.cpp
@@ +40,5 @@
>  }
>  
>  nsISupports* GetUserMediaRequest::GetParentObject()
>  {
> +  return nullptr;

I'm not sure if |GetUserMediaRequest| matches the scenario of returning nullptr here according to https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class. How about asking for advices from a DOM peer?

::: dom/media/MediaPermissionGonk.cpp
@@ +125,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aRequestingPrincipal);
>  
> +  nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>
> +      (nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID()));

Looks like you're replacing all the |mRequest->GetParentObject()| with |nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID())| in the entire file. Could we put this line of code back into |GetUserMediaRequest::GetParentObject()|?

Comment 18

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

(In reply to Shih-Chiang Chien [:schien] from comment #17)
> @jib, do we have a minimal reproduce steps for this bug?

This patch is really for Bug 949907 which has a testcase that works reliably for me, and I've verified that this patch fixes that testcase on OSX. If you could run that testcase on B2G that would be great. The plan is to land this patch from 949907 once reviewed here, per comment 8.

I have no proof that it fixes Ehsan's problem in Comment 0, but I hope so. Ehsan, if you could test the patch and see if it helps that would be great!

> I'm a bit worried about that B2G is not fixed by this patch because no logical change in
> MediaPermissionGonk.cpp.

There is a behavior change on B2G as well from this just like the other platforms, from:

::: dom/media/MediaManager.cpp
@@
> +    // Only run if window is still on our active list.
> +    if (!mManager->IsWindowStillActive(mWindowID)) {
> +      return NS_OK;
> +    }

The windowID in this case is from the innerWindow you already pass in. Since you always had the innerWindow (unlike the JS users) fewer changes were needed there.

> >  nsISupports* GetUserMediaRequest::GetParentObject()
> >  {
> > +  return nullptr;
> 
> I'm not sure if |GetUserMediaRequest| matches the scenario of returning
> nullptr here according to
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class.
> How about asking for advices from a DOM peer?

Good idea. I'll add a DOM reviewer.

> ::: dom/media/MediaPermissionGonk.cpp
> @@ +125,5 @@
> >  {
> >    NS_ENSURE_ARG_POINTER(aRequestingPrincipal);
> >  
> > +  nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>
> > +      (nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID()));
> 
> Looks like you're replacing all the |mRequest->GetParentObject()| with
> |nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID())| in the
> entire file. Could we put this line of code back into
> |GetUserMediaRequest::GetParentObject()|?

No, because that would retie the object to the DOM window, as I understand it. See "dead object" in comment 7.

Comment 19

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> Try - https://tbpl.mozilla.org/?tree=Try&rev=d32113cbce64

Looks good, except a lot of orange on B2G, is that normal? Nothing sticks out.

Comment 20

[(no longer active)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18)
> I have no proof that it fixes Ehsan's problem in Comment 0, but I hope so.
> Ehsan, if you could test the patch and see if it helps that would be great!

I would really like to, but I can't reproduce the bug most of the times.  In fact, I can't reproduce in almost every single attempt, which means that with this patch applied I will probably not see the bug but won't be able to say whether it's because the patch really fixes the bug or that the bug just doesn't happen very often...

Comment 21

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

No worries, if you see the problem still with the patch, let us know of course. I propose we still close this bug once this patch lands, then we can re-open it if the symptom resurfaces.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8370305 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (4)

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

r- because potential leakage is found.

::: dom/media/MediaManager.cpp
@@ +243,5 @@
>      nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);
>  
> +    // Only run if window is still on our active list.
> +    if (!mManager->IsWindowStillActive(mWindowID)) {
> +      return NS_OK;

If we stopped at here, there will be no "getUserMedia:response:allow" or "getUserMedia:response:deny" event and the pending GetUserMediaRunnable will never be removed from MediaManager::mActiveCallbacks.

Comment 23

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #22)
> ::: dom/media/MediaManager.cpp
> @@ +243,5 @@
> >      nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);
> >  
> > +    // Only run if window is still on our active list.
> > +    if (!mManager->IsWindowStillActive(mWindowID)) {
> > +      return NS_OK;
> 
> If we stopped at here, there will be no "getUserMedia:response:allow" or
> "getUserMedia:response:deny" event and the pending GetUserMediaRunnable will
> never be removed from MediaManager::mActiveCallbacks.

If the window is no longer active, it means it was navigated away from and its GetUserMediaRunnable is long gone already. See http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1526 - so there is no leak here AFAIK. It did not leak last I tested it. 

Another way to think of it: On desktop at least, few people ever hit deny, they just ignore the prompt which minimizes as soon as they click somewhere else, so the code does not rely on the allow/deny codepaths happening for cleanup.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8370305 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (4)

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

@jib, Thanks for the explanation. I run the testcase in bug 949907 with or without this patch and didn't see any error log or exception. No permission dialog is shown after applying this patch. (originally there will be a permission dialog for "https://bug949907.bugzilla.mozilla.org")

Comment 25

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #24)
> I run the testcase in bug 949907 with or
> without this patch and didn't see any error log or exception. No permission
> dialog is shown after applying this patch. (originally there will be a
> permission dialog for "https://bug949907.bugzilla.mozilla.org")

Excellent, that matches Ehsan's symptoms which we were unable to reproduce!

Comment 26

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

Just clarifying that I'm still waiting on two more reviews, rather than running with one.

- schien's was primarily for the Gonk code.
- khuey to confirm whether the nullptr in GetParentObject() approach is sound in comment 17.
- rjesup for rest of code.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't see where GetUserMediaRequest is handed out from, but I don't think using nullptr is ok.

Comment 28

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

It is handed out here http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1440

Essentially it's just a message object from chrome to chrome-only observers. The problem with tying it to a window is the window may go away (my third point in comment 7). Someone on #content suggested I use nullptr, I don't remember who now. If there's a trivial pattern I can follow here, please let me know.

Comment 29

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #27)
> I don't see where GetUserMediaRequest is handed out from, but I don't think using nullptr is ok.

Turns out I got the suggestion from you on irc :-)

Dec-18-12:

19:39	jib	So I'm trying to track down bug 949907 and I'm still learning here. Does a webidl object like http://mxr.mozilla.org/mozilla-ce...edia/GetUserMediaRequest.cpp#44 have to have a parent? And if that parent is a window, does this mean the object dies with the window?
19:39	firebot	Bug https://bugzilla.mozilla.org/show_bug.cgi?id=949907 nor, --, ---, nobody, NEW, webrtcUI.jsm (handleRequest): can't access dead object
19:40	khuey	jib: wrapper cached objects have a canonical wrapper
19:41	khuey	jib: the parent object bit tells the bindings code what compartment to create that wrapper in
19:41	khuey	jib: and chrome references to everything in that compartment are cut when the window unloads
19:43	jib	khuey: GetUserMediaRequest is a request object passed from MediaManager (a global) to an observer in webrtcUI.jsm, another global, and it contains a windowID. All of those things I just mentioned can survive a content window, so it is perhaps wrong to tie the request object to the content window. Is there a nother parent I can use here?
19:43	jib	khuey: thanks for explaining, what you say seems to explain the dead object
19:45	khuey	jib: is GetUserMediaRequest visible to content?
19:45	jib	khuey: no
19:45	khuey	or are those objects only manipulated internally by chrome code?
19:45	jib	only internal
19:45	khuey	yeah, then parenting it to the window is wrong
19:46	khuey	you should just use nullptr, and then it'll get parented to whatever accesses it first
19:46	khuey	which if everything touching it is chrome doesn't matter
19:46	jib	khuey: ok cool, thanks!
19:46	khuey	jib: np

Change your mind?

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #29)
> Change your mind?

Nope, if it's ChromeOnly this is fine.  But I couldn't figure that out :P

Comment 31

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

Thanks :-) Down to jesup.

Comment 32

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

Attachment: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (5) r=schien,khuey

Unbitrot. Carrying forward r+ from khuey and schien for DOM and Gonk parts.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8370305 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (4)

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

::: dom/webidl/Navigator.webidl
@@ +342,5 @@
>                                MozGetUserMediaDevicesSuccessCallback onsuccess,
> +                              NavigatorUserMediaErrorCallback onerror,
> +                              // pass in originating innerWindowID and callbacks
> +                              // wont be called if window navigates away.
> +                              optional unsigned long long innerWindowID = 0);

The comment is a bit confusing

Comment 34

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

Attachment: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (6) r=schien,khuey,jesup

Confusing code comment rewritten.

Carrying forward r+ from khuey, schien and jesup.

Will upload this patch to Bug 949907 next and land from there.

Comment 35

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

Bug 949907 is fixed (not duping to avoid linking to it from here).

We should probably consider Aurora uplift next.

Comment 36

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

Should we uplift Bug 949907 to Aurora or will that just draw attention to it?

Comment 37

[Daniel Veditz [:dveditz]]

This won't cause any security problems landing on Aurora.

Comment 38

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

Comment on attachment 8375832 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (6) r=schien,khuey,jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): gUM permission prompt
User impact if declined: 
Leaves a race condition which an attacker may be able to employ as one step to convince the user that a legit website is trying to access their webcam/mic.

Testing completed (on m-c, etc.): 
Fix verified by schien (b2g) and myself (desktop) independently. Landed on m-c a week ago.

Risk to taking this patch (and alternatives if risky): 
Alters permission prompt code on desktop, android and b2g, which except for android, doesn't have much automated coverage on builders (Android thankfully has a robocop2 test, which I tripped with an earlier buggy patch), but everything appears to be working, so I'm going to go with low risk.

String or IDL/UUID changes made by this patch: none

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Did this affect Beta?

Comment 40

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

This weakness has been there since the introduction of the gUM permission prompt AFAIK.

Comment 41

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

We should uplift from public Bug 949907 since we landed from there. Al, can you give me an a+ there?

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Done.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/53b26d3251dd

Is this wontfix for beta/esr24?

Comment 44

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

Comment on attachment 8375832 [details] [diff] [review]
GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (6) r=schien,khuey,jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
This appeared with the introduction of the gUM permission prompt.

User impact if declined: 
Leaves a race condition which an attacker may be able to employ as one step to convince the user that a legit website is trying to access their webcam/mic.

No actual camera exploit was found here (comment 3), though a subsequent prompt may be less scrutinized by a user if this came first followed by a socially engineered "oops, try again".

A misleading UI prompt like this may undermine trust in gUM/webrtc in firefox.

We may have succeeded at disguising the fix.

Testing completed (on m-c, etc.): 
Fix verified by schien (b2g) and myself (desktop) independently. Landed on m-c a week ago, aurora an hour ago.

Risk to taking this patch (and alternatives if risky): 
Alters permission prompt code on desktop, android and b2g, which except for android, doesn't have much automated coverage on builders (Android thankfully has a robocop2 test, which I tripped with an earlier buggy patch), but everything appears to be working, so I'm going to go with low risk.

String or IDL/UUID changes made by this patch: none

Comment 45

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

Need to do the thing again.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Crap, I literally mid-aired with you. Let me know if you need a backout.
https://hg.mozilla.org/releases/mozilla-beta/rev/f988a03ddc6d

Comment 47

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

Looks good!

Comment 48

[Al Billings [:abillings - ex-MoCo]]

I'm not sure why I am needinfo? here.

Comment 49

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

Bug 949907 still needs a mock a+ for beta, but it landed already.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

done.

Comment 51

[u279076]

Nils, can you please evaluate whether this needs QA testing before we release Firefox 28? If not, please set [qa-] in the whiteboard. If so, please make sure this gets verified fixed.

Comment 52

[Nils Ohlmeier [:drno]]

I think because of its security status it should get tested.
However as it is a race condition it is actually not that easy to test. It took me a lot of attempts to reproduce the problem on 27 manually.
I'll investigate if the new automated doorhanger tests can help us to make this reliable reproducible.

Comment 53

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

Just manually testing that we didn't break anything might be valuable since I don't know that our automated tests cover the permission prompt codepath well or at all.

Comment 54

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #53)
> Just manually testing that we didn't break anything might be valuable since
> I don't know that our automated tests cover the permission prompt codepath
> well or at all.

FYI since a few days we have automated testing of the door hanger in browser/base/content/test/general/get_user_media.html
But I fear it is not easily useable for testing this bug, as it serves everything from 127.0.0.1, but we would need a different domain for a proper test (not sure if 127.0.0.1 with a different number would be sufficient/work?).

I manually triggered the bug in FF 27 and an outdated FF 29 build by going to http://people.mozilla.org/~nohlmeier/rtc.html, entering a fake meeting number, hit the AppSpot button and immediately navigate back to the peoples page.
With the same manual test I verified in ca. 20 test runs that I'm no longer able to trigger the door hanger on the peoples page in FF 28 and FF 29.