1548385 - CSP bypass using <script> inside a <template>

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: a.html

See the attached test case. Edge and Chrome block the script from executing but Firefox executes it.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: page.js

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

I'll take a look - seems bad!

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

I took a closer look at the problem and it seems we are blocking the script correctly if the resources are on the web. On the console I get:

Content Security Policy: The page’s settings blocked the loading of a resource at https://bug1548385.bmoattachments.org/page.js (“default-src”).

I wrote a mochitest because we didn't have one for templates (which I'll upload in a minute) which demonstrates that our CSP implementation blocks the resources as expected.

Now, if I download the test and run it from my local system (using file:/// URIs) then our CSP implementation allows the script to execute because we exempt local resources from CSP, see subjectToCSP() here:
https://searchfox.org/mozilla-central/source/dom/security/nsCSPService.cpp#107

It think we are good just landing the test in that case. If so I think we can lower the priority and severity and even open it up.

Brian, Dan, anything I am missing?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1548385: Test CSP blocks scripts correctly within template. r=jkt

Comment 5

[Jonathan Kingston [:jkt] he/him]

I'm also able to replicate what :ckerschb reports for http://192.168.2.106:8080/a.html as it behaves like a normal server.

Out of interest :ckerschb why are we whitelisting local files?

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: all-in-one testcase

Made a self-contained testcase using a data: url instead of a separate file so it will function loaded from bugzilla. Not quite the same, though, since we have special rules for data: too in some circumstances. FWIW this one doesn't execute the script even when loaded from a local file (because the template inclusion isn't a whitelisted file:/// url).

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Jonathan Kingston [:jkt] from comment #5)

Out of interest :ckerschb why are we whitelisting local files?

We really wanted to whitelist resource: (used internally and by legacy add-ons at the time) but do it generically, and file: came along for the ride. This seemed OK at the time because web pages can't include file: urls but can include resource: in some contexts.

But wait, then much later in bug 1432358 we explicitly made resource: subject to CSP (with exceptions). Maybe that's just vestigial and we should get rid of it? Maybe it would be better to invent URI_IGNORED_BY_CSP as an explicit way for protocols to bypass CSP, rather than hoping categories made for other purposes will work?

I could go either way, but I'm sleepy and might have a different opinion tomorrow.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #7)

(In reply to Jonathan Kingston [:jkt] from comment #5)

Out of interest :ckerschb why are we whitelisting local files?

We really wanted to whitelist resource: (used internally and by legacy add-ons at the time) but do it generically, and file: came along for the ride. This seemed OK at the time because web pages can't include file: urls but can include resource: in some contexts.

As Dan points out there is a lot of history involved in whitelisting local resources. Because of Bug 1432358 we explicitly wanted to have CSP cover more of them. Anyway, I think we should file a follow up bug to re-investigate. Having an explicit flag like URI_IGNORED_BY_CSP or something there like makes complete sense to me. We should try to be more explicit about those kind of things and not rely on flags that were invented for some other purpose. Because if that purposes changes, then we might be in trouble.

To sum it up, let's file a follow up bug to investigate if we can replace URI_IS_LOCAL_RESOURCE with e.g. URI_IGNORED_BY_CSP.

Comment 9

[Daniel Veditz [:dveditz]]

To sum it up, let's file a follow up bug to investigate if we can replace URI_IS_LOCAL_RESOURCE with e.g. URI_IGNORED_BY_CSP.

I thought I included this last night but must've edited it out. A more limited fix would be to add to the check in subjectToCSP() that if it is URI_IS_LOCAL_RESOURCE make sure it is not also URI_IS_LOCAL_FILE.

Given the partial exclusions for chrome: and resource: I'm not sure which handlers would actually use a URI_IGNORED_BY_CSP.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

FWIW, I filed Bug 1551253 to re-revaluate if local files should be subject to CSP.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Removing the keywords since this is not a security sensitive bug in the end. Nevertheless we didn't have an explicit testcase for it - so let's use this bug to land one.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff2278c703c1
Test CSP blocks scripts correctly within template. r=jkt

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ff2278c703c1

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

The test within this bug landed - clearning pending ni? requests.