1426363 - Host permissions with scheme wildcard (*://...) are not shown in permissions doorhangers

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Rob Wu [:robwu]]

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

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Security-relevant regression in 58 that we should avoid shipping.

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Julien Cristau [:jcristau]]

tracking per comment 1.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Andrew Swan [:aswan]]

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...

Comment 6

[Andrew Swan [:aswan]]

Attachment: Fix host permission parsing for permission strings

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

Comment 7

[Andrew Swan [:aswan]]

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.

Comment 8

[Rob Wu [:robwu]]

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

[Daniel Veditz [:dveditz]]

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?

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

(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 {

Comment 11

[Andrew Swan [:aswan]]

(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.

Comment 12

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

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.

Comment 13

[Andrew Swan [:aswan]]

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

Comment 14

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1355701c926172d8f9825813e9f7e66421536a
Bug 1426363 Fix host permission parsing for permission strings r=rpl a=abillings

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4d1355701c92

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8938474 [details] [diff] [review]
Fix host permission parsing for permission strings

Fix a sec-high. Beta58+.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/50a2c8cc35fdd51376e75dc1c2f234c800536188

Comment 18

[Emil Ghitta, QA [:emilghitta]]

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.