Closed
Bug 961512
(CVE-2014-1499)
Opened 11 years ago
Closed 11 years ago
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
Categories
(Core :: WebRTC, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: ehsan.akhgari, Assigned: jib)
Details
(Keywords: csectype-spoof, sec-moderate, Whiteboard: [fixed by bug 949907][adv-main28+])
Attachments
(2 files, 6 obsolete files)
97.04 KB,
image/png
|
Details | |
21.69 KB,
patch
|
jib
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
We have a bunch of folks who like to earn bounties off this kind of navigation/timing location confusion :-(
Keywords: csectype-spoof,
sec-moderate
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee: nobody → jib
Reporter | ||
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
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!
Attachment #8363939 -
Attachment is obsolete: true
Attachment #8364188 -
Flags: feedback?(schien)
Attachment #8364188 -
Flags: feedback?(rjesup)
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #8364188 -
Flags: feedback?(schien) → feedback+
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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
Assignee | ||
Comment 14•11 years ago
|
||
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.
Attachment #8364188 -
Attachment is obsolete: true
Attachment #8364188 -
Flags: feedback?(rjesup)
Attachment #8370234 -
Flags: feedback?(schien)
Assignee | ||
Comment 15•11 years ago
|
||
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.
Attachment #8370234 -
Attachment is obsolete: true
Attachment #8370234 -
Flags: feedback?(schien)
Attachment #8370305 -
Flags: review?(schien)
Attachment #8370305 -
Flags: review?(rjesup)
Assignee | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
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()|?
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Flags: needinfo?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #8370305 -
Flags: review?(khuey)
Assignee | ||
Comment 19•11 years ago
|
||
(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.
Reporter | ||
Comment 20•11 years ago
|
||
(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...
Flags: needinfo?(ehsan)
Assignee | ||
Comment 21•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #8370305 -
Flags: review?(schien) → review-
Assignee | ||
Comment 23•11 years ago
|
||
(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•11 years ago
|
||
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")
Attachment #8370305 -
Flags: review- → review+
Assignee | ||
Comment 25•11 years ago
|
||
(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!
Assignee | ||
Comment 26•11 years ago
|
||
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.
I don't see where GetUserMediaRequest is handed out from, but I don't think using nullptr is ok.
Assignee | ||
Comment 28•11 years ago
|
||
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.
Assignee | ||
Comment 29•11 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(khuey)
(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
Flags: needinfo?(khuey)
Attachment #8370305 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 31•11 years ago
|
||
Thanks :-) Down to jesup.
Assignee | ||
Comment 32•11 years ago
|
||
Unbitrot. Carrying forward r+ from khuey and schien for DOM and Gonk parts.
Attachment #8370305 -
Attachment is obsolete: true
Attachment #8370305 -
Flags: review?(rjesup)
Attachment #8375621 -
Flags: review?(rjesup)
Assignee | ||
Updated•11 years ago
|
Attachment #8375621 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8375621 -
Attachment description: GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (5) → GetUserMediaDevices cleanup + untie GetUserMediaRequest obj from DOM (5) r=schien,khuey
Comment 33•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #8375621 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 34•11 years ago
|
||
Confusing code comment rewritten.
Carrying forward r+ from khuey, schien and jesup.
Will upload this patch to Bug 949907 next and land from there.
Attachment #8375621 -
Attachment is obsolete: true
Attachment #8375832 -
Flags: review+
Assignee | ||
Comment 35•11 years ago
|
||
Bug 949907 is fixed (not duping to avoid linking to it from here).
We should probably consider Aurora uplift next.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [fixed by bug 949907]
Target Milestone: --- → mozilla30
Assignee | ||
Comment 36•11 years ago
|
||
Should we uplift Bug 949907 to Aurora or will that just draw attention to it?
Flags: needinfo?(dveditz)
Comment 37•11 years ago
|
||
This won't cause any security problems landing on Aurora.
Flags: needinfo?(dveditz)
Assignee | ||
Comment 38•11 years ago
|
||
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
Attachment #8375832 -
Flags: approval-mozilla-aurora?
Comment 39•11 years ago
|
||
Did this affect Beta?
status-firefox28:
--- → ?
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
status-firefox-esr24:
--- → ?
Assignee | ||
Comment 40•11 years ago
|
||
This weakness has been there since the introduction of the gUM permission prompt AFAIK.
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8375832 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
tracking-firefox29:
--- → +
Assignee | ||
Comment 41•11 years ago
|
||
We should uplift from public Bug 949907 since we landed from there. Al, can you give me an a+ there?
Flags: needinfo?(abillings)
Comment 43•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/53b26d3251dd
Is this wontfix for beta/esr24?
Assignee | ||
Comment 44•11 years ago
|
||
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
Attachment #8375832 -
Flags: approval-mozilla-beta?
Updated•11 years ago
|
Attachment #8375832 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 46•11 years ago
|
||
Crap, I literally mid-aired with you. Let me know if you need a backout.
https://hg.mozilla.org/releases/mozilla-beta/rev/f988a03ddc6d
Assignee | ||
Comment 49•11 years ago
|
||
Bug 949907 still needs a mock a+ for beta, but it landed already.
Comment 50•11 years ago
|
||
done.
Comment 51•11 years ago
|
||
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.
Flags: needinfo?(drno)
Comment 52•11 years ago
|
||
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.
Assignee | ||
Comment 53•11 years ago
|
||
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•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [fixed by bug 949907] → [fixed by bug 949907][adv-main28+]
Updated•11 years ago
|
Alias: CVE-2014-1499
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•