663567 - Verify that content added by XSLT stylesheet is subject to document's CSP

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

Any content to be added by an XSLT stylesheet must be permitted by that document's Content Security Policy.

Comment 1

[Ian Melven :imelven]

Is this still a valid issue, Sid ? (if you know off hand)

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Oh boy... hm.  I don't have a test for this lying around, so no idea if it's an issue.  Brandon: do you have a test case (reduced or not) for this?

Comment 3

[Brandon Sterne (:bsterne)]

I don't, sorry.

Comment 4

[Ian Melven :imelven]

I'm pretty sure Brandon will be ok with me taking this.

Comment 5

[Ian Melven :imelven]

This needs testing to see if it's actually a problem ...

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: mochitest confirming content loaded by XSL is subject to CSP

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

It seems the CSP for content loaded by XSL behaves as expected. It loads the XSP if loaded from the same domain, but prevents the XSL loaded from example.com.
Confirming using the debug output:

CSP debug: blocking request for http://example.org/tests/content/base/test/file_CSP_bug663567_blocks.xsl

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 774136 [details] [diff] [review]
mochitest confirming content loaded by XSL is subject to CSP

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

Looks pretty good, but I'm curious about a few things.

::: content/base/test/file_CSP_bug663567_allows.xml^headers^
@@ +1,1 @@
> +X-Content-Security-Policy: default-src 'self'
\ No newline at end of file

Please use the non-prefixed header.  We're gonna get rid of the prefixed ones soon and "Content-Security-Policy" header payloads are intended to be what's used in the future.

::: content/base/test/test_CSP_bug663567.html
@@ +29,5 @@
> +   *   we load the xsl file using:
> +   *   <?xml-stylesheet type="text/xsl" href="file_CSP_bug663467_allows.xsl"?>
> +   */
> +  var cspframe = document.getElementById('xsltframe');
> +  var xsltAllowedHeader = cspframe.contentWindow.document.getElementById('xsltheader').innerHTML;

Won't calling .innerHTML throw if getElementById returns null (like if the XSLT doesn't load)?  Why don't you just check if the element is null since if the XSLT isn't loaded the <h2 id="xsltheader"> element won't exist...

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> Comment on attachment 774136 [details] [diff] [review]
> mochitest confirming content loaded by XSL is subject to CSP
> 
> Review of attachment 774136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good, but I'm curious about a few things.
> 
> ::: content/base/test/file_CSP_bug663567_allows.xml^headers^
> @@ +1,1 @@
> > +X-Content-Security-Policy: default-src 'self'
> \ No newline at end of file
> 
> Please use the non-prefixed header.  We're gonna get rid of the prefixed
> ones soon and "Content-Security-Policy" header payloads are intended to be
> what's used in the future.

ok, i'll incorporate that.

> 
> ::: content/base/test/test_CSP_bug663567.html
> @@ +29,5 @@
> > +   *   we load the xsl file using:
> > +   *   <?xml-stylesheet type="text/xsl" href="file_CSP_bug663467_allows.xsl"?>
> > +   */
> > +  var cspframe = document.getElementById('xsltframe');
> > +  var xsltAllowedHeader = cspframe.contentWindow.document.getElementById('xsltheader').innerHTML;
> 
> Won't calling .innerHTML throw if getElementById returns null (like if the
> XSLT doesn't load)?  Why don't you just check if the element is null since
> if the XSLT isn't loaded the <h2 id="xsltheader"> element won't exist...

that's true, i'll fix that.

Comment 10

[Ian Melven :imelven]

Comment on attachment 774136 [details] [diff] [review]
mochitest confirming content loaded by XSL is subject to CSP

Same comment on using the unprefixed header

I think it's not totally bad to check the text to see if things loaded all the way.. maybe a try/catch ? 

Otherwise looks good, thanks for writing the test !

Comment 11

[Ian Melven :imelven]

I just happened to be catching up on the w3c webappsec list today and noticed this post : http://lists.w3.org/Archives/Public/public-webappsec/2013Jun/0079.html

In particular, from Adam Barth :

"You might think that XSLT is controlled by style-src because it's
styling information, but we've actually put it into the script-src
bucket because it's just as powerful as script.  You can see some
discussion of this topic if you search for XSLT in
https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html."

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: updated mochi test verifying CSP restricts EventSource using the connect-src directive

Comment 13

[Garrett Robinson [:grobinson]]

Comment on attachment 776612 [details] [diff] [review]
updated mochi test verifying CSP restricts EventSource using the connect-src directive

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

The title of this attachment is totally different from the title and content of the bug. Was this an accidental copy/paste from another bug?

This test looks good, and it is good to see that CSP is doing the right thing for XSLT. The only changes I would make would be to remove some ok()'s that are not really testing anything. With that, r+.

Please unbitrot Makefile.in when you make the other changes.

::: content/base/test/test_CSP_bug663567.html
@@ +68,5 @@
> +    document.getElementById('xsltframe2').addEventListener('load', checkBlocked, false);
> +    next();
> +  },
> +  function () {
> +    ok(true, "All tests finished!\n");

Remove this line; this is not a test.

@@ +74,5 @@
> +  }
> +];
> +
> +function next() {
> +  ok(true, "Begin!");

Remove this line; this is not a test.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_663567_v2_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

removed unnecessary checks, and also renamed the patch-file.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 789146 [details] [diff] [review]
bug_663567_v2_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

carrying over r=grobinson from previous review.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_663567_v3_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

setting up the patch so someone else can check in my code!

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 789232 [details] [diff] [review]
bug_663567_v3_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

I can't get to it in the next few hours... added checkin-needed keyword so it's fair game for anyone.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/850ef8175560

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest-1 timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa55dcb8ed4c

https://tbpl.mozilla.org/php/getParsedLog.php?id=26492003&tree=Mozilla-Inbound

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_663567_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

fixed problem with async calls!

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 790817 [details] [diff] [review]
bug_663567_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

:sstamm already reviewed this, ready to be checked in!

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 790817 [details] [diff] [review]
bug_663567_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch

I guess it has to be the '?' in the checkin-box so someone else can check it in!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/608fd9e61e12

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/608fd9e61e12

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_663567_xslt_test_update.patch

If you update the file
  file_CSP_bug663567_blocks.xml^headers^
and set the CSP to:
  Content-Security-Policy: default-src 'self'
you get the behavior/error you expect.

The setup for this testcase should have been set up like it is now from the beginning.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 799602 [details] [diff] [review]
bug_663567_xslt_test_update.patch

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

This patch updates the test so it checks default-src must authorize xslt loads:

allowed: "default-src 'self'", relative path XSLT
blocked: "default-src *.example.com", relative path XSLT not on example.com

What about the situation where the XSLT is loaded cross-origin and should be allowed/blocked?  From some manual testing, it looks like when it CSP should allow a cross-origin request for XSLT it gets an NS_ERROR_DOM_BAD_URI, which means we're not supposed to load xslt files cross-origin and probably needs CORS to work properly.  So I think this test works fine.  Thanks for the turnaround.

Should we be testing the script-src directive (not default-src) given comment 11 (or is that a follow-up bug)?

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #26)
> Should we be testing the script-src directive (not default-src) given
> comment 11 (or is that a follow-up bug)?

I think this test should only verify that XSLT is subject to the CSP. We should test that XSLT a subject to the script-src directive in a follow up bug.

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/49a194c84ab0

Comment 29

[Garrett Robinson [:grobinson]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #26)
> > Should we be testing the script-src directive (not default-src) given
> > comment 11 (or is that a follow-up bug)?
> 
> I think this test should only verify that XSLT is subject to the CSP. We
> should test that XSLT a subject to the script-src directive in a follow up
> bug.

Follow up in Bug 910139.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49a194c84ab0