Bug 1227462 (CVE-2016-2817)

chrome.tabs.update/create APIs should call checkLoadURI with DISALLOW_INHERIT_PRINCIPAL

VERIFIED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: Muneaki Nishimura, Assigned: rpl, Mentored)

Tracking

({csectype-priv-escalation, dev-doc-complete, sec-moderate})

Trunk
mozilla47
csectype-priv-escalation, dev-doc-complete, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +
sec-bounty +

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ wontfix, firefox46+ verified, firefox47+ verified, firefox48 verified, firefox-esr38 unaffected, firefox-esr45 wontfix)

Details

(Whiteboard: [good first bug][tabs] triaged [adv-main46+])

Attachments

(8 attachments, 28 obsolete attachments)

967 bytes, application/zip
Details
419.23 KB, application/pdf
Details
4.89 KB, patch
kmag
: review+
Details | Diff | Splinter Review
4.78 KB, patch
kmag
: review+
Details | Diff | Splinter Review
6.90 KB, patch
kmag
: review+
Details | Diff | Splinter Review
6.84 KB, patch
kmag
: review+
Details | Diff | Splinter Review
3.70 KB, patch
rpl
: review+
Details | Diff | Splinter Review
8.37 KB, patch
rpl
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Created attachment 8691313 [details]
uxss.xpi

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:

The 'chrome.tabs.update' API allows to navigate tabs to javascript: URLs without any permissions. It can be used for universal XSS attacks to all documents that are opening. It affects not only to ordinary websites but also other extensions. If an other extension'd page is shown in a tab, then script can be  injected from the malicious extension with no permissions, i.e., obtain the target extension's privilege.
Attached is a sample extension that can reproduce the issue.


Actual results:

1. Open arbitrary web sites and extension's page on tabs
2. Click the button of this extension
3. Alert dialog with their current document.location is shown on each tab



Expected results:

The 'chrome.tabs.update' should prohibits navigation to harmful URLs e.g., javascript: and data:. Chrome do so.
(Reporter)

Comment 1

2 years ago
Created attachment 8691314 [details]
Screenshots on my environment

I attached screenshots when the issue reproduced on my PC.

Updated

2 years ago
Component: General → WebExtensions
Flags: needinfo?(amckay)
Product: Core → Toolkit

Updated

2 years ago
Flags: needinfo?(amckay)
Regular add-ons can do this an more already, of course, hopefully bad stuff caught in review. WebExtensions have stricter limits, this is somewhere between sec-moderate and sec-high.
Keywords: csectype-priv-escalation, sec-high
Blocks: 1193837, 1197420
Group: core-security
No longer blocks: 1193837, 1197420
Depends on: 1193837, 1197420
There are probably a few other places we need to do this as well. And it should technically be `checkLoadURIStrWithPrincipal`, but that's not as summary-friendly.
Mentor: kmaglione+bmo@mozilla.com
Summary: Universal XSS in previleged contexts by chrome.tabs.update → chrome.tabs.update/create APIs should call checkLoadURI with DISALLOW_INHERIT_PRINCIPAL
Whiteboard: [good first bug]

Updated

2 years ago
Whiteboard: [good first bug] → [good first bug][tabs]

Updated

2 years ago
Flags: blocking-webextensions?

Comment 4

2 years ago
I'd love to work on this if I could get some more information and what part of the source code is involved here.
Flags: needinfo?(kmaglione+bmo)
For tabs.update and tabs.create, the checks need to happen somewhere near:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#337
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#267

The principal for checkLoadURI should be `context.contentWindow.document.nodePrincipal`. There are a few places we do similar checks in ext-utils.js, if you need examples.
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → ma.steiman
(Reporter)

Comment 6

2 years ago
It seems that an similar issue was fixed on the Firefox 43 and that severity is critical.
https://www.mozilla.org/en-US/security/advisories/mfsa2015-148/
I'm not sure what was the difference between that issue and this, anyway if needed please rate the severity of this bug again.
That issue exposed elevated privileges to web content. This exposes elevated privileges to extension code, which is more trusted than web code, and currently comes with no promises of privilege separation. At the time this bug was reported, we still hadn't implemented permissions checking for `tabs.executeScript`, for instance.

Comment 8

2 years ago
(In reply to Tyler Steiman from comment #4)
> I'd love to work on this if I could get some more information and what part
> of the source code is involved here.

Have you had chance to look at this yet?
Flags: blocking-webextensions? → blocking-webextensions+
Luca, this might be a good bug for you. We have a `checkLoadURL` method in the `context` objects (which are ExtensionPage instances) now.
Assignee: ma.steiman → nobody
(Assignee)

Comment 10

2 years ago
(In reply to Kris Maglione [:kmag] from comment #9)
> Luca, this might be a good bug for you. We have a `checkLoadURL` method in
> the `context` objects (which are ExtensionPage instances) now.

Thanks Kris, I've done a couple of tests about how Chromium behaves in this scenarios and it looks that "javascript:" URLs are checked like javascript evaluation requests on the tabs.update target (mostly like in tabs.executeScript).

tabs.create
- "javascript:" URLs execute successfully in a new "about:blank" tab
- "about:" or "chrome:" URLs do load correctly in a new tab

tabs.update
- "javascript:" URLs
  - logged error: Cannot access a chrome:// URL
  - logged error: Cannot access a chrome-extension:// URL of different extension
  - logged error: Cannot access contents of url "http...". 
                  Extension manifest must request permission to access this host.
- "about:" or "chrome:" URLs do load correctly in the existent tab

checkLoadURL seems to intended to check if a defined URL can be loaded by the current context (the ExtensionPage, but I didn't find any current usage of it) instead of doing the evaluation based on the principal of the target tab.

I'm going to upload a proposed patch which:
- define a new "checkAccessTargetPrincipal" helper in the ExtensionPage prototype
- introduce activeTab and whitelists's MatchPattern behavior in the tabs.update API method
- introduce more tests in the tabs.update test file

In the test cases I have to add a group which tests the error logged by trying accessing 
a tab from a different extension (currently the raise an exception if I try to load a second
extension)
(Assignee)

Comment 11

2 years ago
Created attachment 8706412 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch

This draft patch contains the proposed changes to the ExtensionPage and tabs.update.
Attachment #8706412 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 12

2 years ago
Created attachment 8706414 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-on-javascript-URLs.-r-k.patch

This patch introduces more tests on tabs.update (in particular its behaviour on "javascript:" url applied to different kinds of target tabs)
Attachment #8706414 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8706412 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch

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

I'm not sure we need to do anything this complicated.

The checkLoadURI function is the correct way to check if a principal is allowed to load a URI. It already has special logic for dealing with extension principals, and if that's insufficient, it should be fixed at the platform level.

The principal of the target tab doesn't really matter. If we do the check with the DISALLOW_INHERIT_PRINCIPAL flag, javascript: and data: URLs which inherit the page's principal won't be accepted, no matter what the principal is.

I suppose we could add a special case to allow it when the extension has access to that page, but it doesn't seem necessary. They can already execute scripts via `executeScript`, which already has the appropriate checks in place.

::: browser/components/extensions/ext-tabs.js
@@ +331,5 @@
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        let matchesHost;
> +
> +        TabManager.for(extension).addActiveTabPermission(TabManager.activeTab);

I don't understand why we would do this.

@@ +347,3 @@
>          if (updateProperties.url !== null) {
> +          let updateURI = Services.io.newURI(updateProperties.url, null, null);
> +          let targetPrincipal = tab.linkedBrowser.contentDocument.nodePrincipal;

This won't work under e10s.

@@ +350,5 @@
> +
> +          // TODO: checkAccessTarget should set runtime.lastError
> +          if (updateURI.scheme !== "javascript" ||
> +              context.checkAccessTargetPrincipal(targetPrincipal, matchesHost)) {
> +            tab.linkedBrowser.loadURI(updateProperties.url);

This check isn't safe under e10s. The tab may have loaded a different document by the time we load a URL into it.

::: toolkit/components/extensions/Extension.jsm
@@ +247,5 @@
>    get principal() {
>      return this.contentWindow.document.nodePrincipal;
>    },
>  
> +  checkAccessTargetPrincipal(targetPrincipal, matchesHost) {

I don't think this is necessary.

As far as cross-origin requests go, we already check this at the platform layer.

For load access, we do the same, though it's probably somewhat more restrictive than this check at the moment.
Attachment #8706412 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8706414 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-on-javascript-URLs.-r-k.patch

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

f+ on the general implementation, but I think the behavior we're testing for should be different.

::: browser/components/extensions/test/browser/browser_ext_tabs_update.js
@@ +51,5 @@
> +function* testAccessTarget(url, permissions) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"].concat(permissions),
> +      "webAccessibleResources": [ "tab.html" ],

No spaces around square brackets.

@@ +89,5 @@
> +      });
> +    },
> +  });
> +
> +  let awaitExtensionTabURL = extension.awaitMessage("ready");

There's no need to do this ahead of time. Just use `yield extension.awaitMessage("ready")` when you need it.

@@ +101,5 @@
> +  info(`Testing on tab with URL ${url}`);
> +
> +
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);
> +  gBrowser.selectedTab = tab1;

This isn't necessary. `openNewForegroundTab` already does it.

@@ +109,5 @@
> +
> +  yield BrowserTestUtils.removeTab(tab1);
> +  yield extension.unload();
> +
> +  let errors = Services.console.getMessageArray() || [];

This is problematic for a few reasons. You need to use `SimpleTest.monitorConsole` before you start your test and `endMonitorConsole` when you're done. There are a few other tests that do this.

@@ +125,5 @@
> +  info("Start testing tabs.update on javascript URLs");
> +
> +  let testCases = [{
> +    tabURL: "http://example.net",
> +    extensionSpecialPermissions: ["activeTab"],

The active tab permission should only grant permission for the active tab after the user has interacted with it (e.g., by clicking the extension's toolbar button), so it shouldn't apply here.
Attachment #8706414 - Flags: feedback?(kmaglione+bmo) → feedback+
(In reply to Luca Greco [:rpl] from comment #10)
> (In reply to Kris Maglione [:kmag] from comment #9)
> > Luca, this might be a good bug for you. We have a `checkLoadURL` method in
> > the `context` objects (which are ExtensionPage instances) now.
> 
> Thanks Kris, I've done a couple of tests about how Chromium behaves in this
> scenarios and it looks that "javascript:" URLs are checked like javascript
> evaluation requests on the tabs.update target (mostly like in
> tabs.executeScript).


This comparison should include tests for data: URLs as their behavior is very different between Chrome and Firefox (i.e., data: inherits context in Firefox, but gets a blank context in Chrome)
(Assignee)

Comment 16

2 years ago
(In reply to Kris Maglione [:kmag] from comment #13)
> Comment on attachment 8706412 [details] [diff] [review]
> 0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch
> 
> Review of attachment 8706412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure we need to do anything this complicated.
> 
> The checkLoadURI function is the correct way to check if a principal is
> allowed to load a URI. It already has special logic for dealing with
> extension principals, and if that's insufficient, it should be fixed at the
> platform level.
> 
> The principal of the target tab doesn't really matter. If we do the check
> with the DISALLOW_INHERIT_PRINCIPAL flag, javascript: and data: URLs which
> inherit the page's principal won't be accepted, no matter what the principal
> is.
> 
> I suppose we could add a special case to allow it when the extension has
> access to that page, but it doesn't seem necessary. They can already execute
> scripts via `executeScript`, which already has the appropriate checks in
> place.

I agree, as tabs.executeScript is a better and more explicit way to execute javascript code in the context of a target tab window, I think we can be more restrictive on what kind of url we can load in a tab using tabs.update (and put a note about this in the "WebExtensions - Chrome incompatibilities" MDN doc page).

I'm going to rewrite the patch to simply use the checkLoadURL to test the url and change the tests accordingly (e.g. the different expected error messages, and use different combinations of existent tab url and url to be loaded using tabs.update, like "about:", regular webpages url, "javascript:" and "data:" url, as suggested by Frederik)
(Assignee)

Comment 17

2 years ago
Created attachment 8707641 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

This patch is an update version of the Attachment 8706412 [details] [diff] which implements the more restricted tabs.update.

This new "tabs.update" version may load into a target tab:

- regular web pages URLs
- non privileged "about:" URLs (e.g. about:blank)
- pages from the same extension

it cannot load into a target tab:
- privileged "about:" URLs (e.g. about:config)
- javascript URLs
- data URLs
Assignee: nobody → lgreco
Attachment #8706412 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8707641 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 18

2 years ago
Created attachment 8707651 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

This patch is a rework of the new test from Attachment 0706414 [details] [diff]

It checks which kind of URL can be loaded in different types of existing tabs and when it will log an error
(and it checks that successes and failures only depend from the URL that is going to be loaded, and do not depend from the url that is currently loaded in the target tab)
Attachment #8706414 - Attachment is obsolete: true
Attachment #8707651 - Flags: review?(kmaglione+bmo)
Comment on attachment 8707641 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

We need to do this for tabs.create as well.
Attachment #8707641 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8707651 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

Let's move this to its own test file. It's probably going to trigger timeouts on debug builds if it runs with the other update tests.

::: browser/components/extensions/test/browser/browser_ext_tabs_update.js
@@ +52,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },
> +      "permissions": ["tabs"],
> +      "webAccessibleResources": [ "tab.html" ],

No spaces inside of square brackets.

@@ +78,5 @@
> +          for (let tab of tabs.slice(1)) {
> +            updates.push(new Promise((resolve, reject) => {
> +              browser.tabs.update(tab.id, {
> +                url: tabsUpdateURL,
> +              }, resolve);

We're only intending to update one tab, right?

@@ +178,5 @@
> +          .map((check) => Object.assign({}, check, { existentTabURL }))
> +          .filter(el => el.tabsUpdateURL != el.existentTabURL);
> +
> +    return acc.concat(testSet);
> +  }, []);

I don't think we need to do this anymore. We're not checking what's currently loaded into the tab, so it shouldn't make a difference.
Attachment #8707651 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 21

2 years ago
Created attachment 8709380 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

Same patch as Attachment 8707641 [details] [diff] (just rebased)
Attachment #8707641 - Attachment is obsolete: true
Attachment #8709380 - Flags: review+
(Assignee)

Comment 22

2 years ago
Created attachment 8709382 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Updated test case for the new tabs.update behavior:

- split the test case in a separated test file
- simplified as suggested in Comment 20
Attachment #8707651 - Attachment is obsolete: true
Attachment #8709382 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 23

2 years ago
Created attachment 8709383 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

This patch introduces the checkLoadURL on tabs.create as well.
Attachment #8709383 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 24

2 years ago
Created attachment 8709385 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

This patch introduces a new test file which tests the new tabs.create behaviour.
Attachment #8709385 - Flags: review?(kmaglione+bmo)
Comment on attachment 8709382 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +47,5 @@
> +
> +  info(`tab.update URL "${tabsUpdateURL}" on tab with URL "${existentTabURL}"`);
> +
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, existentTabURL);
> +  gBrowser.selectedTab = tab1;

|openNewForegroundTab| should take care of this.

@@ +52,5 @@
> +
> +  extension.sendMessage("start", tabsUpdateURL);
> +  yield extension.awaitMessage("done");
> +
> +  yield BrowserTestUtils.removeTab(tab1);

It would be good to check the final URL of the tab before we remove it.

@@ +57,5 @@
> +  yield extension.unload();
> +
> +  let errors = Services.console.getMessageArray() || [];
> +
> +  Services.console.reset();

I mentioned this before: We should really use |SimpleTest.monitorConsole| and |SimpleTest.endMonitorConsole| for this.

Console messages are handled asynchronously, so unless you take special precautions, there's a race here. Clearing the console is also not really good test etiquette.
Attachment #8709382 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8709383 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +264,5 @@
>          let url = createProperties.url || aboutNewTabService.newTabURL;
>          url = context.uri.resolve(url);
>  
> +        if (!context.checkLoadURL(url)) {
> +          Cu.reportError(`Permission denied on tabs.create with url "${url}"`);

Let's skip this for now. checkLoadURI already reports the error. We can add more specific reporting when we add lastError support.
Attachment #8709383 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8709385 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

r=me with the same caveats as the tabs.update tests.

::: browser/components/extensions/test/browser/browser_ext_tabs_create_url.js
@@ +31,5 @@
> +        browser.tabs.create({ url: tabsCreateURL }, (tab) => {
> +          if (tab) {
> +            browser.tabs.remove(tab.id);
> +          }
> +          browser.test.sendMessage("done");

Please check that the tab is undefined when we're expecting an error.
Attachment #8709385 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 28

2 years ago
Created attachment 8710003 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

To cover some of the review comments on the test cases, I had to apply some of the changes on this first patch (and I reworked the method to be
more readable and to better reproduce some of the expected behaviour, based on more tests on other Chromium based browsers): 

- removed Cu.reportError (I reported the error there mainly to prevents them to become new silent errors sources, but the runtime.lastError is not so far so, I agree and prefer to leave a comment in the mean time and then update the test case to check it in the callbacks, and remove completely any console service usage in the tests)

- reworked tabs.update to ensure to better reproduce some details of the API behaviour on Chromium:

  - the callback tab parameter should be null when the request is totally invalid

  - the URL in the callback tab parameter should be the requested one 
    (Chromium don't care if the loading is not yet started or completed, 
    on the contrary the other tab fields, e.g. the title, are unmodified)

The new version of this patch introduces a small tweak in ext-utils.js:
    
- small tweak on TabManager.convert(...) to ensure it accepts and returns 
  undefined when its tab parameter is undefined or null

NOTE: added f? due to the reworked tab.update method and the small change to ext-utils.js
Attachment #8709380 - Attachment is obsolete: true
Attachment #8710003 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 29

2 years ago
Created attachment 8710004 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

This new version introduces the suggested changes, the result is a simplified and cleaner version of the new test file introduced by this patch.

- removed any Service.console usage (it is definitely better to check the runtime.lastError in this test case, once it lands and the tabs API methods support it)
    
- removed gBrowser.selectedTab (ouch! sorry, I forgot to removed it from this patch)

- test the tab in the tabs.update callback:
  - on error, tab should be null 
  - on success, tab should be defined and the tab.url should be url in the tabs.update request
Attachment #8709382 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
Created attachment 8710008 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

- fixed bug (aboutNewTabService.newTabURL is usually an "about:" or a "chrome:" URL and it will not pass checkLoadURL)

- fixed nit, moved the "TODO: runtime.lastError" out of the "if (callback)" branch

- minor rework of the tabs.create method, which better reproduces some details of the chrome behavior:

  - the URL in the tab Details should be pre-populated with the requested URL,
  - the callback tab parameter should be undefined when the request is invalid

NOTE: added f? due to the rework on the tabs.create method and the fix applied to do not break the "aboutNewTabService.newTabURL" scenario
Attachment #8709383 - Attachment is obsolete: true
Attachment #8710008 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 31

2 years ago
Created attachment 8710011 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

This new version of the last patch introduces the suggested changes, plus the same simplifications and clean up introduced in the last version of the other test file (the tabs.update test file from Attachment 8710004 [details] [diff]).
Attachment #8709385 - Attachment is obsolete: true
Comment on attachment 8710003 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

Let's save this for a separate bug. Most of it isn't especially related to the issue here.

(In reply to Luca Greco [:rpl] from comment #28)
>   - the URL in the callback tab parameter should be the requested one 
>     (Chromium don't care if the loading is not yet started or completed, 
>     on the contrary the other tab fields, e.g. the title, are unmodified)

I'm on the fence about this. I've run into it before, but I'm not sure the
solution is to special case the URL in the callback arguments. Ideally we
should maintain some kind of internal state so queries also return the updated
URL, but that requires clearing it when we know the load failed.

Can you file a separate bug, and we can discuss it further there?

::: browser/components/extensions/ext-tabs.js
@@ +325,5 @@
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        // An update is valid if at least one of the supported updateProperties is valid
> +        // and the request is going to be handled.
> +        let isValidTabUpdate = false;

I don't think emulating this behavior is necessary. It's not documented, and it doesn't seem useful enough to justify the added complexity.

::: browser/components/extensions/ext-utils.js
@@ +421,5 @@
>    },
>  
>    convert(tab) {
> +    if (!tab) {
> +      return undefined;

I'd rather not do this if we can avoid it. If there are places where we're expecting the tab to be null, we can do |tab && tabManager.convert(...)|.
Attachment #8710003 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8710004 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

f+, but let's save the callback URL checks for a follow-up, if we decide to make those changes.

In the mean time, let's just use an onUpdated observer. You can steal the helper function I added here, if it helps: https://hg.mozilla.org/integration/fx-team/file/8fc9a5eeede3/browser/components/extensions/test/browser/browser_ext_pageAction_context.js#l180
Attachment #8710004 - Flags: feedback+
Comment on attachment 8710008 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

Let's also save this for a follow-up.

::: browser/components/extensions/ext-tabs.js
@@ +265,5 @@
>          url = context.uri.resolve(url);
>  
> +        // Skip checkLoadURL if the URL has been populated from the aboutNewTabService,
> +        // (it is usually an "about:" or "chrome:" URL).
> +        if (url != aboutNewTabService.newTabURL && !context.checkLoadURL(url)) {

I'd rather not special case the new tab URL. Extensions can already load it by simply not passing a URL. If we decide to add a separate special case for it, it should be on the platform side rather than here.
Attachment #8710008 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8710011 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

Same as the other patch. f+, but let's save the URL property checks for a separate bug.
Attachment #8710011 - Flags: feedback+
(Assignee)

Comment 36

2 years ago
Created attachment 8710579 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

Updated the patch with the following changes:

- removed the change on ext-utils.js

- changed the detection of invalid tabs.update requests
  (I think that we have to handle this new scenario, 
  the tabs.update apparently can only fail on invalid tabId on Chromium,
  so this is a Firefox special case, and I think that returning 
  the tab details to the callback when we have basically discarded
  the request can be misleading)
Attachment #8710003 - Attachment is obsolete: true
Attachment #8710579 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 37

2 years ago
Created attachment 8710580 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Updated "tabs.update test case" patch:

- removed assertion on the tab.url (which will be discussed and eventually fixed in a follow up)
Attachment #8710004 - Attachment is obsolete: true
Attachment #8710580 - Flags: review+
(Assignee)

Comment 38

2 years ago
Created attachment 8710588 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

Updated patch on tabs.create:

- cleaned up the tabs.create changes

I had to handle the empty url differently because on empty URLs:

- context.uri.resolve will resolve it as an URL relative to the
  context base URI
- gBrowser.addTab will open a tab on "about:blank", instead of opening
  it on the expected URL

Currently, if we do not use aboutNewTabService.newTabURL, we break a number
of test cases that are counting on this.

Nevertheless, I think that the current implementation is wrong because,
I think it is supposed to behave like the keyboard shortcut or the menu item
that let the user opening a new tab, and:

- the "chrome://..." URL is visible in the location bar (and it shouldn't)
- on a private window it should show "about:privatebrowsing" instead of
  "about:newtab"

I've some ideas (and a preliminary patch for it, I need to complete to fix the test cases),
but it would be better to move the discussion in a separate bugzilla issue as a follow-up.
Attachment #8710008 - Attachment is obsolete: true
Attachment #8710588 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 39

2 years ago
Created attachment 8710594 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

Updated tabs.create test:

- removed all the kind of URLs which are already tested by the other tabs.create test file (now it tests only invalid URLs)

Maybe I should rename the file accordingly.
Attachment #8710011 - Attachment is obsolete: true
Attachment #8710594 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8710580 - Flags: review+ → feedback+
(Assignee)

Comment 40

2 years ago
Created attachment 8710597 [details] [diff] [review]
0005-Bug-1227462-fix-tabs.onUpdated-test-case.-r-kmag.patch

Small change on the test file for tabs.onUpdated, which cannot use anymore "about:preferences" as a valid url for the tabs.update method call
that is used to generate the update events.
Attachment #8710597 - Flags: review?(kmaglione+bmo)
Comment on attachment 8710579 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +323,5 @@
>        update: function(tabId, updateProperties, callback) {
>          let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        if (tab) {

Let's just return early here if there's no matching tab.

@@ +353,5 @@
> +              let keyIsNotURL = (k) => k !== "url";
> +              let valueIsNotNull = (k) => updateProperties[k] !== null;
> +              let otherUpdatedProperties = Object.keys(updateProperties)
> +                                                 .filter(keyIsNotURL)
> +                                                 .some(valueIsNotNull);

I don't think there's any reason to try to exactly mimic Chrome's behavior here. Let's just check this first. If the URL is illegal, we should return immediately with an error, rather than trying to handle cases where we update other properties but then reject the URL.
Attachment #8710579 - Flags: review?(kmaglione+bmo)
Comment on attachment 8710580 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +4,5 @@
> +
> +function* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },

This line is unnecessary (and should probably be an error).

@@ +31,5 @@
> +            if (isErrorExpected) {
> +              // TODO: check runtime.lastError once supported
> +              browser.test.assertEq(undefined, tab, "on error tab should be undefined");
> +            } else {
> +              browser.test.assertTrue(!!tab, "on success the tab should be defined");

No need for the !!
Comment on attachment 8710588 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +278,4 @@
>  
>          function createInWindow(window) {
> +          if (!url) {
> +            url = aboutNewTabService.newTabURL;

Let's just set this in the `else` of `if (createProperties.url)`

@@ +299,5 @@
>              window.gBrowser.pinTab(tab);
>            }
>  
>            if (callback) {
> +            runSafe(context, callback, tab && TabManager.convert(extension, tab));

`tab` should never be null here, so let's skip the `tab && `
Attachment #8710588 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8710594 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create_url.js
@@ +4,5 @@
> +
> +function* testTabsCreateInvalidURL(tabsCreateURL) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },

This isn't necessary.
Attachment #8710594 - Flags: feedback?(kmaglione+bmo) → feedback+
Attachment #8710597 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 45

2 years ago
Created attachment 8710972 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

First patch of the queue, updated with the suggested tweaks:

- return early if there isn't any matching tab
- return early if the URL is invalid (even if it isn't the only property that the add-on wants to update)

In both the above cases the callback |tab| parameter will be |undefined|.

Thanks a lot Kris for your feedback on this:

this "invalid URL" scenario is "Firefox"-specific, so I wasn't really try to mimic any particular Chrome behaviour, but I wanted to find a behaviour for Firefox which could be reasonable (and I think that the last suggested behaviour is pretty good and that it will not be misleading)
Attachment #8710579 - Attachment is obsolete: true
(Assignee)

Comment 46

2 years ago
Created attachment 8710973 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Fixed the two small nits on the new tabs.update test file.
Attachment #8710580 - Attachment is obsolete: true
Attachment #8710973 - Flags: feedback+
(Assignee)

Comment 47

2 years ago
Created attachment 8710975 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

Updated the patch on tabs.create, applying the following suggested tweaks:

- set the "newtab" fallback url in the else branch
- remove the |tab && ...| because tab should never be null

Set the previous r+ flag.
Attachment #8710588 - Attachment is obsolete: true
Attachment #8710975 - Flags: review+
(Assignee)

Comment 48

2 years ago
Created attachment 8710976 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

Fixed the nit on the new tabs.create test file, and renamed it to "browser_ext_tabs_create_invalid_url.js".

Set the previous f+ flag.
Attachment #8710594 - Attachment is obsolete: true
Attachment #8710976 - Flags: feedback+

Updated

2 years ago
Priority: -- → P1
Whiteboard: [good first bug][tabs] → [good first bug][tabs] triaged
(Assignee)

Comment 49

2 years ago
Created attachment 8717298 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch
Attachment #8710972 - Attachment is obsolete: true
Attachment #8710975 - Attachment is obsolete: true
(Assignee)

Comment 50

2 years ago
Created attachment 8717299 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch
Attachment #8710976 - Attachment is obsolete: true
Attachment #8710597 - Attachment is obsolete: true
Attachment #8710973 - Attachment is obsolete: true
(Assignee)

Comment 51

2 years ago
Comment on attachment 8717298 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

This patch is a rebased and updated version of the past main two patches attached to this issue:

- squashed the two patches for tabs.update and tabs.create in a single patch
- ported to the new Promise based API

- checkLoadURL:
  - the new |dontReportErrors| option prevents it from logging into the console errors which we are already reporting using a rejected promise / runtime.lastError

- tabs.create
  - checks the request url, if specified, and do not open a new tab if the url 
    is not allowed (and return a rejected Promise to set the lastError accordingly)
  - preserved the changes introduce by Bug 1242588 (new tab url if url is missing)

- tabs.update
  - rejects and set lastError if no tab is found with the requested tabId
  - rejects and set lastError if the requested url is not allowed
  - the requested url is resolved using context.uri.resolved (as tabs.create already does)
Attachment #8717298 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 52

2 years ago
Comment on attachment 8717299 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Updated version of the patches with the new two test cases "browser_ext_tabs_create_invalid_url.js" and "browser_ext_tabs_update_url.js" (ported to the new Promise based API, added assertion related to the Promise rejected error message) and the small tweak on the "browser_ext_tabs_onUpdated.js" test.
Attachment #8717299 - Flags: review?(kmaglione+bmo)
Comment on attachment 8717298 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +293,5 @@
>  
> +          let url;
> +
> +          if (createProperties.url !== null) {
> +            url = context.uri.resolve(createProperties.url);

I'd rather keep this check in the createInWindow function. I know we technically don't need to wait for the window to load before checking if the URL is legal, but I'd find it easier to follow if all of URL processing was in one place.
Attachment #8717298 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8717299 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js
@@ +41,5 @@
> +add_task(function* () {
> +  info("Start testing tabs.create on invalid URLs");
> +
> +  let dataURLPage = `
> +    data:text/html,<!DOCTYPE html>

Please make sure to trim the leading whitespace from this.

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +5,5 @@
> +function* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"],
> +      "webAccessibleResources": ["tab.html"],

This shouldn't be necessary. Also, if it is necessary it needs to be "web_accessible_resources".

@@ +25,5 @@
> +    background: function() {
> +      browser.test.sendMessage("ready", browser.runtime.getURL("tab.html"));
> +
> +      browser.test.onMessage.addListener((msg, tabsUpdateURL, isErrorExpected) => {
> +        let onTabsUpdated = (tab) => isErrorExpected ?

Please use if-else statements for these.
Attachment #8717299 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 55

2 years ago
Created attachment 8718004 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Updated patch (applied changes based on the last review comments)
Attachment #8717298 - Attachment is obsolete: true
Attachment #8718004 - Flags: review+
(Assignee)

Comment 56

2 years ago
Created attachment 8718005 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Updated patch (applied minor changes based on the last review comments)
Attachment #8717299 - Attachment is obsolete: true
Attachment #8718005 - Flags: review+
(Assignee)

Comment 57

2 years ago
Created attachment 8721380 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Patch rebased, previous review flag state set.
Attachment #8718004 - Attachment is obsolete: true
Attachment #8721380 - Flags: review+
(Assignee)

Comment 58

2 years ago
Created attachment 8721381 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Patch rebased, previous review flag state set.
Attachment #8718005 - Attachment is obsolete: true
Attachment #8721381 - Flags: review+
(Assignee)

Comment 59

2 years ago
Comment on attachment 8721380 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Any webextension addon can trivially exploit this issue to gain access to
   higher privileges provided by tabs with higher privileged origins, 
   by targeting them using chrome.tabs.create/chrome.tabs.update with 
   "data:" or "javascript:" URLs.
   
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   
   The attached xpi file, the new tests included and the early comments 
   describe the issue with enough details:

   In the initial version of the webextension sandbox mechanisms,
   the chrome.tabs.create and chrome.tabs.update API methods
   didn't check the URL which the addon wants to load in a new or
   existent tab.

   "data:" and "javascript:" URLs inherit the existent principal,
   which means that the addon will be able to run arbitrary javascript code
   with the privileges of the principal currently associated with the document
   loaded in the tab (and some of the tabs could have higher privileges than the
   webextension addons, e.g. about:addons, "about:config")

Which older supported branches are affected by this flaw?

   Firefox >= 42, because this issue has been introduced in the initial import 
   of the WebExtension implementation.

   it is worth to mention that the new security sandboxing for WebExtensions
   was still in a prototype stage in those versions, and this level of sandbox
   restriction isn't enforced on no one of the other existent extensions types.

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

   N/A

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

   Backport this fix is possible but a rewrite is needed, because some of the
   internals are changed over the time.

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

   This patch can only affect addons based on webextensions APIs, 

   we have added new test cases to test the new restrictions and fixed tests 
   that were based on wrong assumptions.

   It's still possible that this would also cause problems for webextensions 
   which might depend on being able to load data: or javascript: URLs, 
   which they may not be doing for malicious reasons (especially those who
   have been ported from Chrome which allows the use of this kind of URLs).
Attachment #8721380 - Flags: sec-approval?
Comment on attachment 8721380 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

sec-approval+.

We should take this on Aurora and Beta. Please make and/or nominate patches today.
Attachment #8721380 - Flags: sec-approval? → sec-approval+
status-firefox44: --- → wontfix
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → affected
tracking-firefox45: --- → +
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Keywords: sec-high → sec-moderate
(Assignee)

Comment 61

2 years ago
Created attachment 8721493 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Backport to Aurora of the changes needed to fix this issue (and an existent test that needs to be fixed once the fix is applied).

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9050889c35b
Attachment #8721493 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 62

2 years ago
Created attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

Backport to Beta of the changes needed to fix this issue (and an existent test that needs to be fixed once the fix is applied).

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed65761212cc
Attachment #8721494 - Flags: review?(kmaglione+bmo)
Attachment #8721493 - Flags: review?(kmaglione+bmo) → review+
Attachment #8721494 - Flags: review?(kmaglione+bmo) → review+
Luca, do your tests apply cleanly to beta and aurora?
Flags: needinfo?(lgreco)
Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

Approval Request Comment
See comment 59.

[String/UUID change made/needed]:

None.
Attachment #8721494 - Flags: sec-approval?
Attachment #8721494 - Flags: approval-mozilla-beta?
Comment on attachment 8721493 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Approval Request Comment
See comment 59.

[String/UUID change made/needed]:

None.
Attachment #8721493 - Flags: approval-mozilla-aurora?
Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

sec-approval is only needed for trunk.
Attachment #8721494 - Flags: sec-approval?
Sorry, I thought I'd cleared that flag.
(Assignee)

Comment 68

2 years ago
Created attachment 8721711 [details] [diff] [review]
Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Backport to Aurora of new test cases which explicitly tests the allowed and disallowed URLS.

Unfortunately the original patch couldn't be used like it was because in Aurora the WebExtensions APIs do not support lastError and the Promise-based behaviour.

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e428bf9654
Attachment #8721711 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 69

2 years ago
Comment on attachment 8721711 [details] [diff] [review]
Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Wrong patch attached by mistake, set as obsolete.
Attachment #8721711 - Attachment is obsolete: true
Attachment #8721711 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 70

2 years ago
Created attachment 8721713 [details] [diff] [review]
Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Backport of the new test cases on Aurora.

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ec9328bfb8
Flags: needinfo?(lgreco)
Attachment #8721713 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 71

2 years ago
Created attachment 8721714 [details] [diff] [review]
Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Beta__r_kmag.patch

Backport of the new tests on Beta (the only difference between this patch and the patch applied on Aurora is related to the changes on the browser.ini file).

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42d3c66e6d9
Attachment #8721714 - Flags: review?(kmaglione+bmo)
Attachment #8721714 - Flags: review?(kmaglione+bmo) → review+
Attachment #8721713 - Flags: review?(kmaglione+bmo) → review+
For now, I am not planning to take this uplift:
* not landed in m-c yet (on purpose? in this case 47 should not be marked as affected)
* I don't have the risk evaluation
* it is a sec moderate
(Assignee)

Comment 73

2 years ago
Created attachment 8723632 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Unfortunately I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of newly added eslint rules.

Re-applied the previous r+ flag, 'cause it differs only in the fixed eslint errors (unneeded spaces inside object's curly brackets).
Attachment #8721380 - Attachment is obsolete: true
Attachment #8723632 - Flags: review+
(Assignee)

Comment 74

2 years ago
Created attachment 8723633 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Unfortunately, as for the previous one, I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of the newly added eslint rules.

Re-applied the previous r+ flag, 'cause it differs only in the fixed eslint errors (unneeded spaces inside object's curly brackets).
Attachment #8721381 - Attachment is obsolete: true
Attachment #8723633 - Flags: review+
I didn't realize this hadn't landed on m-c yet.

Luca, is there a reason you haven't requested checkin yet?
Flags: needinfo?(lgreco)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Whiteboard: [good first bug][tabs] triaged → [good first bug][tabs] triaged [checkin needed for attachments 8723632, 8723633]
(Assignee)

Comment 76

2 years ago
(In reply to Kris Maglione [:kmag] from comment #75)
> I didn't realize this hadn't landed on m-c yet.
> 
> Luca, is there a reason you haven't requested checkin yet?

Fixed eslint errors in the patches that will be merged in m-c (and added in the whiteboard comment about which attachments should to land on m-c)
Flags: needinfo?(lgreco)

Comment 78

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc5fbcbece94
https://hg.mozilla.org/mozilla-central/rev/5efb021d57d9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

I am sorry but this is too late. This is RC week and we only take critical fixes.
Attachment #8721494 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox45: affected → wontfix
Comment on attachment 8721493 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Fix for regression from 42. May risk regressions in web extensions. We are a few days away from 46 moving to beta, so we should keep an eye on this in testing.
Attachment #8721493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 81

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/09ca754e5fb7
status-firefox46: affected → fixed
Keywords: dev-doc-needed
Flags: sec-bounty?
Flags: sec-bounty?
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 83

2 years ago
(In reply to Will Bamberg [:wbamberg] from comment #82)
> docs:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Chrome_incompatibilities#tabs
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
> 
> does this cover it, Luca?

Yes, I think that the changes in the docs cover this Chrome incompatibility, Thanks Will!

If you think that it could be an useful addition: starting from Firefox 47 the chrome.runtime.lastError is set when an privileged URL is used, with `URL not allowed: ${url}` as the error message,  and, if the addon doesn't check it in the callback, then it will be logged to the console (which are currently visible in the Browser Console).
Flags: needinfo?(lgreco)
This is a good call! I've not described the exact error message, as I'd worry that might be too volatile, but I've noted this in:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
...and also beefed up the lastError docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Runtime/lastError to be more helpful.
Keywords: dev-doc-needed → dev-doc-complete
I was able to reproduce the initial issue mentioned in Description on Firefox 44.0a1 (2015-09-22) under Windows 10 64-bit.

Manually tested using the uxss.xpi on Firefox 48.0a1 (2016-03-29), Firefox 47.0a2 (2016-03-29) and Firefox 46 beta 6 under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11 and noticed the following:
- The alert dialog is no longer displayed across all Firefox versions
- A browser console error is thrown on Firefox 47.0a2 and Firefox 48.0a1 while clicking on the webextension’s icon:     
Unchecked lastError value: Error: URL not allowed: javascript:alert(document.location);   ExtensionUtils.jsm:240


Luca,any thoughts about this error? Could you please confirm that this is the intended behaviour?
Flags: needinfo?(lgreco)
(Assignee)

Comment 86

2 years ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #85)
> I was able to reproduce the initial issue mentioned in Description on
> Firefox 44.0a1 (2015-09-22) under Windows 10 64-bit.
> 
> Manually tested using the uxss.xpi on Firefox 48.0a1 (2016-03-29), Firefox
> 47.0a2 (2016-03-29) and Firefox 46 beta 6 under Windows 10 64-bit, Ubuntu
> 12.04 32-bit and Mac OS X 10.11 and noticed the following:
> - The alert dialog is no longer displayed across all Firefox versions
> - A browser console error is thrown on Firefox 47.0a2 and Firefox 48.0a1
> while clicking on the webextension’s icon:     
> Unchecked lastError value: Error: URL not allowed:
> javascript:alert(document.location);   ExtensionUtils.jsm:240
>  
> Luca,any thoughts about this error? Could you please confirm that this is
> the intended behaviour?

Sure, intended behaviour confirmed.

This behavior is now documented in the Chrome incompatibilities:

- https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#tabs

In Chromium-based browsers some of these URLs are currently allowed, and so, starting from Firefox 47 where runtime.lastError has been introduced, an Error message is logged in the browser console if not explicitly checked in the addon source code, as now documented on MDN:

-  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Runtime/lastError
Flags: needinfo?(lgreco)
Thanks Luca!

Based on Comment 85 and Comment 86, I am marking this bug as Verified.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox47: fixed → verified
status-firefox48: --- → verified
status-firefox-esr45: affected → wontfix
Whiteboard: [good first bug][tabs] triaged [checkin needed for attachments 8723632, 8723633] → [good first bug][tabs] triaged [checkin needed for attachments 8723632, 8723633][adv-main46+]
Alias: CVE-2016-2817
Whiteboard: [good first bug][tabs] triaged [checkin needed for attachments 8723632, 8723633][adv-main46+] → [good first bug][tabs] triaged [adv-main46+]
You need to log in before you can comment on or make changes to this bug.