Closed Bug 1548385 Opened 9 months ago Closed 8 months ago

CSP bypass using <script> inside a <template>

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: briansmith, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(4 files)

Attached file a.html

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

Group: core-security → dom-core-security
Component: Security → DOM: Security

I'll take a look - seems bad!

Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]

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?

Flags: needinfo?(dveditz)
Flags: needinfo?(brian)

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?

Flags: needinfo?(ckerschb)
Attached file 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).

Flags: needinfo?(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.

(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.

Flags: needinfo?(ckerschb)

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.

Group: dom-core-security
Blocks: 1551253

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

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.

Summary: CSP bypasss using <script> inside a <template> → CSP bypass using <script> inside a <template>

Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff2278c703c1
Test CSP blocks scripts correctly within template. r=jkt

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(brian)
You need to log in before you can comment on or make changes to this bug.