Closed
Bug 1426363
Opened 7 years ago
Closed 7 years ago
Host permissions with scheme wildcard (*://...) are not shown in permissions doorhangers
Categories
(WebExtensions :: Frontend, defect, P1)
Tracking
(firefox-esr52 unaffected, firefox57 unaffected, firefox58+ verified, firefox59 verified)
VERIFIED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | + | verified |
firefox59 | --- | verified |
People
(Reporter: robwu, Assigned: aswan)
References
Details
(5 keywords, Whiteboard: [post-critsmash-triage])
Attachments
(1 file)
1.15 KB,
patch
|
rpl
:
review+
gchang
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
An extension can specify host permissions in manifest.json, e.g. "http://example.com/". There is a special scheme wildcard that matches "http" and "https". I.e. ["*://host/"] is equivalent to ["https://host/", "http://host/"].
Up until Firefox 57, the permission warning correctly showed up when the user tries to install an add-on with such host permissions that start with a wildcard scheme.
As of Firefox 58 (caused by the patch for bug 1398762), this is no longer the case.
STR:
1. Try to install an add-on that uses a host permission that starts with "*://".
For example:
"*://*/*" - https://addons.mozilla.org/en-US/firefox/addon/crxviewer/
"*://*.google.com/*" (and many other google domains) - https://addons.mozilla.org/en-US/firefox/addon/dont-track-me-google1/
2. Look at the doorhanger UI that is supposed to show the permission warnings.
Expected result:
- In the example with the *://*/* permission, I expected a warning about the fact that the extension runs on all sites.
- In the example with Google domains, I expected a bunch of warnings about the extension running on google.
Actual result:
- There is no permission warning for any host permission that starts with "*://".
This should be fixed and the patch should be uplifted to Firefox 58 before it reaches stable. Otherwise extensions can request the very powerful "*://*/*" permission and trick users into installing it without any warnings.
Comment 1•7 years ago
|
||
[Tracking Requested - why for this release]:
Security-relevant regression in 58 that we should avoid shipping.
tracking-firefox58:
--- → ?
Comment 2•7 years ago
|
||
tl;dr: Host permissions with a scheme wildcard (*://...) are no longer shown in the permissions door hanger, though other host permissions (http://...) still are. Regressed by bug 1398762.
Assignee: nobody → aswan
Component: WebExtensions: General → WebExtensions: Frontend
Summary: No warning for permissions starting with scheme wildcard, e.g. "Unparseable host permission *://example.com/*" → Host permissions with scheme wildcard (*://...) are not shown in permissions doorhangers
Comment 4•7 years ago
|
||
somewhere between sec-high and sec-moderate: an extension could request permission for a specific reasonable site plus all sites and fool a user into thinking it's harmless.
Blocks: 1398762
Assignee | ||
Comment 5•7 years ago
|
||
Ugh, overzealous cleaning up of the regexp. I notice that we accept wss: and file: in match patterns too but neither the old or new regexp covered those...
Assignee | ||
Comment 6•7 years ago
|
||
Tested manually. All our automated tests for permission dialogs are in
browser/base/content/test/webextensions, those are overdue for a rewrite
to avoid using signed xpis checked into the tree...
MozReview-Commit-ID: BeisQwgKak9
Attachment #8938474 -
Flags: review?(lgreco)
Updated•7 years ago
|
Attachment #8938474 -
Flags: review?(lgreco) → review+
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Relatively easily, though the exploit takes the form of an extension, the user would still need to be convinced to install the extension.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
There aren't any comments that highlight the issue, its the actual code change that does...
Which older supported branches are affected by this flaw?
beta
If not all supported branches, which bug introduced the flaw?
bug 1398762
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
this backports trivially to beta
How likely is this patch to cause regressions; how much testing does it need?
The risk of regressions is small, this has been tested manually, more extensive testing is probably not needed.
Attachment #8938474 -
Flags: sec-approval?
Reporter | ||
Comment 8•7 years ago
|
||
Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings
Review of attachment 8938474 [details] [diff] [review]:
-----------------------------------------------------------------
This fixes the security bug, but introduces a new minor bug: Host permissions with the file:-scheme should not be treated as http(s) origins since there is no relationship between the hosts of the two classes of schemes (http(s), wss(s), ftp, * versus file - this list is currently exhaustive [1]).
The impact is very small, because WebExtensions can currently only use file://*-permissions to run content scripts. Most extensions that need to run at file: will probably also need access to other sites.
- Before this patch, there has never been any warning for file://-permissions.
- Chromium does not show any warnings for file permissions, but users have a checkbox to opt-in to file access at the extension page ( chrome://extensions ).
- I suggest to add a check for "file:" and introduce a new permission warning for local files (with the same warnings for file://* and file://tmp/*). This permission warning can then be re-used if an API for local file access is introduced (bug 1266960, bug 1246236).
[1] https://searchfox.org/mozilla-central/rev/22c55eb7b7e6494a8615a7af3b613ff899d2cdba/toolkit/components/extensions/schemas/manifest.json#478
Comment 9•7 years ago
|
||
The new regexp will accept chrome: and about: and all kinds of crazy schemes. Will those get caught by the regexp in the manifest.json schema and prevented? Or would the user get prompted for those? Would it be better to use the same core regexp?
Flags: needinfo?(aswan)
Comment 10•7 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #9)
> The new regexp will accept chrome: and about: and all kinds of crazy
> schemes. Will those get caught by the regexp in the manifest.json schema and
> prevented? Or would the user get prompted for those? Would it be better to
> use the same core regexp?
Currently, an "host permission" that is invalid from a schema point of view, it does not reach the regexp from the code that formats the list of the extension permissions, so that they are not going to be shown to the user in the install doorhanger UI.
The extension manifest and the "host permissions" included in the manifest are validated based on the following types defined in the manifest.json schema file:
- "permissions" is a property of type "array of PermissionOrOrigin https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/extensions/schemas/manifest.json#172-180
- "PermissionOrOrigin" can match a "Permission" or a "MatchPattern" type https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/extensions/schemas/manifest.json#407-413
- "MatchPattern" type is defined as (https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/extensions/schemas/manifest.json#470-485):
```
{
"id": "MatchPattern",
"choices": [
{
"type": "string",
"enum": ["<all_urls>"]
},
{
"type": "string",
"pattern": "^(https?|wss?|file|ftp|\\*)://(\\*|\\*\\.[^*/]+|[^*/]+)/.*$"
},
{
"type": "string",
"pattern": "^file:///.*$"
}
]
},
```
While discussing this patch with aswan I've also pointing out that:
- it looks that the original regexp used by that code (before the regression from Bug 1398762) was missing the ftp protocol
(and it is one of those that are explicitly supported by the manifest schema) and the file protocol as well
- the new regexp will match the file protocol, but a file "host permission" is likely to be "file:///*",
which with the old as well as the new regexp, it is not shown to the user in the permissions prompt at install time
(with the attached patch applied "file:///*" is not going to be listed because the host is an empty string,
on the contrary if the host permission is like "file://fakehost/*" it is going to be shown as
"Access your data for fakehost")
And so I do agree with Rob that handling the file protocol explicitly is also necessary, but I'm wondering if it would be reasonable to handle it in a separate security bug (e.g. it is going to require new locale strings in the properties files for both the Firefox Desktop and Firefox for Android, which could be a new "webextPerms.hostDescription.fileUrls=Access your local files").
Also, it looks that we will have to avoid to "break" the loop over the "host permissions" if we find an "<all_urls>" permission, so that we don't allow a malicious extension to hide a "file://..." permission behind an <all_urls> permission:
diff --git a/toolkit/components/extensions/Extension.jsm b/toolkit/components/extensions/Extension.jsm
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -831,24 +831,31 @@ this.ExtensionData = class {
*/
static formatPermissionStrings(info, bundle) {
let result = {};
let perms = info.permissions || {origins: [], permissions: []};
// First classify our host permissions
let allUrls = false, wildcards = new Set(), sites = new Set();
+ let fileUrls = false;
+
for (let permission of perms.origins) {
if (permission == "<all_urls>") {
allUrls = true;
- break;
+ continue;
+ }
+ if (permission.startsWith("file:")) {
+ fileUrls = true;
+ continue;
}
if (permission.startsWith("moz-extension:")) {
continue;
}
+
let match = /^[a-z*]+:\/\/([^/]+)\//.exec(permission);
if (!match) {
Cu.reportError(`Unparseable host permission ${permission}`);
continue;
}
if (match[1] == "*") {
allUrls = true;
} else if (match[1].startsWith("*.")) {
@@ -857,16 +864,19 @@ this.ExtensionData = class {
sites.add(match[1]);
}
}
// Format the host permissions. If we have a wildcard for all urls,
// a single string will suffice. Otherwise, show domain wildcards
// first, then individual host permissions.
result.msgs = [];
+ if (fileUrls) {
+ result.msgs.push(bundle.GetStringFromName("webextPerms.hostDescription.fileUrls"));
+ }
if (allUrls) {
result.msgs.push(bundle.GetStringFromName("webextPerms.hostDescription.allUrls"));
} else {
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #9)
> The new regexp will accept chrome: and about: and all kinds of crazy
> schemes. Will those get caught by the regexp in the manifest.json schema and
> prevented? Or would the user get prompted for those? Would it be better to
> use the same core regexp?
As Luca said, this is one of at least 3 different bits of code that interpret permissions :( But this code is only responsible for the presentation of permissions to users, a string won't make it here unless it is accepted by the schema validator which only accepts these patterns:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json#471-484
I agree that we're not handling file: very well right now but we didn't handle it very well before either. Trying to handle it now is going to get into a big debate about how to word the prompt among other things, I vote for dealing with file: urls over in bug 1266960.
Flags: needinfo?(aswan)
Comment 12•7 years ago
|
||
Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings
sec-approval+ for trunk. Please nominate a patch for the beta branch as well.
Attachment #8938474 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings
(this is the original trunk patch but it applies cleanly to beta)
Approval Request Comment
[Feature/Bug causing the regression]:
bug 1398762
[User impact if declined]:
As described in the bug, some webextension host/origin permissions are not shown in permission prompts.
[Is this code covered by automated tests?]:
no
[Has the fix been verified in Nightly?]:
Manually by the patch author, not yet by QA
[Needs manual test from QE? If yes, steps to reproduce]:
Yes / see the original bug
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
The change is small and easy to analyze, moreover the changed code is only used for formatting messages to be displayed in permission prompts.
[String changes made/needed]:
none
Attachment #8938474 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 14•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1355701c926172d8f9825813e9f7e66421536a
Bug 1426363 Fix host permission parsing for permission strings r=rpl a=abillings
Comment 15•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 16•7 years ago
|
||
Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings
Fix a sec-high. Beta58+.
Attachment #8938474 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•7 years ago
|
||
uplift |
Updated•7 years ago
|
Flags: sec-bounty?
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•7 years ago
|
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Comment 18•7 years ago
|
||
I have managed to reproduce the issue described in comment 0 using Firefox 59.0a1 (BuildId:20171220220602).
This issue is verified fixed using Firefox 58.0 (BuildId:20180118215408) and Firefox 59.0a1 (BuildId:20180118220148) on Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.13.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•7 years ago
|
Group: toolkit-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•6 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•