1381761 - Treating 'data:' documents as unique, opaque origins should still inherit the CSP

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_test_browser_911547.patch

Smaug, it seems that test was written for Bug 911547 to make sure that data: URIs inherit the CSP from the including context and that the CSP survives session restore.

Anyway, here is the main question. As implemented in the test:

<a id="test_data_link" href="data:text/html;charset=utf-8,<input type='text' id='test_id2' value='id2_initial'/> <script>document.getElementById('test_id2').value = 'id2_modified';</script>">Test Link</a>

Later the test queries 'test_id2' using:
content.document.getElementById("test_id2").value

In the new world, where data: URIs do *not* inherit the security context of the including document, isn't test_id2 created by a data: URI and should hence be considered cross origin?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Right, assuming CSP spec doesn't have any special cases for data: handling.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_test_browser_911547.patch

Dan, as you know, we are updating the way the data: URI inheritance security model works within Firefox. By flipping the pref security.data_uri.unique_opaque_origin data: URIs become cross origin with the including context and hence do *not* inherit the CSP for the including context anymore.

I updated this test, to test different things depending on the pref. If the pref is false (like it is right now), the test keeps testing that data: URIs inherit the CSP of the including context. Once the pref is true, this tests ensures that data: URIs do *not* inherit the CSP (even after a session restore).

Further I slightly improved the test to use unique values for each subtest instead of 'ok' and 'fail'. Thanks!

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> data: URIs become cross origin with the including context and hence
> do *not* inherit the CSP for the including context anymore.

That sounds right. As a sanity check is this how Chrome behaves? Without inheritance the only way to get a CSP into a data: document will be to use a <meta> tag.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8887888 [details] [diff] [review]
bug_1381761_test_browser_911547.patch

(In reply to Daniel Veditz [:dveditz] from comment #4)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> > data: URIs become cross origin with the including context and hence
> > do *not* inherit the CSP for the including context anymore.
> 
> That sounds right. As a sanity check is this how Chrome behaves? Without
> inheritance the only way to get a CSP into a data: document will be to use a
> <meta> tag.

Thanks Dan for bringing that up. And as discussed over IRC, data: URI iframes should inherit the CSP of the including context. That translates into the following, that test should *not* be rewritten and should continue to work. We have to adjust our Gecko implementation to inherit the CSP into the data: iframe.

[1] https://www.w3.org/TR/CSP2/#which-policy-applies

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_treating_data_as_opaque_should_inherit_csp.patch

Dan, smaug, similar how we inherit the origin attributes within the scriptsecurityManager we should also inherit the CSP even if we treat data: URIs as unique, opaque origins.

Applying the attached patch makes browser_911547.js test work again as expected. Further I added another test (next patch) that makes sure that data: URI iframes inherit the CSP from the including context. Currently on mozilla-central that should happen because of the way data: URIs inherit the security context, hence the principal that hold the CSP. Once we flip the pref, that CSP inheritance happens within the scriptsecuritymanager.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_test_data_csp.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

origin attributes are very different things comparing to CSP.
Do we inherit CSP to other 3rd party origins?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

ah, I see the spec link.
But I don't understand why inheritance for data: is done in different places than other csp handling.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

You don't test window.open("data:"...);

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8888312 [details] [diff] [review]
bug_1381761_treating_data_as_opaque_should_inherit_csp.patch

Doesn't this inherit csp also to new top level browsing contexts (assuming those are still enabled).
Why is this csp handling in different place than the one related to srcdoc
(that stuff is in GetChannelResultPrincipal) ?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #11)
> Doesn't this inherit csp also to new top level browsing contexts (assuming
> those are still enabled).

Yes, it does. Apparently that is what we want. E.g. looking at test browser_911547.js, which loads [1], which loads the data: URI as a TYPE_DOCUMENT load.

> Why is this csp handling in different place than the one related to srcdoc
> (that stuff is in GetChannelResultPrincipal) ?

The code in GetChanneLResultPrincipal() [1] is not executed for regular data: URI iframe loads. That code is only executed for sandboxed loads - in other words only if the 'sandbox' flags are explicitly set.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_911547_sample.html#16
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#295-328

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Yes, it does. Apparently that is what we want. E.g. looking at test
> browser_911547.js, which loads [1], which loads the data: URI as a
> TYPE_DOCUMENT load.
But that is not what the spec says.


> The code in GetChanneLResultPrincipal() [1] is not executed for regular
> data: URI iframe loads. That code is only executed for sandboxed loads - in
> other words only if the 'sandbox' flags are explicitly set.
Sure, but that doesn't explain why we do csp inheritance stuff in two different methods

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #13)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> > Yes, it does. Apparently that is what we want. E.g. looking at test
> > browser_911547.js, which loads [1], which loads the data: URI as a
> > TYPE_DOCUMENT load.
> But that is not what the spec says.

Dan, I had a chat with smaug on IRC and we both think test browser_911547.js is wrong. Reason is, that the CSP should not be inherited into toplevel data: URI navigations. That's how Chrome used to behave before they started blocking toplevel data: URI navigations. Question now is, what do we do with that test?

Please note that I added an iframe data: URI test (see attached patch) to make sure we inherit the CSP in the TYPE_SUBDOCUMENT case.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8887888 [details] [diff] [review]
bug_1381761_test_browser_911547.patch

># HG changeset patch
># User Christoph Kerschbaumer <[e-mail]>
># Date 1500459181 -7200
>#      Wed Jul 19 12:13:01 2017 +0200
># Node ID 436973f980323ff0c89f2dd411623c6cefca37b3
># Parent  0e205ab88caa99bf66aa6889bfd24a6895f703ca
>Bug 1381761 - Convert test browser_911547.js to comply with new data: URI inheritance model. r=dveditz
>
>diff --git a/browser/components/sessionstore/test/browser_911547.js b/browser/components/sessionstore/test/browser_911547.js
>--- a/browser/components/sessionstore/test/browser_911547.js
>+++ b/browser/components/sessionstore/test/browser_911547.js
>@@ -1,56 +1,77 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
>-// This tests that session restore component does restore the right content
>-// security policy with the document.
>-// The policy being tested disallows inline scripts
>+// This test is two fold:
>+// a) if security.data_uri.unique_opaque_origin == false, then
>+//    this tests that session restore component does restore the right
>+//    content security policy with the document. (The policy being
>+//    tested disallows inline scripts).
>+// b) if security.data_uri.unique_opaque_origin == true, then
>+//    this tests that data: URIs do not inherit the CSP from
>+//    it's enclosing context.
> 
> add_task(async function test() {
>+  let dataURIPref = Services.prefs.getBoolPref("security.data_uri.unique_opaque_origin");
>   // create a tab that has a CSP
>   let testURL = "http://mochi.test:8888/browser/browser/components/sessionstore/test/browser_911547_sample.html";
>   let tab = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, testURL);
>   gBrowser.selectedTab = tab;
> 
>   let browser = tab.linkedBrowser;
>   await promiseBrowserLoaded(browser);
> 
>   // this is a baseline to ensure CSP is active
>   // attempt to inject and run a script via inline (pre-restore, allowed)
>-  await injectInlineScript(browser, `document.getElementById("test_id").value = "fail";`);
>+  await injectInlineScript(browser, `document.getElementById("test_id1").value = "id1_modified";`);
> 
>   let loadedPromise = promiseBrowserLoaded(browser);
>   await ContentTask.spawn(browser, null, function() {
>-    is(content.document.getElementById("test_id").value, "ok",
>+    is(content.document.getElementById("test_id1").value, "id1_initial",
>        "CSP should block the inline script that modifies test_id");
> 
>-    // attempt to click a link to a data: URI (will inherit the CSP of the
>-    // origin document) and navigate to the data URI in the link.
>+
>+    // (a) if security.data_uri.unique_opaque_origin == false:
>+    //     attempt to click a link to a data: URI (will inherit the CSP of
>+    //     the origin document) and navigate to the data URI in the link.
>+    // (b) if security.data_uri.unique_opaque_origin == true:
>+    //     attempt to click a link to a data: URI (will *not* inherit the CSP of
>+    //     the origin document) and navigate to the data URI in the link.
>     content.document.getElementById("test_data_link").click();
>   });
> 
>   await loadedPromise;
> 
>-  await ContentTask.spawn(browser, null, function() {
>-    is(content.document.getElementById("test_id2").value, "ok",
>-       "CSP should block the script loaded by the clicked data URI");
>+  await ContentTask.spawn(browser, {dataURIPref}, function( {dataURIPref}) {
>+    if (dataURIPref) {
>+      is(content.document.getElementById("test_id2").value, "id2_modified",
>+         "data: URI should *not* inherit the CSP of the enclosing context");
>+    } else {
>+      is(content.document.getElementById("test_id2").value, "id2_initial",
>+        "CSP should block the script loaded by the clicked data URI");
>+    }
>   });
> 
>   // close the tab
>   await promiseRemoveTab(tab);
> 
>   // open new tab and recover the state
>   tab = ss.undoCloseTab(window, 0);
>   await promiseTabRestored(tab);
>   browser = tab.linkedBrowser;
> 
>-  await ContentTask.spawn(browser, null, function() {
>-    is(content.document.getElementById("test_id2").value, "ok",
>-       "CSP should block the script loaded by the clicked data URI after restore");
>+  await ContentTask.spawn(browser, {dataURIPref}, function({dataURIPref}) {
>+    if (dataURIPref) {
>+      is(content.document.getElementById("test_id2").value, "id2_modified",
>+         "data: URI should *not* inherit the CSP of the enclosing context");
>+    } else {
>+      is(content.document.getElementById("test_id2").value, "id2_initial",
>+        "CSP should block the script loaded by the clicked data URI after restore");
>+    }
>   });
> 
>   // clean up
>   gBrowser.removeTab(tab);
> });
> 
> // injects an inline script element (with a text body)
> function injectInlineScript(browser, scriptText) {
>diff --git a/browser/components/sessionstore/test/browser_911547_sample.html b/browser/components/sessionstore/test/browser_911547_sample.html
>--- a/browser/components/sessionstore/test/browser_911547_sample.html
>+++ b/browser/components/sessionstore/test/browser_911547_sample.html
>@@ -3,17 +3,17 @@
>   <head>
>     <meta charset="utf-8">
>     <title>Test 911547</title>
>   </head>
> <body>
> 
>   <!--
>    this element gets modified by an injected script;
>-   that script should be blocked by CSP.
>+   that script should be blocked by CSP if security.data_uri.unique_opaque_origin == false;
>    Inline scripts can modify it, but not data uris.
>   -->
>-  <input type="text" id="test_id" value="ok">
>+  <input type="text" id="test_id1" value="id1_initial">
> 
>-  <a id="test_data_link" href="data:text/html;charset=utf-8,<input type='text' id='test_id2' value='ok'/> <script>document.getElementById('test_id2').value = 'fail';</script>">Test Link</a>
>+  <a id="test_data_link" href="data:text/html;charset=utf-8,<input type='text' id='test_id2' value='id2_initial'/> <script>document.getElementById('test_id2').value = 'id2_modified';</script>">Test Link</a>
> 
> </body>
> </html>

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_treating_data_as_opaque_should_inherit_csp.patch

Dan, Smaug, the reason we need the same code in two logical places is that about:srcdoc loads using the sandbox flag within docshell. Hence, when calling GetChannelResultPrincipal, we enter
>  if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
which we don't enter for a regular data: URI load. For the data: URI load of type sub_document, we then fall through to GetChannelURIPrincipal() and create a new nullPrincipal there. This patch is the best I can come up with so we can reuse the same logic.

With this patch applied:
* test_iframe_sandbox_srcdoc.html
* test_data_csp_inheritance.html
* browser_911547.js

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8888474 [details] [diff] [review]
bug_1381761_treating_data_as_opaque_should_inherit_csp.patch

I still don't understand why nsScriptSecurityManager::GetChannelResultPrincipal deals with inheritance for srcdoc and GetChannelURIPrincipal for data:.
Those methods are called in different places.
Do all the callers of GetChannelURIPrincipal expect that srcdoc doesn't inherit csp, but data: does?
Is srcdoc handling perhaps in wrong method?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1381761_treating_data_as_opaque_should_inherit_csp.patch

Smaug, I took a closer look and (as discussed over IRC) I agree that we should add the inherit CSP code only within GetChannelResultPrincipal. The premise of GetChannelResultPrincipal() is that it's called whenever we need the security context of the resource that this channel is loading. In other words, sandboxed loads are only considered within GetChannelResultPrincipal(), but not within GetChannelURIPrincipal().

I added an assertion in the code to make sure we only inherit the CSP to an actual nullPrincipal (e.g. data: when relying on the new inheritance model our about:srcdoc).

What do you think?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=499965180d11ac43edc1e55920f3ca2f0fed2f59

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adf6435ac760
Treating 'data:' documents as unique, opaque origins should still inherit the CSP. r=smaug,dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d497eb5a7b
Convert test browser_911547.js to comply with new data: URI inheritance model. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5767a231874
Test data: URIs inherit the CSP even if treated as unique, opaque origins. r=dveditz

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/adf6435ac760
https://hg.mozilla.org/mozilla-central/rev/a9d497eb5a7b
https://hg.mozilla.org/mozilla-central/rev/a5767a231874