Closed
Bug 1349990
Opened 8 years ago
Closed 8 years ago
Permanent WebRTC permissions are no longer exact host matched
Categories
(Firefox :: Site Permissions, defect, P1)
Firefox
Site Permissions
Tracking
()
VERIFIED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox-esr45 | --- | unaffected |
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | + | verified |
firefox54 | + | verified |
firefox55 | + | verified |
People
(Reporter: johannh, Assigned: johannh)
Details
(Keywords: regression, sec-high, Whiteboard: [fxprivacy])
Attachments
(1 file, 2 obsolete files)
3.21 KB,
patch
|
johannh
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Flags: qe-verify+
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8850927 -
Attachment is obsolete: true
Attachment #8850927 -
Flags: review?(paolo.mozmail)
Attachment #8850928 -
Flags: review?(paolo.mozmail)
Updated•8 years ago
|
Comment 3•8 years ago
|
||
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.
Attachment #8850928 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(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! :)
Assignee | ||
Comment 5•8 years ago
|
||
Updated the test string, carrying over the r+
Attachment #8850928 -
Attachment is obsolete: true
Attachment #8851642 -
Flags: review+
Assignee | ||
Comment 6•8 years ago
|
||
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.
Attachment #8851642 -
Flags: sec-approval?
Comment 7•8 years ago
|
||
sec-approval+ for trunk. We'll want Aurora and Beta patches as soon as this lands made and nominated.
status-firefox52:
--- → unaffected
Updated•8 years ago
|
Attachment #8851642 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96c39cbe5892e49e02a91d74298837d0409c064 Bug 1349990 - Update host matching for webrtc prompts. r=Paolo
Assignee | ||
Comment 9•8 years ago
|
||
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
Attachment #8851642 -
Flags: approval-mozilla-beta?
Attachment #8851642 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a96c39cbe589
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•8 years ago
|
Group: firefox-core-security → core-security-release
Comment 11•8 years ago
|
||
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!
Flags: needinfo?(brindusa.tot)
Attachment #8851642 -
Flags: approval-mozilla-beta?
Attachment #8851642 -
Flags: approval-mozilla-beta+
Attachment #8851642 -
Flags: approval-mozilla-aurora?
Attachment #8851642 -
Flags: approval-mozilla-aurora+
Comment 12•8 years ago
|
||
Hani, could you please verify this bug on latest Nightly.
Flags: needinfo?(brindusa.tot)
QA Contact: hani.yacoub
Comment 13•8 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/019249878268 https://hg.mozilla.org/releases/mozilla-beta/rev/67a86746e6f1
Comment 14•8 years ago
|
||
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?
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 15•8 years ago
|
||
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 ;)
Flags: needinfo?(jhofmann)
Comment 16•8 years ago
|
||
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.
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 17•8 years ago
|
||
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.
Flags: needinfo?(jhofmann)
Comment 18•8 years ago
|
||
Based on comment 14 and 17, mark this bug verified on Nightly 55.0a1 and Aurora 54.0a2.
Comment 19•8 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
status-firefox-esr45:
--- → unaffected
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•