1431371 - (CVE-2018-5135) activeTab permission allows executing scripts on pages it shouldn't

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Abdulrahman Alqabandi]

Attachment: 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

Comment 1

[Andrew Swan [:aswan]]

Attachment: wip

MozReview-Commit-ID: ApnAwGsIOaG

Comment 2

[Andrew Swan [:aswan]]

Attachment: Only grant activeTab to urls that match <all_urls>

MozReview-Commit-ID: ApnAwGsIOaG

Comment 3

[Andrew Swan [:aswan]]

Attachment: Test for activeTab on an about: page

MozReview-Commit-ID: JxrDeE672sV

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Andrew Swan [:aswan]]

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.

Comment 6

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

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.

Comment 7

[Andrew Swan [:aswan]]

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

Comment 8

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

Ideally, the tests would wait until the fix ships. You can nominate this for beta but moderates normally ride the release trains from trunk.

Comment 10

[:Gijs (he/him)]

Can this land?

Comment 11

[Andrew Swan [:aswan]]

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

Yeah, sorry for the delay, I'll land it today.

Comment 12

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe5144564bcd624e42128f6545a790650620d6d
Bug 1431371 Only grant activeTab to urls that match <all_urls> r=kmag

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/7fe5144564bc

Comment 14

[Andrew Swan [:aswan]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Fix a webextension sec issue. Approved for 59b12.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/30b4121ee4c1

Comment 18

[Frederik Braun [:freddy]]

Requesting bounty consideration on behalf of the reporter.