Rob Wu [:robwu] Assignee
	Description • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 1 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 2 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 4 • 6 years ago

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

	Andrew Swan [:aswan]
	Comment 5 • 6 years ago

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

Andrew Swan [:aswan] has approved the revision.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 6 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 8 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 6 years ago

Is this ready for Beta & ESR60 approval requests?

	Rob Wu [:robwu] Assignee
	Comment 10 • 6 years ago

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.

	Rob Wu [:robwu] Assignee
	Comment 11 • 6 years ago

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

	Andrew Swan [:aswan]
	Comment 12 • 6 years ago

(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

	Rob Wu [:robwu] Assignee
	Comment 13 • 6 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 14 • 6 years ago

Seems reasonable, please request approval for those branches.

	Rob Wu [:robwu] Assignee
	Comment 15 • 6 years ago

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

	Rob Wu [:robwu] Assignee
	Comment 16 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 17 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 6 years ago
uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 6 years ago

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

Approved for ESR 60.3.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago
uplift

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

	Gyula Palko[:gpalko]
	Comment 21 • 6 years ago

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.