Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

Summary:

All content injected into web content pages is currently subject to the
same Content Security Policy, regardless of who injected it. For
privileged callers, such as extension content scripts, this means that
some functionality can behave erratically, depending on the page they're
running on.

The plan here is to apply a separate CSP to content injected by certain
privileged callers, rather than subjecting it to page CSP. Content from
system URLs (like moz-extension:) is already immune to CSP. This change
will extend that immunity to any content injected by those callers.


Implementation plan:

Implementing this will require that we record the caller principal
whenever content that loads a URL is injected into a page, and use that
principal as the triggering principal for the load.

My plan is to add a special attribute to principals, specifying that
their CSP should override the CSP of document principals that they
subsume. Initially, this flag would only apply to expanded principals
used in extension content scripts.

The exact mechanics of capturing the triggering principal will need to
vary, but in general, it should happen any time a particular source
attribute is set. For instance, the following should all capture the
caller principal for the `src` URL at call time:

document.write(`<img src="http://example.com/favicon.ico">`);
div.innerHTML = `<img src="http://example.com/favicon.ico">`;
img.setAttribute("src", "http://example.com/favicon.ico");
img.src = "http://example.com/favicon.ico";

Other alterations to the node should retain the existing principals for
existing attributes. For example:

img.setAttribute("src", "http://example.com/favicon.ico");

window.wrappedJSObject.img = img;
window.wrappedJSObject.eval("document.body.appendChild(img)");

should retain the principal from the original setAttribute call, even
though the last insertion happens with a content caller principal.


Scripts injected in this way will not gain any special immunity to CSP.
For instance, this call will succeed regardless of site CSP:

document.createElement("script");
script.setAttribute("src", "https://cdn.framework.org/framework.js");
document.head.appendElement(script);

but any attempts by that script to inject content that violates CSP will
fail, as will any attempts to inject content via that framework
(e.g., `window.wrappedJSObject.$("head").append("<script>")`).


Security & privacy concerns:

This change will allow extensions to inject content into sites which can
(and probably will) cause security and privacy issues. However, it's
already quite easy for malicious or badly-implemented extensions to
create similar issues, and I don't think this change significantly
increases the risk. It may even mitigate it in some cases, since the
alternative of loading or evaling third-party scripts into the content
script sandbox would give them direct access to elevated privileges.

Per the CSP spec, those injections are assumed to be at the user's
behest, and should therefore take priority over the page author's
preferences.


Bug: https://bugzil.la/1267027

Link to standard: https://www.w3.org/TR/CSP/#extensions

Platform coverage: All platforms.

Estimated or target release: Firefox 58

Preference behind which this will be implemented:

No, it will be enabled by default.

Is this feature enabled by default in sandboxed iframes?

Yes.

DevTools bug: N/A

Do other browser engines implement this?

Chrome exempts content injected by extensions from CSP, though
it's somewhat inconsistent about how it handles its inline
scripts policy:

https://developer.chrome.com/extensions/contentSecurityPolicy#interactions

Edge appears to be planning to implement similar overrides, but
still subjects extension content to CSP:

https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11076023/
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/8074756/

Tests:

This functionality is not covered by web platform tests. There
will be new tests for its implementation in the WebExtension framework.

[Boris Zbarsky]

On 9/29/17 3:32 PM, Kris Maglione wrote:
> For instance, the following should all capture the
> caller principal for the `src` URL at call time:
>
>    document.write(`<img src="http://example.com/favicon.ico">`);
>    div.innerHTML = `<img src="http://example.com/favicon.ico">`;
>    img.setAttribute("src", "http://example.com/favicon.ico");
>    img.src = "http://example.com/favicon.ico";

What is the plan to do this, concretely? Changing img.src to thread
through a principal is not too bad but doing it for setAttribute would
be a bit of a performance annoyance, and threading them through the
parser would be _quite_ annoying.

The other option is to basically use something like SubjectPrincipal(),
but we're trying to remove existing uses of that, because the
action-at-a-distance makes it hard to reason about its security properties.

-Boris

[Kris Maglione]

I still haven't settled on the details, but I it will probably
have to involve capturing the caller principal from SetAttr
hooks. Which would involve either changing that machinery to
pass along a JS context when invoked by a scripted caller, or
using something like SubjectPrincipal(). I'd definitely like to
hear opinions on the best approach here.

The question of how to handler parser-generated nodes is
tougher. Just using SubjectPrincipal() is one obvious approach,
but the security characteristics of that worry me (what if the
parser gets invoked by system code while some JS caller is
blocked?). So I think it will probably require some sort of
special activation records that we can check (and sanity check)
from the attribute hooks. But I've barely begun looking into
this part yet.

[Luke Crouch]

On Friday, September 29, 2017 at 2:32:57 PM UTC-5, Kris Maglione wrote:

> Security & privacy concerns:
>
> This change will allow extensions to inject content into sites which can
> (and probably will) cause security and privacy issues. However, it's
> already quite easy for malicious or badly-implemented extensions to
> create similar issues, and I don't think this change significantly
> increases the risk. It may even mitigate it in some cases, since the
> alternative of loading or evaling third-party scripts into the content
> script sandbox would give them direct access to elevated privileges.
>
> Per the CSP spec, those injections are assumed to be at the user's
> behest, and should therefore take priority over the page author's
> preferences.

+1 on this part.

As an add-on author, when I need to inject something the page CSP doesn't allow, I can already over-write the page CSP to allow it. But that feels more dangerous!

[Daniel Veditz]

On Fri, Sep 29, 2017 at 8:33 PM, Boris Zbarsky <bzba...@mit.edu> wrote:

> On 9/29/17 3:32 PM, Kris Maglione wrote:
>
>> For instance, the following should all capture the caller principal for
>> the `src` URL at call time:
>>
>> document.write(`<img src="http://example.com/favicon.ico">`);
>> div.innerHTML = `<img src="http://example.com/favicon.ico">`;
>> img.setAttribute("src", "http://example.com/favicon.ico");
>> img.src = "http://example.com/favicon.ico";
>>
>
> What is the plan to do this, concretely? Changing img.src to thread
> through a principal is not too bad but doing it for setAttribute would be a
> bit of a performance annoyance, and threading them through the parser would
> be _quite_ annoying.
>

​Do you _need_ to make all those ways work? I'm especially worried about
the parser ones. As long as direct DOM manipulation works, and is easier
than overwriting (or removing) the page's CSP, can't we just encourage
people to use that mechanism?

innerHTML ("Satan's candy"), in particular, is often misused and leads to
XSS​. It's the very last thing I'd want to give a "get out of CSP free"
pass.

-
​Dan Veditz​

[Boris Zbarsky]

On 9/30/17 12:19 AM, Kris Maglione wrote:
> I still haven't settled on the details, but I it will probably have to
> involve capturing the caller principal from SetAttr hooks. Which would
> involve either changing that machinery to pass along a JS context when
> invoked by a scripted caller, or using something like
> SubjectPrincipal(). I'd definitely like to hear opinions on the best
> approach here.

I'd really rather we didn't use SubjectPrincipal() here. It's too hard
to reason about, and we have better options.

Passing along a JSContext would work. We could have something like
"null means no scripted caller, otherwise caller's compartment is the
part that matters". This relies on no one on the setattr path messing
with the compartment, but that shouldn't be too hard to ensure,
especially since we only have a few attributes on a few elements for
which this is relevant...

I'd love it if we could pass along something that couldn't be
abused/misused like a JSContext. We could make up a wrapper class, but
no matter what we do we'd have the fundamental tradeoff that either we
grab the principal eagerly, and pay the cost for all the cases where it
doesn't matter, or we grab it lazily and run the risk of thing changing
under us. We should probably measure how expensive setAttribute is and
how expensive grabbing the principal from a JSContext (e.g. by marking
the method as [NeedsCallerPrincipal]) is...

> The question of how to handler parser-generated nodes is tougher. Just
> using SubjectPrincipal() is one obvious approach, but the security
> characteristics of that worry me (what if the parser gets invoked by
> system code while some JS caller is blocked?).

Yes, that is the fundamental problem with SubjectPrincipal().

But even worse, depending on what you're trying to solve, using
SubjectPrincipal() from SetAttr() doesn't even work right. Consider
this case:

document.write("<script src=foo></script><img src=bar>");

The SetAttr call for the <img> happens async, after the <script>
finishes loading. So the principal state needs to be stored in the
parser itself or something.

Of course the other option is to not make this work for document.write...

I wonder whether we can coordinate with the Chrome folks somehow to make
sure that this stuff works in the same set of cases.

-Boris

[Boris Zbarsky]

On 10/2/17 10:50 AM, Daniel Veditz wrote:
> As long as direct DOM manipulation works, and is easier
> than overwriting (or removing) the page's CSP, can't we just encourage
> people to use that mechanism?

The fact is, direct DOM manipulation with no parser involved is really
annoying to use. Compare these two snippets:

var div = document.createElement("div");
div.id = "foo";
div.className = "bar";
parent.prepend(div);

and:

parent.prepend("<div id=foo class=bar>");

That said, I am sympathetic to the concern about innerHTML in
particular. Specifically, if an extension does:

parent.innerHTML += "<div id=foo class=bar>";

instead of doing:

parent.append("<div id=foo class=bar>");

and we exempt the former from CSP, then the extension just introduced
XSS into the page without even noticing...

So to be honest, my gut feeling is that we should not try to make
innerHTML or document.write() work here, but it would be nice to make
the ParentNode.append/prepend methods and maybe createContextualFragment
work...

-Boris

[Daniel Veditz]

On Mon, Oct 2, 2017 at 8:17 AM, Boris Zbarsky <bzba...@mit.edu> wrote:

> The fact is, direct DOM manipulation with no parser involved is really
> annoying to use.
>

​Fair enough. Could we propose improvements to the API​s that would make
them more usable? For example an object argument to createElement() that
contained attribute/value pairs?

var div = document.createElement("div", null, {"id":"foo",
"class":"bar"});
parent.prepend(div);

(the null is for the existing custom elements options param)

Wordier than your parent.prepend("<div id=foo class=bar>") example, but
safer from developers trying to get clever with it. Such a short example
isn't going to fall victim to CSP blockage. In a longer example with src
URLs the overhead might not be too bad.

-
​Dan Veditz​

[Boris Zbarsky]

On 10/2/17 12:03 PM, Daniel Veditz wrote:
> ​Fair enough. Could we propose improvements to the API​s that would make
> them more usable? For example an object argument to createElement() that
> contained attribute/value pairs?

This has definitely been proposed before. Worth checking with Anne to
see what the status is. Specifically, did it die, and if so why?
Because I, too, think this would be an interesting avenue to explore...

-Boris

[Anne van Kesteren]

See https://github.com/whatwg/dom/issues/150. There's not really any
dominant pattern that's succeeded here in libraries that we could
adopt. You typically end up looking at templating and that has its own
host of issues. The other thing that would solve some of this is
browser-backed sanitization, but that's also a hard problem to solve
nobody has been willing to tackle and get standardized.


--
https://annevankesteren.nl/

[Kris Maglione]

On Mon, Oct 02, 2017 at 07:50:41AM -0700, Daniel Veditz wrote:

>On Fri, Sep 29, 2017 at 8:33 PM, Boris Zbarsky <bzba...@mit.edu> wrote:
>
>> On 9/29/17 3:32 PM, Kris Maglione wrote:
>>
>>> For instance, the following should all capture the caller principal for
>>> the `src` URL at call time:
>>>
>>> document.write(`<img src="http://example.com/favicon.ico">`);
>>> div.innerHTML = `<img src="http://example.com/favicon.ico">`;
>>> img.setAttribute("src", "http://example.com/favicon.ico");
>>> img.src = "http://example.com/favicon.ico";
>

>Do you _need_ to make all those ways work? I'm especially worried about

>the parser ones. As long as direct DOM manipulation works, and is easier

>than overwriting (or removing) the page's CSP, can't we just encourage
>people to use that mechanism?

We do if we want Chrome parity, yes.

But I also have another motivation for wanting to make this as comprehensive
as possible. I'd like to make web-accessible extension URLs loadable only by
extension callers, to reduce the fingerprinting risk. Web content would still
be able to fingerprint based on content injected into pages, but they wouldn't
be able to poll for extension IDs.

There's already some risk of breaking extensions that inject scripts into the
page context if we do that. The risk goes up a lot more if we also don't
support things like innerHTML and insertAdjacentHTML.

document.write, though... To be honest, I'd be perfectly happy to forbid
content scripts from using document.write altogether, if I could get away with
it.

[Kris Maglione]

On Mon, Oct 02, 2017 at 11:13:20AM -0400, Boris Zbarsky wrote:
>Passing along a JSContext would work. We could have something like
>"null means no scripted caller, otherwise caller's compartment is the
>part that matters". This relies on no one on the setattr path messing
>with the compartment, but that shouldn't be too hard to ensure,
>especially since we only have a few attributes on a few elements for
>which this is relevant...
>
>I'd love it if we could pass along something that couldn't be
>abused/misused like a JSContext. We could make up a wrapper class,
>but no matter what we do we'd have the fundamental tradeoff that
>either we grab the principal eagerly, and pay the cost for all the
>cases where it doesn't matter, or we grab it lazily and run the risk
>of thing changing under us. We should probably measure how expensive
>setAttribute is and how expensive grabbing the principal from a
>JSContext (e.g. by marking the method as [NeedsCallerPrincipal]) is...

OK, I'll try a talos run with [NeedsCallerPrincipal] added to
setAttribute and see where that comes out. If it looks good,
I'll investigate that route some more. Otherwise, I'll probably
go with a JSContext wrapper and retrieve the principal on
demand.

>>The question of how to handler parser-generated nodes is tougher.
>>Just using SubjectPrincipal() is one obvious approach, but the
>>security characteristics of that worry me (what if the parser gets
>>invoked by system code while some JS caller is blocked?).
>
>Yes, that is the fundamental problem with SubjectPrincipal().
>
>But even worse, depending on what you're trying to solve, using
>SubjectPrincipal() from SetAttr() doesn't even work right. Consider
>this case:
>
> document.write("<script src=foo></script><img src=bar>");
>
>The SetAttr call for the <img> happens async, after the <script>
>finishes loading. So the principal state needs to be stored in the
>parser itself or something.
>
>Of course the other option is to not make this work for document.write...

Making it not work with document.write() is probably acceptable,
but I don't think we'll be able to get away with not supporting
innerHTML and friends.

This is why I wanted to go with an activation record approach
for the parser case, though. It should be easy enough for the
parser to handle removing the activation record when parsing is
interrupted or scripts are unblocked, and restoring it when it
resumes.

We may even be able to store the same type of context object
we'd use for setAttribute in it, and have the parser pass that
along to SetAttr calls.

>I wonder whether we can coordinate with the Chrome folks somehow to
>make sure that this stuff works in the same set of cases.

This is always a touchy subject. So far, they've been unwilling
to publicly participate in standardizing anything related to
their extension APIs.

[Kris Maglione]

Yes, I'll admit that's one of my motivations. Whenever we try to prevent
extensions from doing things for security or performance reasons, extensions
authors tend to just find another way to do it with worse security and
performance characteristics...

I filed bug 1273281 about preventing extensions from changing security headers
without a special permission, but for now, many extensions still do such
things.

[Kris Maglione]

On Mon, Oct 02, 2017 at 11:39:21AM -0700, Kris Maglione wrote:
>On Mon, Oct 02, 2017 at 11:13:20AM -0400, Boris Zbarsky wrote:

>>Passing along a JSContext would work. We could have something like
>>"null means no scripted caller, otherwise caller's compartment is
>>the part that matters". This relies on no one on the setattr path
>>messing with the compartment, but that shouldn't be too hard to
>>ensure, especially since we only have a few attributes on a few
>>elements for which this is relevant...
>>
>>I'd love it if we could pass along something that couldn't be
>>abused/misused like a JSContext. We could make up a wrapper class,
>>but no matter what we do we'd have the fundamental tradeoff that
>>either we grab the principal eagerly, and pay the cost for all the
>>cases where it doesn't matter, or we grab it lazily and run the risk
>>of thing changing under us. We should probably measure how
>>expensive setAttribute is and how expensive grabbing the principal
>>from a JSContext (e.g. by marking the method as
>>[NeedsCallerPrincipal]) is...
>

>OK, I'll try a talos run with [NeedsCallerPrincipal] added to
>setAttribute and see where that comes out. If it looks good, I'll
>investigate that route some more. Otherwise, I'll probably go with a
>JSContext wrapper and retrieve the principal on demand.

So far it doesn't look like there's any significant difference
on any talos test from adding [NeedsCallerPrincipal] to
setAttribute/setAttributeNS/Attr.value, so I'm going to go that
route for now. If it turns out to be a problem later, I'll
refactor it to pass a context object that lazily extracts the
principal.

[Boris Zbarsky]

On 10/2/17 5:35 PM, Kris Maglione wrote:
> So far it doesn't look like there's any significant difference on any
> talos test from adding [NeedsCallerPrincipal] to
> setAttribute/setAttributeNS/Attr.value,

OK. That's a minimum bar, obviously, but I would still like us to
measure what the (presumably nonzero) impact is, so we know what we're
dealing with. In particular, setAttribute may simply not be a huge part
of most talos tests.

What I'd like to see is how long setAttribute() with some meaningless
name like "testing" takes with/without this change on an HTMLDivElement
in both the "in document" and "out of document" cases. Just a silly
microbenchmark is ok for this purpose; it at least gives us an idea of
what happens in the L2-cache-hit case.

-Boris

[Kris Maglione]

For the pretty simple micro-benchmark below, here are the
in-document and out-of-document numbers for three runs without
the subject principal:

1260ns 1820ns
1260ns 1870ns
1014ns 1332ns

and three runs with:

1002ns 1470ns
1280ns 1722ns
1207ns 1898ns

The difference between the two is within the noise.


Without inserting the div into the document, the numbers were
closer to 500ns, with or without the subject principal, in- or
out-of-document.

I haven't tested a sandbox with X-rays, but I'm willing to bet
X-ray overhead completely overwhelms any other meaningful metric
in that case.


<body>
<iframe src="foo.html"></iframe>
<script>
"use strict";

function run(doc) {
let div = document.createElement("div");
doc.body.appendChild(div);

let iterations = 1024 * 1024;

let start = performance.now();
for (let i = 0; i < iterations; i++) {
div.setAttribute("data-num", i);
}
let end = performance.now();
let delta = (end - start) * 1000 * 1000;

doc.body.appendChild(document.createTextNode(`Per-iteration: ${Math.round(delta / iterations)}ns`));
}

let iframe = document.querySelector("iframe");
iframe.addEventListener("load", () => {
run(iframe.contentDocument);
}, {once: true});

window.addEventListener("load", () => {
run(document);
}, {once: true});
</script>
</body>

[Boris Zbarsky]

On 10/2/17 9:50 PM, Kris Maglione wrote:
> For the pretty simple micro-benchmark below, here are the in-document
> and out-of-document numbers for three runs without the subject principal:

Sorry, I should have been clearer: I meant numbers for "inserted into
the document" and "not inserted into the document".

> Without inserting the div into the document, the numbers were closer to
> 500ns, with or without the subject principal, in- or out-of-document.

Gotcha.

I just did a bit of testing with a non-inlined no-op function and it
looks like the overhead of NeedsSubjectPrincipal is on the order of
maybe 1-2ns. Looks like the actual implementation we end up using
mostly consists of reinterpret_cast, which is nice and fast. ;)
There's one memory read from the JSContext to get the compartment, and
one memory read from the compartment to get the principal; as long a
those hit cache all is good.

> I haven't tested a sandbox with X-rays

Yeah, not going to be a big factor there.

Sounds to me like just using NeedsSubjectPrincipal is fine here. Thank
you for checking this stuff!

-Boris

[Kris Maglione]

On Wed, Oct 04, 2017 at 12:42:22AM -0400, Boris Zbarsky wrote:
>On 10/2/17 9:50 PM, Kris Maglione wrote:
>>For the pretty simple micro-benchmark below, here are the
>>in-document and out-of-document numbers for three runs without the
>>subject principal:
>
>Sorry, I should have been clearer: I meant numbers for "inserted into
>the document" and "not inserted into the document".

Well, on the upside, if I hadn't misread you, I wouldn't have
thought to check the cross-document case (which is pretty
relevant to subject principal checks), but would have thought to
check inserted vs. non-inserted, so I think it came out for the
best :)

>I just did a bit of testing with a non-inlined no-op function and it
>looks like the overhead of NeedsSubjectPrincipal is on the order of
>maybe 1-2ns. Looks like the actual implementation we end up using
>mostly consists of reinterpret_cast, which is nice and fast. ;)
>There's one memory read from the JSContext to get the compartment, and
>one memory read from the compartment to get the principal; as long a
>those hit cache all is good.

I'd be pretty surprised if we ever manage to get to that point
without both locations being in the cache. We check compartment
flags all over the place when JS is running. Maybe just after a
block of pure JIT code that touched a huge amount of memory...

So, yeah, it doesn't seem like it should be an issue in
practice.

[Frederik Braun]

On 02.10.2017 18:03, Daniel Veditz wrote:
> ​Fair enough. Could we propose improvements to the API​s that would make
> them more usable? For example an object argument to createElement() that
> contained attribute/value pairs?
>

> var div = document.createElement("div", null, {"id":"foo",
> "class":"bar"});
> parent.prepend(div);
>
> (the null is for the existing custom elements options param)

Well, you _could_ write something like

var div = Object.assign(document.createElement("div"), {
id: "foo",
className: "bar",
});

[Frederik Braun]

On 02.10.2017 18:43, Anne van Kesteren wrote:

> See https://github.com/whatwg/dom/issues/150. There's not really any
> dominant pattern that's succeeded here in libraries that we could
> adopt. You typically end up looking at templating and that has its own
> host of issues. The other thing that would solve some of this is
> browser-backed sanitization, but that's also a hard problem to solve
> nobody has been willing to tackle and get standardized.
>
>

Some folks are Google are working on a solution based on Types:
<https://github.com/mikewest/trusted-types/>

I've tried providing feedback here and there, but they are moving fast
and I'm not included in all of their conversations, since they are not
public (despite their good history of working with W3C WebAppSec). :(