1201973 - "Stop Sharing" in gUM in-use doorhanger doesn't revoke persistent permissions in different-domain iframe

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

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

"Stop sharing" must revoke persistent permissions, or it has not teeth. Try this (marking sec initially to not immediately publicize this trick, though it is fairly obvious. Please try it before removing):

STR:

1. Go to https://jsfiddle.net/jib1/g7atw687/
2. Don't hit "Share", instead choose "Always Share" in the list (trust me).
3. Try to turn off the camera using "Stop Sharing" from the in-use doorhanger.

Expected result:
- Camera stops, and (because the script is importunate) the gUM doorhanger re-appears.

Actual result:
- camera keeps rolling (after brief iframe refresh flicker).

What makes this bug extra foul is how hard it is to revoke permissions in Firefox (you shouldn't have trusted me):

Non-working workaround #1:

- Reset camera permission under "More Information..." in the URL page-info
  dropdown and then open the "Permissions" tab.
  Doesn't work because jsfiddle.net uses an iframe from a different domain.

Non-working workaround #2:

- Right-click on the frame and choose "View Page Info", then "Permissions" tab.
  Still takes you to the top Page info, not the frame's (Bug 1201972).

Non-working workaround #3:

- Open Preferences. There is no centralized UX for Permission Management (Bug 1201961).

Working workaround:

- Type in "about:permissions", and locate NOT jsfiddle.net but the domain from the iframe.
  You wrote it down from the doorhanger you can no longer access, didn't you? Then reset
  camera permission.

  (It's "fiddle.jshell.net")

Comment 1

[Florian Quèze [:florian]]

We have code in place that's supposed to handle this (http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm?rev=47a89d8276cb#865) and I'm pretty sure it's covered by unit tests, so this bug has surprised me.

Possible ideas:
- the issue could be e10s-only (our gUM UI tests don't run with e10s: bug 1071623).
- if it's a recent regression, bug 1173523 could be the cause.

Comment 2

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> "Stop sharing" must revoke persistent permissions, or it has not teeth.

So, stop sharing does revoke persistent permissions.

If you load https://fiddle.jshell.net/jib1/g7atw687/show/ (the relevant frame) directly, grant persistent permissions, then stop sharing, you'll get a permission prompt again when the page reloads.

So the bug here is that when the user clicks "stop sharing," we revoke persistent permissions for the hostname of the top-level page, rather than the domain of the frame actually using devices.

Comment 3

[Florian Quèze [:florian]]

Attachment: Fix / hack

I think a real fix here would require bug 1066082.

The attached patch is a hack that we could probably take if we want to fix this bug sooner than later.

Comment 4

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

Comment on attachment 8658688 [details] [diff] [review]
Fix / hack

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

Worked. About the _devicePermissions, I have a high tolerance for hacks, so you may want to run that by someone else (I don't know if there are are any side-effects). Also one question below:

::: browser/modules/webrtcUI.jsm
@@ +871,5 @@
>      label: stringBundle.getString("getUserMedia.stopSharing.label"),
>      accessKey: stringBundle.getString("getUserMedia.stopSharing.accesskey"),
>      callback: function () {
> +      let uris = aBrowser._devicePermissionURIs || [];
> +      uris.push(Services.io.newURI(aState.documentURI, null, null));

I don't know this code well enough, but isn't aBrowser global? Will this do the right thing if I have cameras running in two unrelated tabs, and revoke one of them?

@@ +876,5 @@
> +      for (let uri of uris) {
> +        let perms = Services.perms;
> +        if (aState.camera &&
> +            perms.testExactPermission(uri, "camera") == perms.ALLOW_ACTION)
> +          perms.remove(uri, "camera");

Nit: the for loops could go inside the tests, not that it matters much in practice.

@@ +918,5 @@
>    }
>  
>    // Now handle the screen sharing indicator.
>    if (!aState.screen) {
> +    removeBrowserNotification(aBrowser, "webRTC-sharingScreen");

Do we need to do a similar thing for screen sharing? (again I could be reading the code wrong)

Comment 5

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

Note to self: once we fix this we should perhaps see if there are similar issues in Android and B2G.

Comment 6

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)

> I don't know this code well enough, but isn't aBrowser global?

No, the 'a' prefix (which we tend to no longer use these days) means 'argument'. Global variables have a 'g' prefix.

> Will this do
> the right thing if I have cameras running in two unrelated tabs, and revoke
> one of them?

Yes.

> @@ +876,5 @@
> > +      for (let uri of uris) {
> > +        let perms = Services.perms;
> > +        if (aState.camera &&
> > +            perms.testExactPermission(uri, "camera") == perms.ALLOW_ACTION)
> > +          perms.remove(uri, "camera");
> 
> Nit: the for loops could go inside the tests, not that it matters much in
> practice.

Looks like the 'let perms = Services.perms;' line should move outside of the loop. But yeah, in most cases there will be only one url.

> @@ +918,5 @@
> >    }
> >  
> >    // Now handle the screen sharing indicator.
> >    if (!aState.screen) {
> > +    removeBrowserNotification(aBrowser, "webRTC-sharingScreen");
> 
> Do we need to do a similar thing for screen sharing? (again I could be
> reading the code wrong)

AFAIK we don't support persistent permissions for screensharing.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f556a538b2

Comment 8

[:Gijs (he/him)]

Comment on attachment 8659254 [details] [diff] [review]
Patch v2

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

in summary: s/persistant/persistent/, except that for sec-sensitive bugs, I didn't think we should have a summary at all - but by now this has been pushed to try already, so...

::: browser/modules/webrtcUI.jsm
@@ +396,5 @@
> +
> +          // Remember on which URIs we set persistent permissions so that we
> +          // can remove them if the user clicks 'Stop Sharing'. There's no
> +          // other way for the stop sharing code to know the hostnames of frames
> +          // using devices until bug 1066082 is fixed.

This sounds like this won't work anymore once the tab is closed. That seems broken.


Reading more context than this patch (ugh at no reviewboard for security bugs), it seems this is in a block that runs when the permissions are already set, which means this comment is misleading ("we set persistent permissions" -- no, we already have them!). Please fix the comment.

@@ +399,5 @@
> +          // other way for the stop sharing code to know the hostnames of frames
> +          // using devices until bug 1066082 is fixed.
> +          let browser = this.browser;
> +          browser._devicePermissionURIs = browser._devicePermissionURIs || [];
> +          browser._devicePermissionURIs.push(uri);

Seems like a Set would be a more correct datatype here, except that you're storing uri objects and so they are never actually going to be equal. Could we use a Set and just pass in uri.spec , constructing the nsIURI when we remove items using the Set ?

@@ +878,5 @@
>      label: stringBundle.getString("getUserMedia.stopSharing.label"),
>      accessKey: stringBundle.getString("getUserMedia.stopSharing.accesskey"),
>      callback: function () {
> +      let uris = aBrowser._devicePermissionURIs || [];
> +      uris.push(Services.io.newURI(aState.documentURI, null, null));

This is confusing though. Why do we need to add an additional URI when people click "stop sharing" ? Won't this already be in the list?

Comment 9

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #8)

> in summary: s/persistant/persistent/, except that for sec-sensitive bugs, I
> didn't think we should have a summary at all - but by now this has been
> pushed to try already, so...

I don't think this is actually a sensitive bug. comment 0 said it was marked sensitive 'initially'.
The risk of users being abused due to this bug exists, but it seems low, as the user needs to be convinced to click "Always share" rather than "Share". And the indicators aren't broken, so someone in this situation should notice that something is not working correctly.

> ::: browser/modules/webrtcUI.jsm
> @@ +396,5 @@
> > +
> > +          // Remember on which URIs we set persistent permissions so that we
> > +          // can remove them if the user clicks 'Stop Sharing'. There's no
> > +          // other way for the stop sharing code to know the hostnames of frames
> > +          // using devices until bug 1066082 is fixed.
> 
> This sounds like this won't work anymore once the tab is closed. That seems
> broken.

If the tab is closed, the frame no longer exists, so there's no need to remember its url anymore.

> Reading more context than this patch (ugh at no reviewboard for security
> bugs), it seems this is in a block that runs when the permissions are
> already set, which means this comment is misleading ("we set persistent
> permissions" -- no, we already have them!). Please fix the comment.

I have a comment saying "Remember on which URIs we *set* persistent permissions" in the block where we saved the permissions, and a second comment in the block where we used existing permissions where it says "Remember on which URIs we *had* persistent permissions".

> @@ +399,5 @@
> > +          // other way for the stop sharing code to know the hostnames of frames
> > +          // using devices until bug 1066082 is fixed.
> > +          let browser = this.browser;
> > +          browser._devicePermissionURIs = browser._devicePermissionURIs || [];
> > +          browser._devicePermissionURIs.push(uri);
> 
> Seems like a Set would be a more correct datatype here, except that you're
> storing uri objects and so they are never actually going to be equal. Could
> we use a Set and just pass in uri.spec , constructing the nsIURI when we
> remove items using the Set ?

We could, but I'm not sure it would be a simplification (and it doesn't matter too much if we store a duplicate).

> @@ +878,5 @@
> >      label: stringBundle.getString("getUserMedia.stopSharing.label"),
> >      accessKey: stringBundle.getString("getUserMedia.stopSharing.accesskey"),
> >      callback: function () {
> > +      let uris = aBrowser._devicePermissionURIs || [];
> > +      uris.push(Services.io.newURI(aState.documentURI, null, null));
> 
> This is confusing though. Why do we need to add an additional URI when
> people click "stop sharing" ? Won't this already be in the list?

Hmm... this code isn't well written indeed. Adding the uri to _devicePermissionURIs is unintended, I should have used .concat to duplicate the array.

And no, this URI isn't guaranteed to be in the list. Eg. you could have started sharing your camera in this tab, then shared it again with 'always share' in a second tab, and when you click 'stop sharing' in the first tab, we do need to revoke the persistent permission that has been saved by the second tab.


An alternative approach that may be more reliable (and less confusing in the code) could be to just remove persistent permissions for the domains of all the frames that are part of the current tab. Even frames that have never tried to use a device. Would you prefer that?

Comment 10

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> 
> > in summary: s/persistant/persistent/, except that for sec-sensitive bugs, I
> > didn't think we should have a summary at all - but by now this has been
> > pushed to try already, so...
> 
> I don't think this is actually a sensitive bug. comment 0 said it was marked
> sensitive 'initially'.
> The risk of users being abused due to this bug exists, but it seems low, as
> the user needs to be convinced to click "Always share" rather than "Share".
> And the indicators aren't broken, so someone in this situation should notice
> that something is not working correctly.
> 
> > ::: browser/modules/webrtcUI.jsm
> > @@ +396,5 @@
> > > +
> > > +          // Remember on which URIs we set persistent permissions so that we
> > > +          // can remove them if the user clicks 'Stop Sharing'. There's no
> > > +          // other way for the stop sharing code to know the hostnames of frames
> > > +          // using devices until bug 1066082 is fixed.
> > 
> > This sounds like this won't work anymore once the tab is closed. That seems
> > broken.
> 
> If the tab is closed, the frame no longer exists, so there's no need to
> remember its url anymore.

My point is, I expect "stop sharing" to revoke the persistent permission even if it was granted in a different session. The comment made it seem that this code is adding a URI to an array when the permission is granted, and the array is killed off when the tab is killed off, ergo the data will not be there the next time you open this website.

> > Reading more context than this patch (ugh at no reviewboard for security
> > bugs), it seems this is in a block that runs when the permissions are
> > already set, which means this comment is misleading ("we set persistent
> > permissions" -- no, we already have them!). Please fix the comment.
> 
> I have a comment saying "Remember on which URIs we *set* persistent
> permissions" in the block where we saved the permissions, and a second
> comment in the block where we used existing permissions where it says
> "Remember on which URIs we *had* persistent permissions".

We seem to be talking past each other. The comment here says "on which URIs we set persistent permissions", which implied to me that this is code that is setting the permissions *now*, not code that runs whenever the permissions *are already set*. The comments here just need to be clearer.

> > @@ +399,5 @@
> > > +          // other way for the stop sharing code to know the hostnames of frames
> > > +          // using devices until bug 1066082 is fixed.
> > > +          let browser = this.browser;
> > > +          browser._devicePermissionURIs = browser._devicePermissionURIs || [];
> > > +          browser._devicePermissionURIs.push(uri);
> > 
> > Seems like a Set would be a more correct datatype here, except that you're
> > storing uri objects and so they are never actually going to be equal. Could
> > we use a Set and just pass in uri.spec , constructing the nsIURI when we
> > remove items using the Set ?
> 
> We could, but I'm not sure it would be a simplification (and it doesn't
> matter too much if we store a duplicate).
> 
> > @@ +878,5 @@
> > >      label: stringBundle.getString("getUserMedia.stopSharing.label"),
> > >      accessKey: stringBundle.getString("getUserMedia.stopSharing.accesskey"),
> > >      callback: function () {
> > > +      let uris = aBrowser._devicePermissionURIs || [];
> > > +      uris.push(Services.io.newURI(aState.documentURI, null, null));
> > 
> > This is confusing though. Why do we need to add an additional URI when
> > people click "stop sharing" ? Won't this already be in the list?
> 
> Hmm... this code isn't well written indeed. Adding the uri to
> _devicePermissionURIs is unintended, I should have used .concat to duplicate
> the array.
> 
> And no, this URI isn't guaranteed to be in the list. Eg. you could have
> started sharing your camera in this tab, then shared it again with 'always
> share' in a second tab, and when you click 'stop sharing' in the first tab,
> we do need to revoke the persistent permission that has been saved by the
> second tab.
> 
> 
> An alternative approach that may be more reliable (and less confusing in the
> code) could be to just remove persistent permissions for the domains of all
> the frames that are part of the current tab. Even frames that have never
> tried to use a device. Would you prefer that?

I don't know how slow the permissions manager is, but I do know that we used to have issues with FB using a gazillion iframes for other reasons. It seems silly to iterate over everything when we can just iterate only over some things. If anything, we should fix the bug referenced in the comment so we can actually ask the media code "give me all the places that are sharing stuff in this doctree", where it should be doing something better than an exhaustive depth-first docshell-tree search. :-)

I think the patch isn't bad per se, but I *would* like to see the comments improved (and the by-ref-accidental-addition-to-stored-urls thing fixed).

Comment 11

[Florian Quèze [:florian]]

Attachment: Patch v3

The 2 comments were reversed compared to what I thought I had done. Sorry for the confusion. I also changed "we had persistent permissions" to "we found persistent permissions" to be slightly more explicit.

Also changed the .push call to a .concat.

There's an edge case that isn't covered by the patch: the case I mentioned in comment 9 ("you could have started sharing your camera in this tab, then shared it again with 'always share' in a second tab, and when you click 'stop sharing' in the first tab, we do need to revoke the persistent permission that has been saved by the second tab.") isn't addressed if in the first tab the device was used from a frame.
I don't think it matters much, as it's really a tiny edge case, and if the frame attempts to reload itself to abuse the persistent permission after the user stopped sharing, clicking 'stop sharing' again will this time actually revoke the persistent permission.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/215df49b91e7

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backing out Bug 1184220 wasn't enough to fix the bustage, apparently, so I'm backing out everything else from that push to hopefully get back to a clean slate.

https://hg.mozilla.org/integration/fx-team/rev/6dbfe6719621

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This relanded in https://hg.mozilla.org/integration/fx-team/rev/2524d0d7a041
Merged to m-c in https://hg.mozilla.org/mozilla-central/rev/2524d0d7a041

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> Working workaround:
> 
> - Type in "about:permissions", and locate NOT jsfiddle.net but the domain
> from the iframe.
>   You wrote it down from the doorhanger you can no longer access, didn't
> you? Then reset camera permission.

Move this to the "Non-working workaround" category -- we've killed about:permissions

What might have worked:
 * if you can find the iframe (which could hide itself) right-click in it
   and open the frame by itself from the "This Frame" sub-menu (show only,
   new tab, new window, etc).
 * from there the door hanger or Page Info will let you control the correct domain

Comment 16

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> > Working workaround:
> > 
> > - Type in "about:permissions", and locate NOT jsfiddle.net but the domain
> > from the iframe.
> >   You wrote it down from the doorhanger you can no longer access, didn't
> > you? Then reset camera permission.
> 
> Move this to the "Non-working workaround" category -- we've killed
> about:permissions

Just to be clear, this bug was fixed for 43, and about:permissions was removed in 45.

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

Managed to reproduce this issue on Windows 10 x 86 using Firefox 42.0 (20151029151421) and STR from Comment 0.
I confirm that the 'Stop Sharing' option from the in-use doorhanger revokes the "Always Share" persistent permission. 
The issue was verified across platforms [1] and using Firefox 43.0 (20151208100201).

[1]Mac OS X 10.11.1, Ubuntu 14.04 x86 and Windows 10 x86.