Closed Bug 1381761 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 files, 3 obsolete files)

No description provided.
Assignee: nobody → ckerschb
Blocks: 1337268
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
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?
Flags: needinfo?(bugs)
Right, assuming CSP spec doesn't have any special cases for data: handling.
Flags: needinfo?(bugs)
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!
Attachment #8887430 - Attachment is obsolete: true
Attachment #8887888 - Flags: review?(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 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
Attachment #8887888 - Flags: review?(dveditz)
Blocks: 1324406
No longer blocks: 1337268
Summary: Convert test browser_911547.js to comply with new data: URI inheritance model → Treat 'data:' documents as unique, opaque origins should still inherit the CSP
Attachment #8887888 - Attachment is obsolete: true
Summary: Treat 'data:' documents as unique, opaque origins should still inherit the CSP → Treating 'data:' documents as unique, opaque origins should still inherit the CSP
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.
Attachment #8888312 - Flags: review?(dveditz)
Attachment #8888312 - Flags: review?(bugs)
Attachment #8888313 - Flags: review?(dveditz)
origin attributes are very different things comparing to CSP.
Do we inherit CSP to other 3rd party origins?
ah, I see the spec link.
But I don't understand why inheritance for data: is done in different places than other csp handling.
You don't test window.open("data:"...);
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) ?
Attachment #8888312 - Flags: review?(bugs)
(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
Flags: needinfo?(bugs)
(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
Flags: needinfo?(bugs)
(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 on attachment 8887888 [details] [diff] [review]
bug_1381761_test_browser_911547.patch

># HG changeset patch
># User Christoph Kerschbaumer <ckerschb@christophkerschbaumer.com>
># 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>
Attachment #8887888 - Attachment is obsolete: false
Attachment #8887888 - Flags: review?(dveditz)
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
Attachment #8888312 - Attachment is obsolete: true
Attachment #8888312 - Flags: review?(dveditz)
Attachment #8888474 - Flags: review?(dveditz)
Attachment #8888474 - Flags: review?(bugs)
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?
Attachment #8888474 - Flags: review?(bugs)
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?
Attachment #8888474 - Attachment is obsolete: true
Attachment #8888474 - Flags: review?(dveditz)
Attachment #8889901 - Flags: review?(bugs)
Attachment #8889901 - Flags: review?(bugs) → review+
Blocks: 1386183
Attachment #8888313 - Flags: review?(dveditz) → review+
Attachment #8887888 - Flags: review?(dveditz) → review+
Pushed by mozilla@christophkerschbaumer.com:
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
Depends on: 1387811
You need to log in before you can comment on or make changes to this bug.