876044 - (CVE-2013-1698) Domain displayed in cam/mic permission dialog matches URL location bar, not content

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: 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).

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Dependant file for test case

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Attempt #2

Comment 3

[Jason Smith [:jsmith]]

The test case included here doesn't work.

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 5

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

Tentatively assigning to Dao

Comment 6

[Jason Smith [:jsmith]]

Confirmed on Nightly 5/24.

Comment 7

[Jason Smith [:jsmith]]

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.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

We should be able to take a low-risk speculative fix in the next week, otherwise it may be too late for FF22.

Comment 9

[Dão Gottwald [:dao]]

(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.

Comment 10

[Dão Gottwald [:dao]]

Oops, I didn't mean to reset the tracking flags. Stupid bug when reloading bugzilla pages...

Comment 11

[Dão Gottwald [:dao]]

Attachment: 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.

Comment 12

[Jason Smith [:jsmith]]

(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.

Comment 13

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

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.

Comment 14

[Jason Smith [:jsmith]]

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.

Comment 15

[Eric Rescorla (:ekr)]

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.

Comment 16

[Jason Smith [:jsmith]]

(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.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Jason Smith [:jsmith]]

(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.

Comment 20

[Jason Smith [:jsmith]]

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.

Comment 21

[Jason Smith [:jsmith]]

Talked with Daniel Veditz in IRC - he would have originally went with sec-moderate on this, but could buy into sec-high here.

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Daniel Veditz [:dveditz]]

(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.

Comment 24

[Jason Smith [:jsmith]]

That probably then drops this to a non-blocker if it's sec-moderate, but we still need to get this fixed.

Comment 25

[Daniel Veditz [:dveditz]]

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.

Comment 26

[Daniel Veditz [:dveditz]]

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.

Comment 27

[Jason Smith [:jsmith]]

(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.

Comment 28

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: 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.

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 30

[Jason Smith [:jsmith]]

(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?

Comment 31

[Justin Dolske [:Dolske]]

people.mo seems to be misconfigured -- filed bug 877486. Until then your testcase should work by switching people.mozilla.org to people.mozilla.com.

Comment 32

[Justin Dolske [:Dolske]]

Attachment: Fixed (I hope) testcase

s/people.org/people.com/

Comment 33

[Justin Dolske [:Dolske]]

(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 34

[Justin Dolske [:Dolske]]

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?

Comment 35

[Jason Smith [:jsmith]]

(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 36

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

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.

Comment 37

[Dão Gottwald [:dao]]

(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 [e-mail] 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.

Comment 38

[Dão Gottwald [:dao]]

Attachment: 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

Comment 39

[Dão Gottwald [:dao]]

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

Comment 40

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

Comment on attachment 754734 [details] [diff] [review]
patch

sec-approval+ for M-C.

Comment 41

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d76dbfda3e55

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d76dbfda3e55

Comment 43

[Alex Keybl [:akeybl]]

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.

Comment 44

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

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.

Comment 45

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

The patch appears to work correctly on Aurora, so landing there.  Beta needs to be figured out ASAP for beta 4...

Comment 46

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2229cf072c0d

Comment 47

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

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.

Comment 48

[Jason Smith [:jsmith]]

lgtm with dolske's attached test case on trunk

Comment 49

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

Attachment: Change how getusermedia dropdowns are generated

patch as merged to Beta (tested and works correctly)

Comment 50

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4d420f0b48bb

Comment 51

[Jason Smith [:jsmith]]

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.

Comment 52

[Jason Smith [:jsmith]]

Verified with one followup - bug 878924.

Comment 53

[Jason Smith [:jsmith]]

Also confirmed fixed on beta using dolske's test case.