1487478 - (CVE-2018-12397) "file:///*" extension permission has no warning

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[Rob Wu [:robwu]]

Attachment: file-perm-nowarning.xpi

When an extension requests the "file:///*" permission, it gets access to local files, but there are no permission warnings (unlike "file://*/*", which has the "Access your data for all websites" warning).

1. Visit about:config and set xpinstall.signatures.required to false (requires Nightly or unbranded build).
2. Install the attached xpi file.
3. Visit a local file or directory in the browser.

Expected:
- At step 2, a warning should be displayed about the fact that extensions can access local files.
- At step 3, the local file should have a pink background (=extension ran script in the file).

(alternatively, depending on the resolution of bug 1487353, the local file should not be pink, and the warning at step 2 is not relevant)

Actual:
- At step 2, no warnings are displayed.

Comment 1

[Rob Wu [:robwu]]

Attachment: no-warning.png

Screenshot taken at step 2 in Firefox Nightly (63). I also confirmed this bug in stable (61).

Compare this with the screenshot from bug 1487353, where the permission warning was actually displayed: https://bug1487353.bmoattachments.org/attachment.cgi?id=9005150

Comment 2

[Rob Wu [:robwu]]

"Unparseable host permission file:///*" appears in the global console when the installation doorhanger is shown. Reminds me of bug 1426363 ...

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1487478 - Move MatchPattern normalization into classifyPermission

Comment 4

[Rob Wu [:robwu]]

I scanned AMO and found:
- 164 extensions use "file:///" in permissions, optional_permissions or content_scripts[*].matches.
- 7 extensions don't have "<all_urls>", "https://*/*", "http://*/*" or "*://*/*" in permissions or content_scripts[*].matches.

Fixing this bug would result in an additional "Access your data for all websites" warning for new installations of these 7 add-ons that are listed on AMO (AMO file IDs: 854766 866049 876007 872569 909653 940036 975288 ).

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 9005277 [details]
Bug 1487478 - Move MatchPattern normalization into classifyPermission

Andrew Swan [:aswan] has approved the revision.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fa56922a47956d230e8e6581843fa1615282ff
https://hg.mozilla.org/mozilla-central/rev/d2fa56922a47

Comment 7

[Ryan VanderMeulen [:RyanVM]]

What branches are affected by this issue? Also, can you help assign this bug a severity?
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 8

[Rob Wu [:robwu]]

All branches are affected.

This allows extensions to run content scripts in local pages without permission warnings.

Extensions cannot navigate to file:-URLs themselves, so in order to exploit this, a user needs to:
- Install an extension that exploits this vulnerability.
- Open a local file in the browser.

The impact can be improved even more when combined with bug 1487353.

I'd therefore rate this as follows:
csectype-priv-escalation,csectype-spoof,sec-moderate

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this ready for Beta & ESR60 approval requests?

Comment 10

[Rob Wu [:robwu]]

Andrew, in https://phabricator.services.mozilla.com/D4707#156518 you said:

> This bug doesn't seem like it necessarily needs to be uplifted.

Do you still believe that it's true, given comment 8 ?
If not, then I will prepare a new patch for Beta and ESR60.

Comment 11

[Rob Wu [:robwu]]

(That "QA Contact" field was automatically populated for some reason unknown to me)

Comment 12

[Andrew Swan [:aswan]]

(In reply to Rob Wu [:robwu] from comment #10)
> > This bug doesn't seem like it necessarily needs to be uplifted.
> 
> Do you still believe that it's true, given comment 8 ?

I think that's a question for abillings and/or dveditz

Comment 13

[Rob Wu [:robwu]]

Should this patch be uplifted to Beta and ESR? I believe so given the low risk of the patch and the impact (see comment 8), but I'd like a confirmation given comment 12.

Comment 14

[Daniel Veditz [:dveditz]]

Seems reasonable, please request approval for those branches.

Comment 15

[Rob Wu [:robwu]]

Attachment: 0001-Bug-1487478-Move-MatchPattern-normalization-into-cla.patch

This patch is a clone of https://hg.mozilla.org/mozilla-central/rev/d2fa56922a47 with one modification: the "throw new Error(...)" change was reverted to "Cu.reportError(...);" + "continue" to minimize the risk of causing regressions. This was approved in: https://phabricator.services.mozilla.com/D4707?id=14415#inline-26619

Comment 16

[Rob Wu [:robwu]]

Comment on attachment 9015713 [details] [diff] [review]
0001-Bug-1487478-Move-MatchPattern-normalization-into-cla.patch

Automated tests are not checked in yet, but I added extensive test coverage in the patch at bug 1484263.

The exact scenario that triggered this bug has not been added yet; I can either delay that other patch or add a test case later as a part of this bug.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Extensions can run content scripts in local pages without permission warnings.
https://bugzilla.mozilla.org/show_bug.cgi?id=1487478#c8

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The warning check is only activated for new extension installs. 7 extensions on AMO are affected: https://bugzilla.mozilla.org/show_bug.cgi?id=1487478#c4

String changes made/needed: None

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes lack of permission warning (about local file access) upon extension install.

User impact if declined: Extensions can run content scripts in local pages without permission warnings.
https://bugzilla.mozilla.org/show_bug.cgi?id=1487478#c8

Fix Landed on Version: 64, pending uplift to 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The warning check is only activated for new extension installs. 7 extensions on AMO are affected: https://bugzilla.mozilla.org/show_bug.cgi?id=1487478#c4

String or UUID changes made by this patch: None

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9015713 [details] [diff] [review]
0001-Bug-1487478-Move-MatchPattern-normalization-into-cla.patch

Baked on nightly for a week without any reported regression, approved for uplift to our last 63 beta. Thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3479e999805b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9015713 [details] [diff] [review]
0001-Bug-1487478-Move-MatchPattern-normalization-into-cla.patch

Approved for ESR 60.3.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cb73374a0e4e

Comment 21

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 64(20181010235834), latest Beta from https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta and latest ESR60 from https://tools.taskcluster.net/index/gecko.v2.mozilla-esr60.latest.firefox/win64-opt

Permission warning about data access is displayed on the doorhanger when the attached extension is installed.