The default bug view has changed. See this FAQ.
Bug 876044 (CVE-2013-1698)

Domain displayed in cam/mic permission dialog matches URL location bar, not content

VERIFIED FIXED in Firefox 22

Status

()

Firefox
Device Permissions
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: mwobensmith, Assigned: dao)

Tracking

({csectype-spoof, relnote, sec-moderate})

Trunk
Firefox 24
csectype-spoof, relnote, sec-moderate
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 wontfix, firefox22+ verified, firefox23+ verified, firefox24+ verified, firefox-esr17 unaffected, b2g18 unaffected, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected)

Details

(Whiteboard: [getUserMedia][blocking-gum-][adv-main22+])

Attachments

(5 attachments, 3 obsolete attachments)

Created attachment 754016 [details]
Cross-domain iframe

When getUserMedia is called from a cross-domain iframe, the domain displayed in the permission dialog matches the topmost page (displayed in address bar) and not the domain from which the iframe was sourced.

Click on link in first iframe - same domain - permission dialog will display correct domain.

Click on link in second iframe - cross-domain, from people.mozilla.org - permission dialog will display incorrect domain (above).
Created attachment 754017 [details]
Dependant file for test case
Created attachment 754018 [details]
Attempt #2
Attachment #754016 - Attachment is obsolete: true

Updated

4 years ago
Component: WebRTC → General
Product: Core → Firefox
QA Contact: jsmith
The test case included here doesn't work.
Test case isn't working likely due to mixed content blocking. Bugzilla is HTTPS, and people.mozilla.org is HTTP.

I'll try to create another example that is Bugzilla-friendly.
Tentatively assigning to Dao
Assignee: nobody → dao
Whiteboard: [getUserMedia]
Confirmed on Nightly 5/24.
This was working. Don't remember when. But it's on release too.

Going with sec-high on this one. If the sec team disagrees, feel free to change.
status-b2g18: --- → unaffected
status-b2g18-v1.0.0: --- → unaffected
status-b2g18-v1.0.1: --- → unaffected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox-esr17: --- → unaffected
Keywords: regression, sec-high

Updated

4 years ago
status-firefox21: affected → wontfix
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Whiteboard: [getUserMedia] → [getUserMedia][blocking-webrtc+]

Updated

4 years ago
Whiteboard: [getUserMedia][blocking-webrtc+] → [getUserMedia][blocking-gum+]
We should be able to take a low-risk speculative fix in the next week, otherwise it may be too late for FF22.
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 9

4 years ago
(In reply to Jason Smith [:jsmith] from comment #7)
> This was working. Don't remember when. But it's on release too.

Did you try to find a regression range? I actually don't think it was ever working.
tracking-firefox22: + → ?
tracking-firefox23: + → ?
tracking-firefox24: + → ?
(Assignee)

Comment 10

4 years ago
Oops, I didn't mean to reset the tracking flags. Stupid bug when reloading bugzilla pages...
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 11

4 years ago
Created attachment 754734 [details] [diff] [review]
patch

This displays the correct host for requests from framed pages and *removes* the host from the page-specific indicator ("You are currently sharing your camera with ..."), because this can subsume streams for multiple different hosts. The button in the navigation toolbar still lists all locations for active streams.
Attachment #754734 - Flags: review?(dolske)
Attachment #754734 - Flags: feedback?(rjesup)
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Jason Smith [:jsmith] from comment #7)
> > This was working. Don't remember when. But it's on release too.
> 
> Did you try to find a regression range? I actually don't think it was ever
> working.

My notes I took when I tested this feature originally indicates I looked into this on October 11 and 12 2012 (a few days after this feature landed), although I ran into problems testing the iframe test cases I had due to a different platform bug happening (two audio requests in a row were failing). I think you are right though based on running hg blame on the webrtcUI file against the changes in this diff - looks like this was present since the feature landed. I probably need to be a bit more careful in the future doing verifications then.
Keywords: regression

Updated

4 years ago
Flags: needinfo?
Comment on attachment 754734 [details] [diff] [review]
patch

> This displays the correct host for requests from framed pages and *removes*
> the host from the page-specific indicator ("You are currently sharing your
> camera with ..."), because this can subsume streams for multiple different
> hosts. The button in the navigation toolbar still lists all locations for
> active streams.

The patch changes this to say "... with this page."

"with this page" implies it's the page listed in the address bar (which this indicator is part of).  An alternative: remove "with the page", leaving "You are currently sharing your camera and microphone."

However, it's arguable, and that's an issue for the UX and privacy people, who I think should at least look at this change.  From my technical point of view, this change seems ok.  And I'll note that the window to get into 22 is small, so any change would need to be approved soon (probably within 48 hours)

Also adding ekr for feedback on the security model.
Attachment #754734 - Flags: feedback?(rjesup)
Attachment #754734 - Flags: feedback?(ekr)
Attachment #754734 - Flags: feedback+
Flags: needinfo?

Updated

4 years ago
Flags: needinfo?(mmc)
Note - I don't think we normally take patches on uplift that switch around localizable strings (correct me if I'm wrong though). We might need to figure out a solution that doesn't cause l10n risk for aurora and beta.
I haven't looked at the code, but I certainly agree that the UX would be better if it
referred to the origin where the JS was running.
(In reply to Eric Rescorla (:ekr) from comment #15)
> I haven't looked at the code, but I certainly agree that the UX would be
> better if it
> referred to the origin where the JS was running.

Right. Typical security UX like this usually wants to show the user where the indicator's origin is deriving from.
I agree with jsmith and ekr that the interface should show the origin, not the enclosing page. The originating URL still can collude with the enclosing page to pass on media content, but it seems closer to the truth to show the originating URL in the UI.

I didn't fully grok the difference between dao and jesup's patches, but having "You are currently sharing your camera and mic" without specifying the host also seems fine for including with the page info, since we can't fully enumerate the list of hosts who can see the media, and it may not add much value to the user to try.

Thanks,
Monica
Flags: needinfo?(mmc)
dao - can you confirm that this is sec-high and critical to be uplifted? This has string changes, so typically it's outside of our normal landing criteria.
Flags: needinfo?(dao)
(In reply to Alex Keybl [:akeybl] from comment #18)
> dao - can you confirm that this is sec-high and critical to be uplifted?
> This has string changes, so typically it's outside of our normal landing
> criteria.

FWIW, I think there might be possibility to avoid the string changes here. Let's get confirmation from dao on that.
Btw - I went with sec-high originally because this allows a malicious website to trick the user into thinking the iframe has the same safety as the top-level origin, which could make the user think it's safe to give access to the iframe for camera/mic usage, when in fact, it's not. The implications of a malicious site getting the camera for instance means that they could get a picture of the user from their camera.
Talked with Daniel Veditz in IRC - he would have originally went with sec-moderate on this, but could buy into sec-high here.
I'm going to go with sec-moderate rather than sec-high--although it's domain spoofing granting camera access is not quite as bad as bugs where one site can steal your bank account or other personal data.
Keywords: sec-high → csec-spoof, sec-moderate
(In reply to Matt Wobensmith from comment #4)
> Test case isn't working likely due to mixed content blocking. Bugzilla is
> HTTPS, and people.mozilla.org is HTTP.

You should be able to change the testcase to https://people.mozilla.com and have it work if that's the problem.
That probably then drops this to a non-blocker if it's sec-moderate, but we still need to get this fixed.
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum-]
I'm with Randell (comment 13) and ekr (comment 15) in being unhappy with the text saying you're sharing "with this page". It's unclear and misleading to the average user who might not be aware that it's a hidden injected iframe from "revengeporn.com" running the camera.[1]

If the average user interprets "with this page" as meaning the URL in the address bar this patch is not really solving the security problem raised by this bug. A slight improvement in that it's technically no longer lying, but I'm not sure that's worth rushing in Firefox 22.

[1] Of course it could be an injected script instead and then the misleading top-level URI is all we could say.
If we do ship without a fix for this in Firefox 22 we should be up front about it and put it in the relnotes as a known issue. It's reasonably likely that this is the kind of thing security researchers will try right off the bat and then complain about.
Keywords: relnote

Updated

4 years ago
relnote-firefox: --- → ?
(In reply to Daniel Veditz [:dveditz] from comment #26)
> If we do ship without a fix for this in Firefox 22 we should be up front
> about it and put it in the relnotes as a known issue. It's reasonably likely
> that this is the kind of thing security researchers will try right off the
> bat and then complain about.

We probably should relnote this anyways as this does exist in FF 20 and FF 21.
Created attachment 755665 [details]
Fix the HTTPS issue for people.mozilla.com

Updated test.

When I tried this initially, I was getting SSL errors from people.mozilla.com... now, that is no longer happening.
Attachment #754018 - Attachment is obsolete: true
OK, forget the test case. Still getting an intermittent SSL error on people.mozilla.com. I think we know how to recreate and verify this bug going forward.

Dan, responding to comment 22, I have to respectfully disagree. I feel that theft of camera/mic is every bit as bad as financial data, and possibly worse.
(In reply to Matt Wobensmith from comment #29)
> Dan, responding to comment 22, I have to respectfully disagree. I feel that
> theft of camera/mic is every bit as bad as financial data, and possibly
> worse.

Why?
people.mo seems to be misconfigured -- filed bug 877486. Until then your testcase should work by switching people.mozilla.org to people.mozilla.com.
Created attachment 755705 [details]
Fixed (I hope) testcase

s/people.org/people.com/
Attachment #755665 - Attachment is obsolete: true
(In reply to Daniel Veditz [:dveditz] from comment #25)

> If the average user interprets "with this page" as meaning the URL in the
> address bar this patch is not really solving the security problem raised by
> this bug. A slight improvement in that it's technically no longer lying, but
> I'm not sure that's worth rushing in Firefox 22.

I think an important detail has been missed, here.

The "with this page" string in this patch is _only_ used on the "reminder" notification shown _after_ the user has granted permission for a specific WebRTC request. The initial permission prompt will continue to show a host name.

So with the patch here, clicking in the cross-origin testcase iframe results in:

1) A prompt for "Would you like you share your microphone with people.mozilla.com". [Without patch we incorrectly show the hostname of the top-level page.]
2) Click allow
3) A green icon in the URL bar, upon clicking it shows a panel with "You are currently sharing your microphone with this page." [Without patch we only list the hostname of the... first? last? (not sure) request that was allowed.]

I think that's adequate; the purpose of the icon/message is as a reminder that a users camera/mic is active for something in the page. The details of exactly which subresource is using it don't really matter at that point.
Comment on attachment 754734 [details] [diff] [review]
patch

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

::: browser/modules/webrtcUI.jsm
@@ +122,5 @@
>      return;
>    }
>  
> +  let contentWindow = Services.wm.getOuterWindowWithId(aWindowID);
> +  let host = contentWindow.document.documentURIObject.asciiHost;

Existing bug:

If you load the testcase here from a file, we prompt with "Would you like to share your microphone with ?". file:/// is so annoying. :( New bug, if it's not filed?
Attachment #754734 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #34)
> Comment on attachment 754734 [details] [diff] [review]
> patch
> 
> Review of attachment 754734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/webrtcUI.jsm
> @@ +122,5 @@
> >      return;
> >    }
> >  
> > +  let contentWindow = Services.wm.getOuterWindowWithId(aWindowID);
> > +  let host = contentWindow.document.documentURIObject.asciiHost;
> 
> Existing bug:
> 
> If you load the testcase here from a file, we prompt with "Would you like to
> share your microphone with ?". file:/// is so annoying. :( New bug, if it's
> not filed?

That's bug 801066.
Comment on attachment 754734 [details] [diff] [review]
patch

>diff --git a/browser/modules/webrtcUI.jsm b/browser/modules/webrtcUI.jsm

> function handleRequest(aSubject, aTopic, aData) {

>-  browser.ownerDocument.defaultView.navigator.mozGetUserMediaDevices(
>+  Services.wm.getMostRecentWindow(null).navigator.mozGetUserMediaDevices(

Why this change? Just to avoid having to get "browser" here, I suppose? Seems more robust to use the window associated with the site actually making the request, but I suppose it doesn't matter so much.
(Assignee)

Comment 37

4 years ago
(In reply to Jason Smith [:jsmith] from comment #19)
> (In reply to Alex Keybl [:akeybl] from comment #18)
> > dao - can you confirm that this is sec-high and critical to be uplifted?
> > This has string changes, so typically it's outside of our normal landing
> > criteria.
> 
> FWIW, I think there might be possibility to avoid the string changes here.
> Let's get confirmation from dao on that.

I can add a minimal patch without string changes for uplifting. That patch would fix the permission dialog and leave the "You are currently sharing your camera with ..." message alone.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #36)
> Comment on attachment 754734 [details] [diff] [review]
> patch
> 
> >diff --git a/browser/modules/webrtcUI.jsm b/browser/modules/webrtcUI.jsm
> 
> > function handleRequest(aSubject, aTopic, aData) {
> 
> >-  browser.ownerDocument.defaultView.navigator.mozGetUserMediaDevices(
> >+  Services.wm.getMostRecentWindow(null).navigator.mozGetUserMediaDevices(
> 
> Why this change? Just to avoid having to get "browser" here, I suppose?

Yep.

> Seems more robust to use the window associated with the site actually making
> the request, but I suppose it doesn't matter so much.

It really shouldn't matter.
Flags: needinfo?(dao)
(Assignee)

Comment 38

4 years ago
Created attachment 755827 [details] [diff] [review]
patch without string changes for uplifting

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: domain spoofing when requesting camera/microphone access
Testing completed (on m-c, etc.): n/a
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none
Attachment #755827 - Flags: approval-mozilla-beta?
Attachment #755827 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 39

4 years ago
Comment on attachment 754734 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

pretty easy

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

nope

Which older supported branches are affected by this flaw?

21 (release), soon 22 and 23

If not all supported branches, which bug introduced the flaw?

n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

patch for uplifting to aurora and beta is attached

How likely is this patch to cause regressions; how much testing does it need?

unlikely and it's easy to test manually e.g. with the attached testcase
Attachment #754734 - Flags: feedback?(ekr) → sec-approval?

Updated

4 years ago
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][webrtc-uplift]
Comment on attachment 754734 [details] [diff] [review]
patch

sec-approval+ for M-C.
Attachment #754734 - Flags: sec-approval? → sec-approval+
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 41

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d76dbfda3e55
https://hg.mozilla.org/mozilla-central/rev/d76dbfda3e55
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

4 years ago
Keywords: verifyme
QA Contact: jsmith
Comment on attachment 755827 [details] [diff] [review]
patch without string changes for uplifting

Patch is WebRTC specific which limits risk here. That along with the low risk evaluation for WebRTC and the attached test case makes this ok for both Aurora & Beta.
Attachment #755827 - Flags: approval-mozilla-beta?
Attachment #755827 - Flags: approval-mozilla-beta+
Attachment #755827 - Flags: approval-mozilla-aurora?
Attachment #755827 - Flags: approval-mozilla-aurora+
I tried merging this to beta (patch doesn't apply quite cleanly), but the result of the merge is I don't get any permission requesters at all.  I'm going to try aurora now.
The patch appears to work correctly on Aurora, so landing there.  Beta needs to be figured out ASAP for beta 4...
https://hg.mozilla.org/releases/mozilla-aurora/rev/2229cf072c0d
status-firefox23: affected → fixed
Services.wm.getOuterWindowWithId (bug 861495) doesn't exist on beta, so the patch needs to avoid using it (both in the modified code as well as the newly added code), as the pre-http://hg.mozilla.org/mozilla-central/rev/9db0e05ead33 code did.

Updated

4 years ago
Keywords: branch-patch-needed
lgtm with dolske's attached test case on trunk
Status: RESOLVED → VERIFIED

Updated

4 years ago
status-firefox24: fixed → verified
Created attachment 757251 [details] [diff] [review]
Change how getusermedia dropdowns are generated

patch as merged to Beta (tested and works correctly)
https://hg.mozilla.org/releases/mozilla-beta/rev/4d420f0b48bb
status-firefox22: affected → fixed
(Assignee)

Updated

4 years ago
Keywords: branch-patch-needed
Both Aurora & Beta patches contain a similar bad origin bug on uplift. We need a followup here.

STR

1. Run dolske's test case
2. Request and accept permissions for the iframe
3. Select the visible indicator

Expected

The origin should be the iframe's origin.

Actual

The origin is the top-level origin.
relnote-firefox: ? → ---

Updated

4 years ago
Depends on: 878924
Verified with one followup - bug 878924.
status-firefox23: fixed → verified
(Assignee)

Updated

4 years ago
Blocks: 878924
No longer depends on: 878924

Updated

4 years ago
Whiteboard: [getUserMedia][blocking-gum-][webrtc-uplift] → [getUserMedia][blocking-gum-]
Also confirmed fixed on beta using dolske's test case.
status-firefox22: fixed → verified

Updated

4 years ago
Keywords: verifyme
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][adv-main22+]
Alias: CVE-2013-1698
Group: core-security
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.