Bug 1431371 (CVE-2018-5135)

activeTab permission allows executing scripts on pages it shouldn't

RESOLVED FIXED in Firefox 59

Status

P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: qab, Assigned: aswan)

Tracking

({csectype-priv-escalation, sec-moderate})

58 Branch
mozilla60
csectype-priv-escalation, sec-moderate
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 fixed, firefox60 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main59+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8943569 [details]
POC.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

-----Originally reported in bug 1429881#c2-------

browser.tabs.executeScript is blocked from running in 'about:cache' IFF coming from extensions normal background script. However, I noticed that the same is not true if executeScript is called from a browserAction popup script.

1. Install attached PoC extension
2. Navigate to 'about:cache' (or just a new tab/home)
3. Click on browser action button (top right green widget)

According to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/executeScript 
"You can only inject code into pages whose URL can be expressed using a match pattern: meaning, its scheme must be one of "http", "https", "file", "ftp""

Though there is clearly an exception somewhere for browserAction popup scripts.


Actual results:

We can execute script on 'about:cache' as well as other extension pages. Scripts are _not_ executed on privileged pages. (such as about:addons)


Expected results:

Both instances should be blocked
(Reporter)

Updated

a year ago
Group: toolkit-core-security
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Group: firefox-core-security
Summary: executeScript is not restricted when coming from browserAction popup script → activeTab permission allows executing scripts on pages it shouldn't

Updated

a year ago
Assignee: nobody → aswan
Priority: -- → P1
(Assignee)

Comment 1

a year ago
Created attachment 8945994 [details] [diff] [review]
wip

MozReview-Commit-ID: ApnAwGsIOaG
Attachment #8945994 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 2

a year ago
Created attachment 8946541 [details] [diff] [review]
Only grant activeTab to urls that match <all_urls>

MozReview-Commit-ID: ApnAwGsIOaG
Attachment #8946541 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Attachment #8945994 - Attachment is obsolete: true
Attachment #8945994 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

a year ago
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 3

a year ago
Created attachment 8946542 [details] [diff] [review]
Test for activeTab on an about: page

MozReview-Commit-ID: JxrDeE672sV
Attachment #8946542 - Flags: review?(kmaglione+bmo)
Attachment #8946541 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8946542 [details] [diff] [review]
Test for activeTab on an about: page

Review of attachment 8946542 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/browser/browser_ext_activeTab_privileged.js
@@ +10,5 @@
> +      let tab;
> +
> +      browser.test.onMessage.addListener(async () => {
> +        try {
> +          await browser.tabs.executeScript(tab.id, {

Nit: assert.rejects(...) would be better.
Attachment #8946542 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 5

a year ago
Comment on attachment 8946541 [details] [diff] [review]
Only grant activeTab to urls that match <all_urls>

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Moderately easily.  However we don't have a sec rating on this bug yet, I don't think it is especially dangerous since we already guard against injecting content scripts into pages with system principals:
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/toolkit/components/extensions/WebExtensionPolicy.cpp#396-398

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment suggests what the problem is but I'm not sure how to obfuscate it without making it completely opaque.

Which older supported branches are affected by this flaw?
The regressing patch was uplifted to 58 (which was beta at the time of uplift, and is release at the time of this comment)

If not all supported branches, which bug introduced the flaw?
bug 1396399

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
this should backport trivially to all affected branches

How likely is this patch to cause regressions; how much testing does it need?
It seems unlikely to cause regressions, our existing automation should catch any major regressions.
Attachment #8946541 - Flags: sec-approval?
Blocks: 1396399
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csectype-priv-escalation, sec-moderate
Comment on attachment 8946541 [details] [diff] [review]
Only grant activeTab to urls that match <all_urls>

As a sec-moderate, this can just go into trunk.
Attachment #8946541 - Flags: sec-approval?
(Assignee)

Comment 7

a year ago
(In reply to Al Billings [:abillings] from comment #6)
> As a sec-moderate, this can just go into trunk.

I just want to make sure I've got this right -- this should land in central (60) and ride the trains even though the bug is present in 58 and 59?  And should the test be withheld for some time or is that just for sec-high and higher?
Flags: needinfo?(abillings)
Ideally, the tests would wait until the fix ships. You can nominate this for beta but moderates normally ride the release trains from trunk.
status-firefox58: affected → wontfix
Flags: needinfo?(abillings)
Duplicate of this bug: 1435933

Comment 10

11 months ago
Can this land?
Flags: needinfo?(aswan)
(Assignee)

Comment 11

11 months ago
(In reply to :Gijs from comment #10)
> Can this land?

Yeah, sorry for the delay, I'll land it today.
https://hg.mozilla.org/mozilla-central/rev/7fe5144564bc
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Comment 14

11 months ago
Comment on attachment 8946541 [details] [diff] [review]
Only grant activeTab to urls that match <all_urls>

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1396399

[User impact if declined]:
sec-moderate vulnerability on webextensions

[Is this code covered by automated tests?]:
yes

[Has the fix been verified in Nightly?]:
via the automated test, yes

[Needs manual test from QE? If yes, steps to reproduce]: 
i don't believe additional manual testing is needed

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
The actual code change is quite small, and it just narrows the set of pages that extensions may access.

[String changes made/needed]:
none
Flags: needinfo?(aswan)
Attachment #8946541 - Flags: approval-mozilla-beta?
Group: toolkit-core-security → core-security-release
Comment on attachment 8946541 [details] [diff] [review]
Only grant activeTab to urls that match <all_urls>

Fix a webextension sec issue. Approved for 59b12.
Attachment #8946541 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1436482
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Alias: CVE-2018-5135

Updated

7 months ago
Product: Toolkit → WebExtensions
Group: core-security-release
status-firefox58: wontfix → ---
You need to log in before you can comment on or make changes to this bug.