Closed Bug 910139 (CVE-2014-1485) Opened 8 years ago Closed 8 years ago

CSP should block XSLT as script, not as style

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox25 --- wontfix
firefox26 --- affected
firefox27 --- verified
firefox-esr17 --- wontfix
firefox-esr24 --- wontfix
b2g18 --- wontfix

People

(Reporter: freddy, Assigned: ckerschb)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-moderate, verifyme, Whiteboard: [adv-main27+])

Attachments

(2 files, 8 obsolete files)

8.02 KB, patch
ckerschb
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
7.59 KB, patch
ckerschb
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
It looks like this was already handled in bug 663567, but according to the webappsec-WG test suite for CSP (see URL), we fail this test and other browsers pass.

In order to achieve compliance with the spec, we should make sure this test passes.
(Bypassing CSP can be very harmful in b2g, so I'm making this bug hidden)
Here's the CSP for that test:
script-src http://www2.webappsec-test.info; report-uri ../support/report.php?reportID=219436899

This assumes that XSLT are subject to script-src, not style-src.  We submit them to style-src directives (or default-src in this case).  
  http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#725
then:
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#87

So it looks like we're possibly not in line with the spec:
4.2: The script-src directive restricts which scripts the protected resource can execute. The directive also controls other resources, such as XSLT style sheets [XSLT], which can cause the user agent to execute script.

I don't like "such as" here.  That means we'll encounter behaviors like this that differ across browsers.  Changing the nsIContentPolicy context in the style loader may break other things.  The other option is to special case this in CSP's ShouldLoad (ugh).
If we just changed it from style to script it would be a one-line fix, correct?
Does anything speak against this?
If not, I'll volunteer to patch nsXMLContentSink.cpp and the tests for bug 663567. 

Do we want to raise this with the webappsec wg to make sure the wording will become more precise than "such as", so we don't trip over anything else that implicitly slips in?
Maybe just change it to "The directive also controls XSLT style sheets [XSLT]." and add other subjects once the emerge (explicit being better than implicit here)
It wouldn't be a one-line fix because we don't want CSS files to be classified as script (both are TYPE_STYLE).  :(  We have to split that category.
Oh, I was misunderstanding the code and hoped it was just a XSLT code path, that we could easily switch out :(
The codepath in question is in fact XSLT-only.

The options are to change XSLT to use something other than TYPE_STYLESHEET for the conpol check or to change CSP to look at the type for TYPE_STYLESHEET and detect the XSLT types....
I would prefer to reclassify XSLT calls into the content policy checks as TYPE_SCRIPT (faster and cleaner for CSP), but that's potentially confusing or problematic for any other consumers of XSLT content policy checks.  We could of course add comments in nsIContentPolicy.idl to call this out, but still...  

bz: what would you recommend here?
Flags: needinfo?(bzbarsky)
Going to punt to Jonas on this.
Flags: needinfo?(bzbarsky)
Summary: CSP should block XSLT → CSP should block XSLT as script, not as style
bz: Did you? ;)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Assignee: nobody → mozilla
jonas?  Can we reclassify XSLT loads as TYPE_SCRIPT or do we need to change CSP to classify based on nsIContentPolicy.TYPE* and mime type?
If we decide to go with the easy fix, than this is the patch we want. I am totally willing to update to a more nuanced solution if Jonas thinks its worth doing.
Attachment #803288 - Flags: feedback?(jonas)
Attachment #803289 - Flags: feedback?(jonas)
I suspect the best solution is to add a new class specifically for XSLT. It doesn't seem to me like neither SCRIPT or STYLE really fits the bill for XSLT as it falls somewhere in between.
Flags: needinfo?(jonas)
Jonas: are you recommending TYPE_SCRYLE?  :)
ckerschb: how hard would it be to hack up nsIContentPolicy to add a new TYPE_XSLT?
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13)
> ckerschb: how hard would it be to hack up nsIContentPolicy to add a new
> TYPE_XSLT?

I would say: let's give it a try and we shall see how hard it is :-)
This patch code passes all mochitests and also considers XSLT to be script (see test code in this bug).
I left some TODOs in the patch which Sid pointed out to me and are worth discussing:

1) Do we want to update nsDataDocumentContentPolicy?
Basically, can XSLTs be referenced by a document loaded via XMLHttpRequest or data URIs?

2) Do we need to update the nsWebBrowserContentPolicy?

I guess in both cases the question is, is XSLT still considered STYLE, or rather SCRIPT? In case we consider it to be SCRIPT, we should probably update nsDataDocumentContentPolicy and nsWebBrowserContentPolicy?
Attachment #803288 - Attachment is obsolete: true
Attachment #803288 - Flags: feedback?(jonas)
Comment on attachment 803289 [details] [diff] [review]
bug_910139_xslt_as_script_not_style_tests.patch

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

Can you get someone else to look at this. Sorry, my backlog is too big.
Attachment #803289 - Flags: feedback?(jonas) → feedback?
can you recommend another reviewer, jonas?  Maybe jst?
Flags: needinfo?(jonas)
Comment on attachment 807277 [details] [diff] [review]
bug_910139_xslt_as_script_not_style.patch

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

This looks pretty good. Though not sure what the answer is for the other content policies off the top of my head.

::: content/base/src/nsDataDocumentContentPolicy.cpp
@@ +124,5 @@
>        aContentType == nsIContentPolicy::TYPE_SUBDOCUMENT ||
>        aContentType == nsIContentPolicy::TYPE_SCRIPT) {
> +      // TODO: do we want to add XSLT here
> +      // is XSLT considered style, or rather script?
> +      // aContentType == nsIContentPolicy::TYPE_XSLT

I think XSLT isn't applied for data documents anyway. So might as well forbid it here too.
Attachment #807277 - Flags: feedback+
just did a rebase to allow easy import for reviewing this patch!
Attachment #803289 - Attachment is obsolete: true
Attachment #803289 - Flags: feedback?
Attachment #809398 - Flags: review?(grobinson)
incorporated Jonas suggestion end reverted the uuid - no ned to generate a new uuid when just adding a constant!
Attachment #807277 - Attachment is obsolete: true
Attachment #809399 - Flags: review?(jonas)
Attachment #809399 - Flags: review?(grobinson)
Attachment #809399 - Flags: review?(jonas) → review?(bent.mozilla)
Comment on attachment 809399 [details] [diff] [review]
bug_910139_xslt_as_script_not_style.patch

I've spent the last month doing reviews (neglecting my own work) so please try to find someone else on the DOM list? Sorry :(
Attachment #809399 - Flags: review?(bent.mozilla)
jst, can you take this one?
Flags: needinfo?(jst)
Comment on attachment 809399 [details] [diff] [review]
bug_910139_xslt_as_script_not_style.patch

smaug, do you mind reviewing this?
Attachment #809399 - Flags: review?(bugs)
Attachment #809399 - Flags: review?(grobinson) → review+
Comment on attachment 809398 [details] [diff] [review]
bug_910139_xslt_as_script_not_style_tests.patch

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

This is an example of a test where you don't need to have two different source files. You can just serve the same file with two different headers to test both the allowed case and the blocked case. You can use file_CSP_bug910139, and serve it with "script-src 'self'" for the allowed case, and "script-src '*.example.com'" for the blocked case.

For an example of another test that does this, which also has a handy .sjs file that lets you cut down on code duplication, check out test_CSP_bug888172.html. It would be easily to generalize the .sjs to cspserver.sjs or similar, and to pass in the filename of the HTML source file as an additional query parameter ;)

It's up to you if you want to make this change; two test cases isn't a heinous amount of duplication, so I'm fine with it either way.
Attachment #809398 - Flags: review?(grobinson) → review+
Eliminating duplicate code is always worth doing. I am also using an sjs file now, that seems to be a way better setup than using all these ^headers^ everywhere. Thanks Christoph
Attachment #809398 - Attachment is obsolete: true
Attachment #810689 - Flags: review?(grobinson)
Comment on attachment 810689 [details] [diff] [review]
bug_910139_xslt_as_script_not_style_tests.patch

carrying over grobinsion r+
Attachment #810689 - Flags: review?(grobinson) → review+
Comment on attachment 809399 [details] [diff] [review]
bug_910139_xslt_as_script_not_style.patch

Stealing review, r=jst. Also, nice subtle bugfix in nsContentBlocker.cpp :)
Attachment #809399 - Flags: review?(bugs) → review+
Flags: needinfo?(jst)
Flags: needinfo?(jonas)
When this lands, we have to update the commit messages in the patches to reflect the changes made (not the subject of the bug).  Should be:

"Bug 910139 - add TYPE_XSLT to nsIContentPolicy for XSLT loads and update CSP to treat XSLT as script. r=grobinson,jst"
"Bug 910139 - tests for new nsIContentPolicy TYPE_XSLT via CSP. r=grobinson"
Do these tests work on b2g or do we need to disable them like the other csp tests that break on b2g?
Flags: needinfo?(mozilla)
Hm, when I pushed to try I was only testing Linux X64 - can I somehow find out locally? Don't have the emulator setup though. Probably we need to disable them though.
Flags: needinfo?(mozilla)
Pushed to try, let's see what happens for B2G:
  https://tbpl.mozilla.org/?tree=Try&rev=acf9e68fda79
carrying over r+ from grobinson
Attachment #810689 - Attachment is obsolete: true
Attachment #812118 - Flags: review+
carrying over r+ from grobinson and jst
Attachment #809399 - Attachment is obsolete: true
Attachment #812119 - Flags: review+
Sid, I guess this looks good, passing tests on try (Comment 31) - would you do me the honor and merge it in?
Has bitrot from bug 920223.  Christoph, can you update the patch and double-check that the tests run?
rebase against inbound
Attachment #812119 - Attachment is obsolete: true
Attachment #812317 - Flags: review+
rebase against inbound
Attachment #812118 - Attachment is obsolete: true
Attachment #812319 - Flags: review+
Attachment #812317 - Flags: checkin?(sstamm)
Attachment #812319 - Flags: checkin?(sstamm)
Comment on attachment 812317 [details] [diff] [review]
bug_910139_csp_should_block_xslt_as_script.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c7e4e24bea
Attachment #812317 - Flags: checkin?(sstamm) → checkin+
Comment on attachment 812319 [details] [diff] [review]
bug_910139_csp_should_block_xslt_as_script_tests.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a896a85ce0c
Attachment #812319 - Flags: checkin?(sstamm) → checkin+
Flags: in-testsuite+
Target Milestone: --- → mozilla27
Is this worth uplifting to aurora?
Flags: needinfo?(mozilla)
It seems to me it's not a high risk security threat, I guess it can wait. But probably Sid should decide. Sid?
Flags: needinfo?(mozilla) → needinfo?(sstamm)
I don't think it's urgent, though it's a pretty small patch so I could go either way.
Flags: needinfo?(sstamm)
Keywords: verifyme
verified with Nightly build 20131024030204
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main27+]
Alias: CVE-2014-1485
Group: core-security
You need to log in before you can comment on or make changes to this bug.