910139 - (CVE-2014-1485) CSP should block XSLT as script, not as style

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Frederik Braun [:freddy]]

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)

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 2

[Frederik Braun [:freddy]]

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)

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 4

[Frederik Braun [:freddy]]

Oh, I was misunderstanding the code and hoped it was just a XSLT code path, that we could easily switch out :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Going to punt to Jonas on this.

Comment 8

[Frederik Braun [:freddy]]

bz: Did you? ;)

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style.patch

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.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style_tests.patch

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Jonas: are you recommending TYPE_SCRYLE?  :)
ckerschb: how hard would it be to hack up nsIContentPolicy to add a new TYPE_XSLT?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(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 :-)

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style.patch

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?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

can you recommend another reviewer, jonas?  Maybe jst?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style_tests.patch

just did a rebase to allow easy import for reviewing this patch!

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style.patch

incorporated Jonas suggestion end reverted the uuid - no ned to generate a new uuid when just adding a constant!

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

jst, can you take this one?

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 809399 [details] [diff] [review]
bug_910139_xslt_as_script_not_style.patch

smaug, do you mind reviewing this?

Comment 24

[Garrett Robinson [:grobinson]]

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.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style_tests.patch

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

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 810689 [details] [diff] [review]
bug_910139_xslt_as_script_not_style_tests.patch

carrying over grobinsion r+

Comment 27

[Johnny Stenback (:jst)]

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 :)

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

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"

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Do these tests work on b2g or do we need to disable them like the other csp tests that break on b2g?

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Pushed to try, let's see what happens for B2G:
  https://tbpl.mozilla.org/?tree=Try&rev=acf9e68fda79

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style_tests.patch

carrying over r+ from grobinson

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_xslt_as_script_not_style.patch

carrying over r+ from grobinson and jst

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Sid, I guess this looks good, passing tests on try (Comment 31) - would you do me the honor and merge it in?

Comment 35

[Sid Stamm [:geekboy or :sstamm]]

Has bitrot from bug 920223.  Christoph, can you update the patch and double-check that the tests run?

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_csp_should_block_xslt_as_script.patch

rebase against inbound

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_910139_csp_should_block_xslt_as_script_tests.patch

rebase against inbound

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a896a85ce0c

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 42

[Daniel Veditz [:dveditz]]

Is this worth uplifting to aurora?

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

It seems to me it's not a high risk security threat, I guess it can wait. But probably Sid should decide. Sid?

Comment 44

[Sid Stamm [:geekboy or :sstamm]]

I don't think it's urgent, though it's a pretty small patch so I could go either way.

Comment 45

[Tracy Walker [:tracy]]

verified with Nightly build 20131024030204