Closed Bug 876044 (CVE-2013-1698) Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Device Permissions, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox21 --- wontfix
firefox22 + verified
firefox23 + verified
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected

People

(Reporter: mwobensmith, Assigned: dao)

References

Details

(Keywords: csectype-spoof, relnote, sec-moderate, Whiteboard: [getUserMedia][blocking-gum-][adv-main22+])

Attachments

(5 files, 3 obsolete files)

Attached file Cross-domain iframe (obsolete) —
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).
Attached file Attempt #2 (obsolete) —
Attachment #754016 - Attachment is obsolete: true
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.
Whiteboard: [getUserMedia] → [getUserMedia][blocking-webrtc+]
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.
(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.
Oops, I didn't mean to reset the tracking flags. Stupid bug when reloading bugzilla pages...
OS: Mac OS X → All
Hardware: x86 → All
Attached patch patchSplinter Review
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
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?
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.
(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
(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.
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.
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.
(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)
[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?
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?
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+
https://hg.mozilla.org/mozilla-central/rev/d76dbfda3e55
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
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...
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.
lgtm with dolske's attached test case on trunk
Status: RESOLVED → VERIFIED
patch as merged to Beta (tested and works correctly)
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: ? → ---
Depends on: 878924
Verified with one followup - bug 878924.
Blocks: 878924
No longer depends on: 878924
Whiteboard: [getUserMedia][blocking-gum-][webrtc-uplift] → [getUserMedia][blocking-gum-]
Also confirmed fixed on beta using dolske's test case.
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.