1349990 - Permanent WebRTC permissions are no longer exact host matched

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Johann Hofmann [:johannh]]

STR:

Using Firefox 53 and above:

- Go to https://badssl.com/
- Execute the following JS: "navigator.mediaDevices.getUserMedia({audio: true})"
- Check the checkbox to permanently allow the permission
- Go to https://upgrade.badssl.com/
- Notice there's no permission indicator on the identity icon
- Execute the following JS: "navigator.mediaDevices.getUserMedia({audio: true})"
- Notice that the site is "recording" without your permission. Fun!

Results:

This is likely regressed by bug 1206232 and should be trivial to fix.

This is made especially bad by bug 1349988 where permissions in the control center are only checked with the exact domain. So the WebRTC permission is never shown to the user (although the recording icon is indeed shown, thankfully). That bug has always been there and I wouldn't say it's critical since all of the powerful web permissions "are" exact host matched. Sucks in combination.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: patch

Comment 2

[Johann Hofmann [:johannh]]

Attachment: patch v2 (added missing semicolons)

Comment 3

[:Paolo Amadini]

Comment on attachment 8850928 [details] [diff] [review]
patch v2 (added missing semicolons)

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

Thanks for finding this, sadly I didn't catch it while reviewing bug 1206232. I only noticed the PermissionUI.jsm instance in bug 1206232 comment 76, which I've reviewed again now and I think is fine, even if we started using PermissionUI.jsm for permissions that don't use the exact host match, namely "image", "cookie", "popup", "install", and "indexedDB" as you specified in the patch.

Thanks for handling the bug quickly before we released the change! I guess for the future we should try to split patches like this in smaller changesets and get more eyes on them.

::: browser/modules/test/unit/test_SitePermissions.js
@@ +86,5 @@
> +      // Check that the sub-origin does inherit the permission from its parent.
> +      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.ALLOW);
> +    } else {
> +      Assert.ok(false, `Found an unknown permission ${permission} in exact host match test.
> +                        Please add new permissions from SitePermissions.jsm to this test.`);

nit: maybe we shouldn't include all these spaces in the string, for the sake of correctness. I know it doesn't matter here but code gets copied around in surprising ways...

It's a very good design to enforce a specific choice of permission type in the testing code, by the way.

Comment 4

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #3)
> Comment on attachment 8850928 [details] [diff] [review]
> patch v2 (added missing semicolons)
> 
> Review of attachment 8850928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for finding this, sadly I didn't catch it while reviewing bug
> 1206232. I only noticed the PermissionUI.jsm instance in bug 1206232 comment
> 76, which I've reviewed again now and I think is fine, even if we started
> using PermissionUI.jsm for permissions that don't use the exact host match,
> namely "image", "cookie", "popup", "install", and "indexedDB" as you
> specified in the patch.
> 
> Thanks for handling the bug quickly before we released the change! I guess
> for the future we should try to split patches like this in smaller
> changesets and get more eyes on them.
> 

Agreed. That patch just grew way too big and took several turns during implementation, I definitely learned from that.

> ::: browser/modules/test/unit/test_SitePermissions.js
> @@ +86,5 @@
> > +      // Check that the sub-origin does inherit the permission from its parent.
> > +      Assert.equal(SitePermissions.get(subUri, permission).state, SitePermissions.ALLOW);
> > +    } else {
> > +      Assert.ok(false, `Found an unknown permission ${permission} in exact host match test.
> > +                        Please add new permissions from SitePermissions.jsm to this test.`);
> 
> nit: maybe we shouldn't include all these spaces in the string, for the sake
> of correctness. I know it doesn't matter here but code gets copied around in
> surprising ways...

Yeah, I don't know why, but I thought that that whitespace gets ignored in template strings. I'll replace it.

> 
> It's a very good design to enforce a specific choice of permission type in
> the testing code, by the way.

Thanks, I agree! :)

Comment 5

[Johann Hofmann [:johannh]]

Attachment: patch v3 (changed string in test)

Updated the test string, carrying over the r+

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8851642 [details] [diff] [review]
patch v3 (changed string in test)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If you realize the vulnerability anyone with basic web development skills can exploit this, assuming they are hosting on a subdomain of a service that commonly asks for webrtc permissions. (Which should luckily be rare).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I tried to say "update" instead of "fix" host matching to not make it an obvious target from reading the commit message, but yeah, if you're smart the commit itself is hard to misunderstand.

Which older supported branches are affected by this flaw?
None (starts at Beta 53)

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Doesn't apply

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions IMO. I ran all tests locally that I think could be affected.

Comment 7

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

sec-approval+ for trunk. We'll want Aurora and Beta patches as soon as this lands made and nominated.

Comment 8

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a96c39cbe5892e49e02a91d74298837d0409c064
Bug 1349990 - Update host matching for webrtc prompts. r=Paolo

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8851642 [details] [diff] [review]
patch v3 (changed string in test)

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1206232
[User impact if declined]: User camera, microphone or screen can be leaked to sub-sites of sites that were allowed permanent permissions for either of these.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's just adding flags to a JavaScript object. This behavior was always the intended original behavior and should not fail tests.
[String changes made/needed]: None

Comment 10

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

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

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8851642 [details] [diff] [review]
patch v3 (changed string in test)

Fix a sec-high. Aurora54+ & Beta53+.

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 12

[Brindusa Tot, Desktop QA]

Hani, could you please verify this bug on latest Nightly.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/019249878268
https://hg.mozilla.org/releases/mozilla-beta/rev/67a86746e6f1

Comment 14

[Brindusa Tot, Desktop QA]

Verified the scenario from Description on latest Nightly 55.0a1 Build ID 20170329071901 and this scenario seems to be fixed.

But I observed the following, 
- Go to https://badssl.com/
- Execute the following JS: "navigator.mediaDevices.getUserMedia({audio: true})"
- Check the checkbox to permanently allow the permission and allow it permanently
- Go to https://upgrade.badssl.com/
- Notice there's no permission indicator on the identity icon
- Execute the following JS: "navigator.mediaDevices.getUserMedia({audio: true})"
- Check the checkbox to permanently allow the permission and allow it permanently also for https://upgrade.badssl.com/
- Remove the "Allowed" permission of Microphone for https://upgrade.badssl.com/.
- Go back to https://badssl.com/
	--> Observe that for https://badssl.com/ - you don't have any permission set for Microphone.

Same was observed on latest Aurora 54.0a2 build ID 20170330004004.

Is this behavior expected?

Comment 15

[Johann Hofmann [:johannh]]

Yes, it's a bit complicated, but this is necessary because right now we don't have a good mechanism for revoking sub-frame WebRTC permissions so we just remove all stream permissions that were set during the tab lifetime. Will be solved by bug 1224453.

Good catch ;)

Comment 16

[Brindusa Tot, Desktop QA]

Thanks, Johann, so I guess I can mark this bug verified on Nightly and Aurora. I will also verify the bug on Beta 53.0b8 when will be available.

But still, I have one question related to bug 1224453. Do you know if there are any plans to fix bug 1224453 in time so that it would be uplifted to 53? The scenario described in comment 14 seems to be similar with the one from the description of the bug, just that in the other way: the permission is cut off without announcing the user.

Comment 17

[Johann Hofmann [:johannh]]

No, that bug will introduce new copy and is quite complex and will have to ride the trains.

Silently removing a permission is not perfect from a UX perspective but this is an extreme edge case (you started streaming on a domain and saved the permission permanently and then immediately went to a different domain in the same tab and started streaming and cancelled streaming through the identity popup). It's also not dangerous in any way, since the user will simply be re-prompted to give permission again. This bug was about silently -giving- permission to a website without asking the user.

So I really think it's fine to ship.

Comment 18

[Brindusa Tot, Desktop QA]

Based on comment 14 and 17, mark this bug verified on Nightly 55.0a1 and Aurora 54.0a2.

Comment 19

[Hani Yacoub, Desktop QA]

Build ID: 20170330120906
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

Verified as fixed on Firefox Beta 53.0b8 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.