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

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bsterne, Assigned: ckerschb)

Tracking

(Blocks 2 bugs)

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Any content to be added by an XSLT stylesheet must be permitted by that document's Content Security Policy.
Is this still a valid issue, Sid ? (if you know off hand)
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?
I don't, sorry.
Blocks: CSP
I'm pretty sure Brandon will be ok with me taking this.
Assignee: brandon → imelven
Flags: needinfo?(imelven)
Assignee: imelven → nobody
This needs testing to see if it's actually a problem ...
Flags: needinfo?(imelven)
Assignee: nobody → mozilla
Attachment #774136 - Flags: feedback?(sstamm)
Attachment #774136 - Flags: feedback?(imelven)
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 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...
Attachment #774136 - Flags: feedback?(sstamm)
(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 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 !
Attachment #774136 - Flags: feedback?(imelven) → feedback+
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 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.
Attachment #776612 - Flags: review?(grobinson) → review+
removed unnecessary checks, and also renamed the patch-file.
Attachment #776612 - Attachment is obsolete: true
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.
Attachment #789146 - Flags: review+
setting up the patch so someone else can check in my code!
Attachment #789146 - Attachment is obsolete: true
Attachment #789232 - Flags: review+
Attachment #789232 - Flags: checkin?(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.
Attachment #789232 - Flags: checkin?(sstamm)
fixed problem with async calls!
Attachment #789232 - Attachment is obsolete: true
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!
Attachment #790817 - Flags: review+
Attachment #790817 - Flags: checkin+
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!
Attachment #790817 - Flags: checkin+ → checkin?
Attachment #790817 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/608fd9e61e12
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #799602 - Flags: review?(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)?
Attachment #799602 - Flags: review?(sstamm) → review+
(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.
Attachment #790817 - Attachment is obsolete: true
(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.
https://hg.mozilla.org/mozilla-central/rev/49a194c84ab0
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.