941404 - XHR fetch of document with CSP header sets CSP for document that loads it (unless CSP already set)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Deian Stefan]

If a page does not have a CSP policy set then using XHR to fetch a
document for which a CSP header is provided will set the page policy
to that of the XHR-fetched document. This can easily be used to break
a page with an overly-strict policy.


I speculate that this is because we set the channel owner to the
document principal
(https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1904).
Since the principal does not have CSP set, the the document load
(https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2034)
will set the new CSP in InitCSP:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2723.

As noted in bug 919587 comment 11, DOMParser similarly uses the
principal. It seems like we want to use a clone of the principal that
does not share the underlying CSP.

Comment 1

[Deian Stefan]

Attachment: mochitest for bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

You can't use a clone: the principal has to alias; otherwise the page won't be able to touch the data document when it has a nonce principal.

What we should _actually_ do, imo, is not do the CSP stuff in data documents, since imo it makes no sense to do it there.

Comment 3

[Deian Stefan]

(In reply to Boris Zbarsky [:bz] from comment #2)
> What we should _actually_ do, imo, is not do the CSP stuff in data
> documents, since imo it makes no sense to do it there.

Yes, I agree. (I was thinking of the same semantics, but you are right: my proposal would break the existing nonce principal semantics.)

I'd like to take a shot at this. Can one of you guys assign me to it? (I don't have editbugs privs yet.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Done, and now you do.

Comment 5

[Deian Stefan]

Attachment: 0001-Bug-941404-Data-documents-should-not-set-CSP.patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8336388 [details] [diff] [review]
0001-Bug-941404-Data-documents-should-not-set-CSP.patch

r=me, though I mostly skimmed the tests

Comment 7

[Deian Stefan]

Attachment: 0001-Bug-941404-Data-documents-should-not-set-CSP.patch

Disable mochitest on b2g. Carrying over r+ from bz.

Comment 8

[Garrett Robinson [:grobinson]]

Comment on attachment 8336388 [details] [diff] [review]
0001-Bug-941404-Data-documents-should-not-set-CSP.patch

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

Drive-by review! Please make sure the test still passes after you fix the policy.

::: content/base/test/csp/file_CSP_bug941404_xhr.html^headers^
@@ +1,1 @@
> +Content-Security-Policy: default-src none 'unsafe-inline' 'unsafe-eval'

s/none/'none'

::: content/base/test/csp/test_CSP_bug941404.html
@@ +29,5 @@
> +// a postMessage handler that is used by sandboxed iframes without
> +// 'allow-same-origin' to communicate pass/fail back to this main page.
> +// it expects to be called with an object like {ok: true/false, desc:
> +// <description of the test> which it then forwards to ok()
> +window.addEventListener("message", receiveMessage, false);

Is this code leftover? I can't find any sandboxed iframes using postMessage.

Comment 9

[Deian Stefan]

Attachment: 0001-Bug-941404-Data-documents-should-not-set-CSP.patch

Cleanup test. Thanks Garrett!

Carrying r+ from bz.

Comment 10

[Deian Stefan]

Try is good: https://tbpl.mozilla.org/?tree=Try&rev=729bee837023

Comment 11

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/beea4e4c8449

Comment 12

[Phil Ringnalda (:philor)]

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