1062876 - (CVE-2014-1585) The "stop sharing" option in the video sharing control in the URL bar has no effect in iframes

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Eric Shepherd [:sheppy]]

STR:

1. Go to http://jsfiddle.net/codepo8/agaRe/4/
2. Grant permission to share your camera
3. Once streaming begins, click the orange video sharing icon in the URL bar
4. Choose the "Stop Sharing" option from the popup in the sharing control box that appears

Notice that streaming continues.

There is no console output indicating an error state.

jesup suggests in IRC this may be because the video is in an iframe.

Comment 1

[Eric Shepherd [:sheppy]]

FWIW, no output in browser console either.

Comment 2

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

Goes back to at least beta 32 (likely further)
Reloading tab fixes it.
On 32 (no orange notifier at top) the indicator in the URL bar goes away, but the sharing dropdown still shows the page as sharing.
Affects privacy, marking s-s
+flo-retina

Comment 3

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

MediaManager:5 logs (inbound):
-1577191616[7fa8a1d824a0]: Returning success for getUserMedia()
-1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 27 capturing video    
-1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 20 capturing video    
-1577191616[7fa8a1d824a0]: Revoking MediaCapture access for window 20

Comment 4

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

"Normal" sequence on gum_test.html:

-1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 40 capturing video    
-1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 40 capturing video    
-1577191616[7fa8a1d824a0]: Revoking MediaCapture access for window 40

Comment 5

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

Simpler test:
http://mozilla.github.com/webrtc-landing/gum_iframe.html

Confirmed, it's an iframe issue

Comment 6

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] from comment #3)
> MediaManager:5 logs (inbound):
> -1577191616[7fa8a1d824a0]: Returning success for getUserMedia()
> -1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 27 capturing
> video    
> -1577191616[7fa8a1d824a0]: MediaCaptureWindowState: window 20 capturing
> video    
> -1577191616[7fa8a1d824a0]: Revoking MediaCapture access for window 20

Is the front-end code sending the wrong window ID in the revoke notification, or is the media manager code not recursing correctly into sub-windows when access is revoked for a window? (My guess is the latter, but I just wanted to check we have the same understanding.)

Comment 7

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

Stop screen/window sharing seems to work.  I'm 90% certain the issue is that OnNavigation doesn't iterate the child window list the way the "what are we sharing with" function uses

Comment 8

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

Attachment: refactor window iteration code for MediaManager

tested and works with all 3 uses - current state; stop screensharing only, stop all sharing.  One undealt-with item I noticed while testing it (which may be a bug elswhere): if I share in an iframe, then hit back I *don't* got OnNavigation (and so don't end the share).  If I share in a normal page (gum_test.html) I do get OnNavigation

Comment 9

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

Caused/exposed by bug 885796 which landed in 29

Since the b2g ui has no "stop sharing", this doesn't apply to b2g (except maybe for the likely separate bug of "no OnNavigation() sent on Back")

Comment 10

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

Is there a separate bug on the back button?

When I test, it's not just the back button: Normal forward navigation away from gum_iframe.html seems to leave the camera running as well! E.g:

1. I go to http://mozilla.github.com/webrtc-landing/gum_iframe.html and enable video.
2. I now go to cnn.com and the camera light is still on.
3. I hit the back button, and there I am waving back at me, still being recorded it seems.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8484235 [details] [diff] [review]
refactor window iteration code for MediaManager

I can confirm that this patch fixes the 'Stop sharing' button for iframes, but I also see bug 1062981.

Comment 12

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

Comment on attachment 8484235 [details] [diff] [review]
refactor window iteration code for MediaManager

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

This is an urgent patch and a refactor in one, so forgive me but I'd like to see it again as I think the refactor needs work.

::: dom/media/MediaManager.cpp
@@ +1832,5 @@
> +{
> +  if (aListeners) {
> +    auto length = aListeners->Length();
> +    for (size_t i = 0; i < length; ++i) {
> +      GetUserMediaCallbackMediaStreamListener *listener = aListeners->ElementAt(i);

Nit: using auto* would avoid unwrapping the nsRefPtr here

@@ +1850,5 @@
>  void
>  MediaManager::OnNavigation(uint64_t aWindowID)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "OnNavigation called off main thread");
> +  LOG(("OnNavigation for %llu", aWindowID));

Did you mean to leave in this?

@@ +2265,3 @@
>  void
> +MediaManager::IterateWindowListeners(nsPIDOMWindow *aWindow,
> +                                     WindowListenerCallback aCallback,

I think I've commented on this before, and was told to wait until this cleanup I believe, so here goes:

I recommend renaming aWindow to aInnerWindow here and assert that it is an inner-window which removes an unused code-path and reduces the complexity of this function which right now unnecessarily can be called with either an inner or an outer window.

@@ +2293,5 @@
>          docShell->GetChildAt(i, getter_AddRefs(item));
>          nsCOMPtr<nsPIDOMWindow> win = item ? item->GetWindow() : nullptr;
>  
>          if (win) {
> +          IterateWindowListeners(win, aCallback, aData);

Then add code here to pass in an innerWindow.

@@ +2300,5 @@
>      }
>    }
>  }
>  
> +

extra line?

::: dom/media/MediaManager.h
@@ +535,5 @@
>    MediaEngineAudioSource* GetSource();
>  };
>  
> +// we could add MediaManager if needed
> +typedef void (*WindowListenerCallback)(MediaManager *aThis,

aThis == ugly. Please make callbacks private member functions instead, as that is effectively what they are. I have a patch that shows this.

@@ +537,5 @@
>  
> +// we could add MediaManager if needed
> +typedef void (*WindowListenerCallback)(MediaManager *aThis,
> +                                       uint64_t aWindowID,
> +                                       StreamListeners *aListeners,

None of the callbacks do anything if aListeners is null, so why not keep aListeners invariant and pass it by reference? Less can go wrong.

@@ +538,5 @@
> +// we could add MediaManager if needed
> +typedef void (*WindowListenerCallback)(MediaManager *aThis,
> +                                       uint64_t aWindowID,
> +                                       StreamListeners *aListeners,
> +                                       void *aData);

I don't like that we now have to deal with void* and blind casting where before we didn't. I suggest we avoid this by making InvokeWindowListeners a template member function.

Comment 13

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

Comment on attachment 8484235 [details] [diff] [review]
refactor window iteration code for MediaManager

whoops. splinter collision.

Comment 14

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

> @@ +1850,5 @@
> >  void
> >  MediaManager::OnNavigation(uint64_t aWindowID)
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "OnNavigation called off main thread");
> > +  LOG(("OnNavigation for %llu", aWindowID));
> 
> Did you mean to leave in this?

yes

> 
> @@ +2265,3 @@
> >  void
> > +MediaManager::IterateWindowListeners(nsPIDOMWindow *aWindow,
> > +                                     WindowListenerCallback aCallback,
> 
> I think I've commented on this before, and was told to wait until this
> cleanup I believe, so here goes:
> 
> I recommend renaming aWindow to aInnerWindow here and assert that it is an
> inner-window which removes an unused code-path and reduces the complexity of
> this function which right now unnecessarily can be called with either an
> inner or an outer window.

The choice is 3x of "window->IsInnerWindow ? window : window->GetCurrentInnerWindow()" (and a null-check following), or one instance of it in IterateWindowListeners.  Hoisting it might make more sense when it wasn't shared.  (Oh, it's recursive as well, so we'd need to move the copy of that to the recursion point, instead of removing it, so we'd just be adding 3 copies and moving one).



> @@ +537,5 @@
> >  
> > +// we could add MediaManager if needed
> > +typedef void (*WindowListenerCallback)(MediaManager *aThis,
> > +                                       uint64_t aWindowID,
> > +                                       StreamListeners *aListeners,
> 
> None of the callbacks do anything if aListeners is null, so why not keep
> aListeners invariant and pass it by reference? Less can go wrong.

GetWindowListeners returns an * anyways, so it really doesn't help much.

> @@ +538,5 @@
> > +// we could add MediaManager if needed
> > +typedef void (*WindowListenerCallback)(MediaManager *aThis,
> > +                                       uint64_t aWindowID,
> > +                                       StreamListeners *aListeners,
> > +                                       void *aData);
> 
> I don't like that we now have to deal with void* and blind casting where
> before we didn't. I suggest we avoid this by making InvokeWindowListeners a
> template member function.

Absolutely.  Thanks for the patch

Comment 15

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

Attachment: refactor window iteration code for MediaManager

Comment 16

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

Comment on attachment 8485099 [details] [diff] [review]
refactor window iteration code for MediaManager

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

r=me with nit.

::: dom/media/MediaManager.cpp
@@ +1831,5 @@
> +{
> +  if (aListeners) {
> +    auto length = aListeners->Length();
> +    for (size_t i = 0; i < length; ++i) {
> +      auto listener = aListeners->ElementAt(i);

Perhaps auto& to avoid pass-by-value and refcounting in this loop.

Comment 17

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

Comment on attachment 8485099 [details] [diff] [review]
refactor window iteration code for MediaManager

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not hard once one realizes what the purpose is, and the refactor tends to hide that.  The actual exploit that leaves the camera active is easy once you realize iframes cause it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? not really - the refactor tends to hide that OnNavigation changes from one-off to tree-walk.  It's also very non-obvious that's connected to iframes.

Which older supported branches are affected by this flaw? back to 29, when Stop Sharing landed.  Includes ESR31

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? backports shouldn't be hard or risky.  33+ are easy, since those are very close to 35.

How likely is this patch to cause regressions; how much testing does it need? basic testing of the parts touched: Stop sharing, stop screensharing, navigation (front/back/URL/reload etc), with and without iframes.  Regression risk is pretty low.

Note: likely land the other bug on navigation with iframes not invoking OnNavigation (ends up in the bfcache) with this.

Comment 18

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

Do you have a suggested security rating for this issue?

Comment 19

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

I'd suggest sec-high (for the privacy impact).

Comment 20

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

Comment on attachment 8485099 [details] [diff] [review]
refactor window iteration code for MediaManager

sec-approval for checkin on 9/16 or later (two weeks into the cycle).

Comment 21

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

Comment on attachment 8485099 [details] [diff] [review]
refactor window iteration code for MediaManager

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 885796 

User impact if declined: Privacy risk

Testing completed: personal testing of all the various end-sharing options and navigation

Risk to taking this patch (and alternatives if risky): Low risk outside of coding errors.  Mostly just a basic clean refactor, plus taking a case that didn't walk the tree and making it instead call the walk-the-tree iterator.

String or UUID changes made by this patch: none



[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high

User impact if declined: Privacy risk

Fix Landed on Version: About to land per s-a approval in 35 (and likely 34/33, and I'd consider asking for 32 (probably with a simpler patch) if there's a spill.

Risk to taking this patch (and alternatives if risky): very low risk once verified in the other builds

String or UUID changes made by this patch: none

Comment 22

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

I don't see how hiding this bug benefits us - the bug is typically visible to the user when it occurs, and the "exploit" requires getting the user to grant permission once anyhow.

Comment 23

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

I think sec-high is also vastly overstating the impact here.

Comment 28

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

 https://hg.mozilla.org/integration/mozilla-inbound/rev/786737c759d7

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/786737c759d7

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/321f747c7d7f

Needs rebasing for beta/esr31 uplift.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d508b53c3dee
https://hg.mozilla.org/releases/mozilla-esr31/rev/f81868952e3d

Comment 32

[Ada [:adalucinet]]

Reproduced with Nightly 2014-09-04: when I hit "Stop Sharing" option, the streaming continues on http://jsfiddle.net/codepo8/agaRe/4/. 
Verified as fixed with Firefox 33 beta 6 (Build ID: 20140922173023), latest Aurora 34.0a2 (Build ID: 20140923004002) and latest Nightly 35.0a1 (Build ID: 20140923030204) on Windows 7 64-bit, Linux 14.04 32-bit and Mac OS X 10.9.5 with both links from comment 0 and comment 5.

Comment 33

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

Verified fixed on Fx31.1.1esr.

Comment 34

[Florian Quèze [:florian]]

I added tests for this in bug 1120546.