Cross-origin access for content scripts isn't working as expected

VERIFIED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: wbamberg, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 verified, firefox48 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8662671 [details]
co.xpi

According to the Chrome docs, I think, content scripts should get the same cross-origin privileges that their parent extension has.

I've attached a WebExtension.

It adds a button. When you're on a page under https://developer.mozilla.org/, if you click the button, it injects a content script. The content script fetches content from https://example.com/ and replaces the page with the fetched content.

The WebExtension has this in its manifest.json:

  "permissions": ["https://developer.mozilla.org/*", "https://example.com/"],

With these permissions, I think it ought to work, and it does on Chrome. But on Firefox it doesn't, and I get a "Cross-Origin Request Blocked" error in the console:

Updated

2 years ago
Blocks: 1214433
Priority: -- → P1

Updated

2 years ago
Blocks: 1161828
This should probably apply to cross-compartment DOM access too. It looks like Chrome doesn't support that, though, so this is simpler than I thought.

It seems we already have the cross-origin security checks required for this (bug 1180921), so this is just a matter of giving content scripts an XMLHttpRequest that's bound to their principal, rather than the principal of the content document.

Provisionally taking this, since it looks pretty doable now.
Assignee: nobody → kmaglione+bmo
Actually, I take some of that back. We currently run content scripts in a sandbox with the principal of the content window, which makes this a bit hairier.
(Assignee)

Comment 3

2 years ago
We actually designed for this to work, but I forgot to implement one piece of it. We need to make sure that the expanded principal that the sandbox gets has a principal with the addonId OriginAttribute set. Bug 1182357 adds a facility to create such a principal. We just need to use it in ExtensionContent.jsm when creating our sandbox.

Still not sure this will all work as expected though :-).
Oh, good, that saves me the trouble of suggesting it. In that case, I guess someone should file a bug for finishing adding expanded principals to content scripts (I'm probably not the best person) that blocks this.

Once the scripts have the proper extension principal, we can just inject XMLHttpRequest bindings from a compartment bound to that principal without resorting to any major hacks.

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 5

2 years ago
Created attachment 8695102 [details] [diff] [review]
patch

I started working on that whiteListedHosts bug I mentioned in the review yesterday and it kinda snowballed into this.
Assignee: kmaglione+bmo → wmccloskey
Status: NEW → ASSIGNED
Attachment #8695102 - Flags: review?(kmaglione+bmo)
Comment on attachment 8695102 [details] [diff] [review]
patch

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +222,2 @@
>      // Make sure we don't hand out the system principal by accident.
>      prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);

At some point, I think we're going to need to do a check for other kinds of privileged principals, too. Chrome doesn't let add-ons overlay other add-ons without special permissions (there are also privilege escalation issues there), for instance.

@@ +222,5 @@
>      // Make sure we don't hand out the system principal by accident.
>      prin = Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);
> +  } else {
> +    let extensionPrincipal = ssm.createCodebasePrincipal(this.extension.baseURI, {addonId: extensionId});
> +    prin = ssm.createExpandedPrincipal([contentPrincipal, extensionPrincipal]);

Is there a reason we need to create the expanded principal explicitly rather than passing the array to |Cu.Sandbox|? The sandbox code also checks for system principals when creating expanded principals, so that might let us skip extracting the content principal and the security checks, too.
Attachment #8695102 - Flags: review?(kmaglione+bmo) → review+
Actually, on second thought, can you add a test to check that scripts in the content page don't wind up with elevated privileges, and can't access the sandbox's XMLHttpRequest constructor if it somehow gets leaked to them?

I know it's probably a bit paranoid, but that would be a massive security hole if it ever happened.

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a967e08dff7

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a967e08dff7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

2 years ago
Duplicate of this bug: 1235543
I was able to reproduce the initial issue on Firefox 43.0a1 (2015-08-22) under Windows 10 64-bit.

Verified fixed on Firefox 48.0a1 (2016-03-22) and Firefox 46.0b4 (20160322075646) using Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 12.04 32-bit.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox48: --- → verified
You need to log in before you can comment on or make changes to this bug.