Closed Bug 1062876 (CVE-2014-1585) Opened 5 years ago Closed 5 years ago

The "stop sharing" option in the video sharing control in the URL bar has no effect in iframes

Categories

(Core :: WebRTC, defect)

32 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 + verified
firefox34 + verified
firefox35 + verified
firefox-esr31 33+ verified
b2g-v2.0 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sheppy, Assigned: jesup)

References

Details

(Keywords: privacy, sec-moderate, Whiteboard: [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-])

Attachments

(1 file, 1 obsolete file)

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.
FWIW, no output in browser console either.
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
Group: core-security
Keywords: privacy
Version: unspecified → 32 Branch
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
"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
Simpler test:
http://mozilla.github.com/webrtc-landing/gum_iframe.html

Confirmed, it's an iframe issue
Summary: The "stop sharing" option in the video sharing control in the URL bar has no effect (in iframe?) → The "stop sharing" option in the video sharing control in the URL bar has no effect in iframes
(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.)
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
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
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")
Blocks: 885796
status-b2g-v2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Attachment #8484235 - Flags: review?(jib)
Attachment #8484235 - Flags: feedback?(florian)
Whiteboard: [webrtc-uplift]
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 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.
Attachment #8484235 - Flags: feedback?(florian) → feedback+
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.
Attachment #8484235 - Flags: review?(jib)
Attachment #8484235 - Flags: review-
Attachment #8484235 - Flags: feedback?(florian)
Attachment #8484235 - Flags: feedback+
Comment on attachment 8484235 [details] [diff] [review]
refactor window iteration code for MediaManager

whoops. splinter collision.
Attachment #8484235 - Flags: feedback?(florian) → feedback+
> @@ +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
Attachment #8484235 - Attachment is obsolete: true
Attachment #8485099 - Flags: review?(jib)
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.
Attachment #8485099 - Flags: review?(jib) → review+
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.
Attachment #8485099 - Flags: sec-approval?
Do you have a suggested security rating for this issue?
I'd suggest sec-high (for the privacy impact).
Whiteboard: [webrtc-uplift] → [webrtc-uplift][checkin on 9/16]
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).
Attachment #8485099 - Flags: sec-approval? → sec-approval+
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
Attachment #8485099 - Flags: approval-mozilla-esr31?
Attachment #8485099 - Flags: approval-mozilla-beta?
Attachment #8485099 - Flags: approval-mozilla-aurora?
Attachment #8485099 - Flags: approval-mozilla-esr31?
Attachment #8485099 - Flags: approval-mozilla-esr31+
Attachment #8485099 - Flags: approval-mozilla-beta?
Attachment #8485099 - Flags: approval-mozilla-beta+
Attachment #8485099 - Flags: approval-mozilla-aurora?
Attachment #8485099 - Flags: approval-mozilla-aurora+
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.
Group: core-security
I think sec-high is also vastly overstating the impact here.
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
 https://hg.mozilla.org/integration/mozilla-inbound/rev/786737c759d7
Whiteboard: [webrtc-uplift][checkin on 9/16] → [webrtc-uplift]
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/786737c759d7
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify+
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.
Verified fixed on Fx31.1.1esr.
Whiteboard: [adv-main33+][adv-esr31.2+]
Alias: CVE-2014-1585
Depends on: CVE-2014-1586
No longer depends on: CVE-2014-1586
Depends on: CVE-2014-1586
I added tests for this in bug 1120546.
Flags: in-testsuite+
Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-]
You need to log in before you can comment on or make changes to this bug.