Closed Bug 1425224 (CVE-2018-5112) Opened 6 years ago Closed 6 years ago

browser.devtools.panels.create does not ensure panel pagePath is relative URL

Categories

(WebExtensions :: Developer Tools, defect, P2)

58 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox59 verified)

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- verified

People

(Reporter: qab, Assigned: rpl)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(3 files, 3 obsolete files)

Attached file devtools-panels.zip
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

PoC attached.


1. Unpack attached PoC addon
2. Go to about:debugging and temporarily load the unpacked addon
3. Open web console and navigate to 'My Panel'



Actual results:

We can load non-relative URLs including privileged about: urls


Expected results:

According to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/create 
"The URL should be relative to the manifest.json file itself"

Also, as per Bug 1380597 this is breaking security measures.
Shane, can you take a look?


Separately, Andy, maybe we need a separate bug to audit entrypoints that take paths/urls for this type of problem? And/or some kind of more systemic check in place for this stuff, so we don't run into it again and again as we add more APIs? (To be clear, not sure how hard that is, but seems worth thinking about.)
Group: firefox-core-security → toolkit-core-security
Component: Untriaged → WebExtensions: Developer Tools
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(amckay)
Product: Firefox → Toolkit
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(lgreco)
Flags: needinfo?(amckay)
This patch changes the type used in the devtools.panels.create API schema to use ExtensionURL instead of string, which means that the schema wrapper will raise an error if the string is not a relative url:

- ExtensionURL definition:
  https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/toolkit/components/extensions/schemas/manifest.json#421-423

- Schemas.jsm format validation used by the ExtensionURL:
  https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/toolkit/components/extensions/Schemas.jsm#900-931
Assignee: nobody → lgreco
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(lgreco)
Attachment #8936883 - Flags: review?(mixedpuppy)
Comment on attachment 8936883 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema_.patch

># HG changeset patch
># User Luca Greco <lgreco@mozilla.com>
># Date 1513272059 -3600
>#      Thu Dec 14 18:20:59 2017 +0100
># Node ID a2595c4be21bfe6f6a75cf4b630c2bad59c3b25e
># Parent  8062887ff0d9382ea84177f2c21f62dc0e613d9e
>Bug 1425224 - Use manifest.ExtensionURL in devtools.panels.create API schema.
>
>diff --git a/browser/components/extensions/schemas/devtools_panels.json b/browser/components/extensions/schemas/devtools_panels.json
>--- a/browser/components/extensions/schemas/devtools_panels.json
>+++ b/browser/components/extensions/schemas/devtools_panels.json
>@@ -329,22 +329,22 @@
>         "parameters": [
>           {
>             "name": "title",
>             "type": "string",
>             "description": "Title that is displayed next to the extension icon in the Developer Tools toolbar."
>           },
>           {
>             "name": "iconPath",
>-            "type": "string",
>+            "$ref": "manifest.ExtensionURL",

Maybe use ImageDataOrExtensionURL?  

Are we also going to check in the api if the url can be accessed?
I've added a new test case and changed the type of the icon url used in the schema file,
the reason is that for devtools.panels.create also supports an empty string as the icon url and in that case the default extension icon has to be used for the panel.

The devtools.panels.create API method doesn't seem to support an icon passed as image data (also in the Chrome docs they only mention that it should be a relative url, to be fair they don't mention the empty string behavior, but I've seen an empty string used in some of the devtools extension, e.g. react devtools, and so we supported this undocumented behavior as it is supported on Chrome and used by existent extensions). 

I also tested locally some of the most used devtools addons (e.g. React DevTools, Redux DevTools, Ember Inspector and VueJS DevTools) to double check that they doesn't break with the patch applied.

An alternative approach (which I would even prefer to the approach used in this patch) would be to define an additional format in Schemas.jsm, but I've currently preferred this one mostly because it seems easier to uplift on different releases because it doesn't touch webextensions internals that could have changes in the meantime.
Attachment #8936883 - Attachment is obsolete: true
Attachment #8936883 - Flags: review?(mixedpuppy)
Attachment #8938412 - Flags: review?(mixedpuppy)
Small fix on the last version of this patch (the new version only needs the changes applied in the schema file and the new test case).

The additional details from comment 4 are all still valid for this version of the patch.
Attachment #8938412 - Attachment is obsolete: true
Attachment #8938412 - Flags: review?(mixedpuppy)
Attachment #8938435 - Flags: review?(mixedpuppy)
Comment on attachment 8938435 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema_.patch

comment the schema "empty string" choice if possible.
Attachment #8938435 - Flags: review?(mixedpuppy) → review+
Patch updated with a better description in the API schemas for the iconPath property (which now mentions explicitly the behavior of an empty string as an iconPath) and updated the commit summary to include the reviewer.
Attachment #8938435 - Attachment is obsolete: true
Attachment #8938446 - Flags: review?(mixedpuppy)
Attachment #8938446 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8938446 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema__r_mixedpuppy.patch

[Security approval request comment]

## How easily could an exploit be constructed based on the patch?
## Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The new tests is what more clearly shows which are the panel urls that
used to be loadable in the extension devtools panel and are now rejected as invalid.

Nevertheless, loading this URLs in the panel doesn't seem to give to the extension code any direct access to the window global related to the loaded URL (because the panel onShown event is never been sent for a non-extension page loaded in the panel, and this event is the only way that the extension can use to get access to the window global for the created extension panel), and so the extensions
that is trying to exploit the issue shouldn't be able to do anything more
(besides being able to load in the devtools panel URLs that they are not, and shouldn't be, able to load anywhere).

## Which older supported branches are affected by this flaw?

The related feature is support on Firefox >= 54 (which is the version in which this API has been landed) and so the affected branches should be:

- mozilla-central
- mozilla-beta
- mozilla-release

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

Bug 1300587

## Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The attached patch should be able to be imported on the older branches listed above without further changes.
(the changes on the JSON schema file should be able to apply without any conflicts, while the tests could meet some minor conflicts, but the new test cases is in its isolated test function and so I don't expect any complex conflicts).

## How likely is this patch to cause regressions; how much testing does it need?

Very unlikely: 
The behavior expected by the existent addons that are using this API as expected (with valid URLs relative to the extension base URI) is unmodified by this patch, and so non-malicious extensions should not notice any change or regression.

The change can only affects the devtools.panels API namespace, the devtools.panels.create API method in particular, and so only extensions that are creating a devtools panel can regress.

It can be tested explicitly using the following STR (the same that I used to verify the patch locally on mozilla-central):

- Install one or more of the following extensions:
    AdBlock Plus, React DevTools, Redux DevTools, Ember Inspector, VueJS DevTools
- load a webpage in a new tab 
  (for same tool is better to choose a page that uses the JS framework related
  to the devtools panel, because the extension will not create the panel otherwise, 
  e.g. searching from "<framework_name> todo mvc" is usually enough to find a
  webpage to test the extension on)
- open the developer tools on the new tab
- verify that the icon related to the extension panel is visible as expected
- select the extension devtools panel and verify that it is loaded as expected
Attachment #8938446 - Flags: sec-approval?
No need for sec-approval for sec-moderate bugs: https://wiki.mozilla.org/Security/Bug_Approval_Process
Comment on attachment 8938446 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema__r_mixedpuppy.patch

As qab noted, sec-approval not strictly necessary for sec-moderate bugs. If this were a more severe bug we'd minus this request and ask you to move the tests into a separate patch to be checked in after we released a fix (or even a separate bug if that would help you track it better).

Since it's moderate it's fine.
Attachment #8938446 - Flags: sec-approval? → sec-approval+
(In reply to Daniel Veditz [:dveditz] from comment #10)
> As qab noted, sec-approval not strictly necessary for sec-moderate bugs. If
> this were a more severe bug we'd minus this request and ask you to move the
> tests into a separate patch to be checked in after we released a fix (or
> even a separate bug if that would help you track it better).
> 
> Since it's moderate it's fine.

Thanks a lot for these additional details, it was my first sec-approval,
and these details are be very helpful to me, so that I can use
the right workflow upfront when preparing this kind of patches.
Comment on attachment 8938446 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema__r_mixedpuppy.patch

Approval Request Comment

[Feature/Bug causing the regression]: Bug 1300587 (which has introduced support for the devtools.panels.create API method in Firefox >= 54)

[User impact if declined]: An extension may open in a devtools panel some non-"moz-extension://" urls (the list of the urls that are actually able to load in the panel varies based on the process where the extension pages are loaded, e.g. about:addons only loads successfully if the extension pages are running in the main process).

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

[Has the fix been verified in Nightly?]: Not yet

[Needs manual test from QE? If yes, steps to reproduce]: Yes, see Comment 0 for the STR, it would be also reasonable to test manually some of the common devtools addons from AMO (e.g. React/Redux DevTools, VueJS DevTools and Ember Inspector) to double-check that the fixes are not introducing any regression that breaks the existent devtools extensions).

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

[Is the change risky?]: No

[Why is the change risky/not risky?]: The allowed panel urls and icon urls are limited using the API JSON schema file, no new code has been added (besides the additional test case).  

[String changes made/needed]: None
Attachment #8938446 - Flags: approval-mozilla-beta?
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/36585f7098f7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi Florin, can we have some QA's help to verify this?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
Comment on attachment 8938446 [details] [diff] [review]
1-Bug_1425224___Use_manifest_ExtensionURL_in_devtools_panels_create_API_schema__r_mixedpuppy.patch

Fix a sec issue related to devtools. Beta58+.
Attachment #8938446 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've assigned this for verification.
I reproduced this issue on a build before 04.Jan.2018, "My Panel" tab is displayed in "Web Console", once I click on any of the "about: " links nothing happened and sometimes the tab content turns white.
But on a build after 04.Jan.2018 after I temporarily load the unpacked addon in "about:debugging", "My Panel" tab isn't displayed in "Web Console".

Is there any other way that I could verify this bug on the latest nightly?
Thanks.
Flags: needinfo?(lgreco)
Updated flag status-firefox58 by mistake.
The tab content turns white when the particular about page is not loadable in the particular process where the extension is running (and this is an expected behavior of the unfixed version when the extension is running in the extension process, as also briefly described in comment 12).

The test described in comment 19 is enough to test that the fix is in place, we should test it explicitly for both remote (which is the default on Windows systems) and non-remote webextension modes (which is the default on Linux and OSX systems).

It is also worth to make the opposite test (regular devtools extension should not break with the fix in place) as described in Comment 12:

- installing some of the common devtools extensions from AMO (e.g. React/Redux DevTools, VueJS DevTools and Ember Inspector), 
- load a webpage that can be inspected with the target extension (this step is needed because some of these addons do not create the panel for webpages that cannot be inspected with the particular extension):
  - React DevTools => http://todomvc.com/examples/react/#/
  - Redux DevTools => https://zalmoxisus.github.io/examples/todomvc/
  - VueJS DevTools => http://todomvc.com/examples/vue/
  - Ember Inspector => http://todomvc.com/examples/emberjs/
- open the developer tools and double-check that they are behaving as on a "non fixed Firefox version", e.g.:
  - the panel icon is the expected one
  - the extension panel is loaded as expected once it is selected
Flags: needinfo?(lgreco)
restore status-firefox58 state as set in comment 20 (changed by mistake due to mid-air collision of the last two comments added to this bugzilla issue)
Build ID: 20180108220132
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Verified as fixed on Firefox Nightly 59.0a1 on Windows 10 x 64, Mac OS X 10.13 and Ubuntu 16.04 x64.

I can confirm that after I temporarily load the unpacked addon in "about:debugging", "My Panel" tab isn't displayed in "Web Console".
I installed some of the common devtools extensions, loaded the webpage that can be inspected with the target extension and I can confirm that the panel icon is the expected one and the extension panel is loaded as expected once it is selected.
Whiteboard: [adv-main58+]
Alias: CVE-2018-5112
Flags: sec-bounty?
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: toolkit-core-security → core-security-release
Attached image icon differences.png
I reproduced this issue on Nightly 59.0a1(2017-12-14) under Windows 10 x64 using the steps from comment 0.

I retested the issue, by taking in consideration the informations from comment 21, on Beta 59.0b3 under Windows 10 x64, Ubuntu 16.04 x64 and macOS x10.13 with the following results:
  1. After I temporary loaded the unpacked addon in about:debugging, the "My Panel" tab is not displayed in "Web Console".
  2. After I installed some of the common devtools extensions (React Developer Tools, Redux DevTools, VueJS DevTools and Ember Inspector), loaded the webpage that can be inspected with the target extension, the panel icons corresponded with the expected ones (for Redux DevTools and VueJS DevTools) and the extension panel was loaded as expected once it was selected.

Note: It is pretty obvious that the panel icons correspond also for React Developer Tools and Ember Inspector. But I am not sure if it's expected for these addons' icons to not be similar in every location: React Developer Tools (AMO, about:addons page, Web console vs awesome bar) and for the Ember Inspector (AMO, about:addons page vs Web Console) (see attachment).
Flags: needinfo?(lgreco)
Yes: those extensions are explicitly using a different icon for the extension listed in "about:addons", the browserAction and/or devtools panel and so the different icons are part of the expected behavior.
Flags: needinfo?(lgreco)
According to comment 24 and comment 25 I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty+
Product: Toolkit → WebExtensions
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.