Closed Bug 1487478 (CVE-2018-12397) Opened 2 years ago Closed 2 years ago

"file:///*" extension permission has no warning

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(firefox-esr6063+ verified, firefox62 wontfix, firefox63+ verified, firefox64+ verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ verified
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Keywords: csectype-priv-escalation, csectype-spoof, sec-moderate, Whiteboard: [adv-main63+][adv-esr60.3+])

Attachments

(4 files)

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.
Attached image 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
Assignee: nobody → rob
Status: NEW → ASSIGNED
"Unparseable host permission file:///*" appears in the global console when the installation doorhanger is shown. Reminds me of bug 1426363 ...
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 ).
Attachment #9005277 - Attachment description: Bug 1487478 - Handle empty hosts in URL pattern → Bug 1487478 - Move MatchPattern normalization into classifyPermission
Comment on attachment 9005277 [details]
Bug 1487478 - Move MatchPattern normalization into classifyPermission

Andrew Swan [:aswan] has approved the revision.
Attachment #9005277 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fa56922a47956d230e8e6581843fa1615282ff
https://hg.mozilla.org/mozilla-central/rev/d2fa56922a47
Group: toolkit-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
What branches are affected by this issue? Also, can you help assign this bug a severity?
https://wiki.mozilla.org/Security_Severity_Ratings
Flags: needinfo?(rob)
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
Flags: needinfo?(rob)
Is this ready for Beta & ESR60 approval requests?
Flags: needinfo?(rob)
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.
Flags: needinfo?(rob) → needinfo?(aswan)
QA Contact: ddurst
(That "QA Contact" field was automatically populated for some reason unknown to me)
(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
Flags: needinfo?(aswan)
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.
Flags: needinfo?(dveditz)
Seems reasonable, please request approval for those branches.
Flags: needinfo?(dveditz)
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 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
Attachment #9015713 - Flags: approval-mozilla-esr60?
Attachment #9015713 - Flags: approval-mozilla-beta?
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.
Attachment #9015713 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9015713 [details] [diff] [review]
0001-Bug-1487478-Move-MatchPattern-normalization-into-cla.patch

Approved for ESR 60.3.
Attachment #9015713 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main63+][adv-esr60.3+]
Alias: CVE-2018-12397
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.