1358009 - (CVE-2017-7834) Content-Security-Policy Bypass with data: URL opened in a new tab

Status: RESOLVED FIXED

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

Description

[Jordi Chancel]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

The Content-Security-Policy can be bypassed using a data: URL opened in a new tab.

Content-Security-Policy doesn't allow javascript execution code on the webpage which uses this security, but in Firefox, if a data: URL is loaded in a new tab, the javascript code contained into this data URL is executed and it can steal the cookie of the website (and others malicious actions).

example:
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'unsafe-eval'">
<a href=" data:text/html;html,<script>alert(document.cookie);</script>">Open Me in a new tab</a>



Actual results:

The Content-Security-Policy can be bypassed if a data URL is opened into a new tab.


Expected results:

In Mozilla Firefox, if a Webpage uses the Content-Security-Policy security, javascript code shouldn't be executed with a data: URL opened in a new tab.

Comment 1

[Jordi Chancel]

Attachment: testcase2.html

This testcase is the same that the first testcase uploaded but it generates also a test cookie.

Comment 2

[Jordi Chancel]

Attachment: Video-Example1.html

Comment 3

[:Gijs (he/him)]

Ben, can you investigate what's happening here?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I think this is more of Christoph's area.  I also think he has some work-in-progress to forbid top level data URLs.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Jordi, thanks for filing the bug. In Firefox, data: URIs inherit the security context of the enclosing document, which is different than in all other browser. Over the years we have accumulated multiple bug reports that data URIs can be used for XSS (see Bug 255107). The only reason we haven't changed that until now was, that Firefox was the only browser that implemented the data: URI behavior according to the spec. Recently the spec changed, so that data: URIs should not inherit the security context of the enclosing doucment, hence we are about to change that behavior within Bug 1324406.

If you try your testcase (using a simple alert instead of document.cookie) you will see that all other browsers will prompt the alert, right? Within Firefox, if you simply click the link in your testcase, then Firefox will block the load with the error:

> Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src 'unsafe-eval'”). Source: alert('foo');.

If you right click, open-link-in-new-tab the data: URI link, then I think the problem is that we don't serialize the CSP together with the principal between child and parent, loose the CSP and hence prompt the alert.

Comment 6

[Daniel Veditz [:dveditz]]

Christoph: if we didn't inherit the origin then fine, we also don't inherit the CSP. But if we're inheriting the origin, and the CSP is embedded in the principal (maybe it isn't anymore?) then why doesn't the new document get the CSP? I thought that was one of the points of putting the CSP in the principal. What other "inherit" cases are also broken?

When does this break? The user ctrl-clicks? what if there's a target=_blank to trigger the new page? That would be a more worrying scenario since it's only normal user interaction.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> Christoph: if we didn't inherit the origin then fine, we also don't inherit
> the CSP. But if we're inheriting the origin, and the CSP is embedded in the
> principal (maybe it isn't anymore?) then why doesn't the new document get
> the CSP?

The CSP is still attached to the principal, but as explained within comment 5, when performing e.g. a right-click->open-link-in-new-tab, we do a bunch of front end checks. Amongst others we check whether the page should load in the child or the parent. Currently we don't serialize the CSP between child and parent when serializing a principal and hence we loose it. I think this is what's happening, but obviously needs more investigation.

> When does this break?

I did a bunch of quick checks:
*) regular click - CSP blocks
*) ctrl-click - alert prompted
*) right-click->open-link-in-new-tab - alert prompted
*) right-click->open-link-in-new-window - alert prompted
*) <a href="data..." target="_blank" ...> - CSP blocks

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

After some debugging I finally located the exact problem [1]. As already suspected we serialize the principal but we don't serialize the CSP within the principal. I remember we already had a discussion around that a while ago and I don't think there is an easy solution to that problem. Gijs, any ideas?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#266

Comment 9

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> After some debugging I finally located the exact problem [1]. As already
> suspected we serialize the principal but we don't serialize the CSP within
> the principal. I remember we already had a discussion around that a while
> ago and I don't think there is an easy solution to that problem. Gijs, any
> ideas?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.
> js#266

I either wasn't involved in the discussion or just flat out don't remember it (sorry!). Why can't we serialize the principal? Also, that's browser-child.js - does this mean the problem doesn't reproduce with e10s disabled?

Comment 10

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> > When does this break?
> 
> I did a bunch of quick checks:
> *) ctrl-click - alert prompted
> *) <a href="data..." target="_blank" ...> - CSP blocks

If the _blank case works, even for new windows (turn off "open new windows in a new tab instead") we should be able to make the ctrl-click and context menu cases work without too much craziness.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #9)
> I either wasn't involved in the discussion or just flat out don't remember
> it (sorry!). Why can't we serialize the principal? Also, that's
> browser-child.js - does this mean the problem doesn't reproduce with e10s
> disabled?

We can serialize the principal itself - I think the problem is that we don't serialize the CSP within the principal. In other words we loose any CSP attached to the principal during serialization/deserialization. But I just tried with e10s prefed off, and I get the same result, so the problem must be somewhere else.

But somewhere along the road we must loose the CSP, and as far as I remember that could only happen when we serialize/deserialize. The other thing that came to my mind was that we don't pass the right principal (the one with the CSP) to the docshell load, so that then within docshell we create a CSP from the referrer. But that's also not the case - within LoadURI() we already get a valid (actually the correct) triggeringPrincipal (just without the CSP attached).

Mhm, I guess this needs more debugging to find out where we actually loose the CSP - beats me.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Gijs, one problem I already identified (even though it's not the root cause of the problem here) is the fact that we are potentially generating a new codebasePrincipal using the right OA [1], but not resetting any CSP on the old principal. Looking at the stacktrace underneath, do you think we are using the wrong principal within openLinkInTab to begin with? Because this principal is the one that is passed all the way to be set on the loadinfo.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/76b5194257a9#l4.36

0 _loadURIWithFlags(browser = [object XULElement], uri = "data:text/html;html,<script>alert('foo');</script>", params = [object Object]) ["chrome://browser/content/browser.js":1011]
    this = [object ChromeWindow]
1 loadURIWithFlags(aURI = "data:text/html;html,<script>alert('foo');</script>", aFlags = [object Object]) ["chrome://browser/content/tabbrowser.xml":7787]
    this = [object XULElement]
2 addTab(aURI = "data:text/html;html,<script>alert('foo');</script>", aReferrerURI = [xpconnect wrapped nsIURI @ 0x7fffd651aa60 (native @ 0x7fffc1605458)]) ["chrome://browser/content/tabbrowser.xml":2405]
    this = [object XULElement]
3 loadOneTab(aURI = "data:text/html;html,<script>alert('foo');</script>", aReferrerURI = [xpconnect wrapped nsIURI @ 0x7fffd651aa60 (native @ 0x7fffc1605458)]) ["chrome://browser/content/tabbrowser.xml":1522]
    this = [object XULElement]
4 openLinkIn(url = "data:text/html;html,<script>alert('foo');</script>", where = "tab", params = [object Object]) ["chrome://browser/content/utilityOverlay.js":436]
    this = [object ChromeWindow]
5 openLinkInTab(event = [object XULCommandEvent]) ["chrome://browser/content/nsContextMenu.js":1085]
    this = contextMenu.target     = data:text/html;html,<script>alert('foo');</script>
contextMenu.onImage    = false
contextMenu.onLink     = true
contextMenu.link       = data:text/html;html,<script>alert('foo');</script>
contextMenu.inFrame    = false
contextMenu.hasBGImage = false

6 oncommand(event = [object XULCommandEvent]) ["chrome://browser/content/browser.xul":1]
    this = [object XULElement]

Comment 13

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Gijs, one problem I already identified (even though it's not the root cause
> of the problem here) is the fact that we are potentially generating a new
> codebasePrincipal using the right OA [1], but not resetting any CSP on the
> old principal.

I'm not sure I understand. We should be setting the same CSP on the *new* principal as on the *old* principal, AIUI. We can't reuse the principal because that breaks opening inheriting-principal-links in containers. That we're not copying the csp is definitely a bug here, though I don't see any way to actually do this. Like, nsIPrincipal just doesn't have any scriptable APIs that allow us to set the CSP. :-\

But yeah, even if we fixed that this would still be broken in the e10s case because of the serialization. Given that we can already generate a JSON representation of the CSP, presumably this is fixable?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_data_csp_bypass_quickfix.patch

(In reply to :Gijs from comment #13)
> I'm not sure I understand. We should be setting the same CSP on the *new*
> principal as on the *old* principal, AIUI. We can't reuse the principal
> because that breaks opening inheriting-principal-links in containers. That
> we're not copying the csp is definitely a bug here, though I don't see any
> way to actually do this. Like, nsIPrincipal just doesn't have any scriptable
> APIs that allow us to set the CSP. :-\

Sorry, for not being precise. What I meant is that we have to set the same CSP on the new principal that was present on the old principal. Have a look at the attached patch and you know what I mean :-) One of the problems though is that csp is not scriptable. So, I think what we could do is expose a function on nsIPrincipal 'generateNewPrincipalFromOldWithNewArgs()' or something that allows us to pass in the OA we want and just return a new principal (including the CSP). Basically move useOAForPrincipal on nsIPrincipal and implement it in c++. That would work right?

The attached patch fixes the problem for all *non* e10s cases for the tests on comment 7.

> But yeah, even if we fixed that this would still be broken in the e10s case
> because of the serialization. Given that we can already generate a JSON
> representation of the CSP, presumably this is fixable?

The serialization of the CSP is a real problem. When first parsing the CSP we generate an object representation of the CSP (see nsCSPUtils.h). Serializing that object representation seems hard and reparsing the CSP string also seems like a non optimal solution. I have written a bunch of serialization code for principals, but serializing the CSP definitely seems like the hardest. Any suggestions on how to fix that problem?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

What makes the problem worse is, that CSP is an active standard which is extended on a regular basis. In terms of maintenance, we would always have to update the serialization code in addition to any actual CSP changes. Keeping that in sync seems error prone on top of the actual serialization work.

Comment 16

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> What makes the problem worse is, that CSP is an active standard which is
> extended on a regular basis. In terms of maintenance, we would always have
> to update the serialization code in addition to any actual CSP changes.
> Keeping that in sync seems error prone on top of the actual serialization
> work.

How is the cspJSON code implemented? That seems like it'd be sufficient, generally speaking...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> So, I think what we could do is expose
> a function on nsIPrincipal 'generateNewPrincipalFromOldWithNewArgs()' or
> something that allows us to pass in the OA we want and just return a new
> principal (including the CSP). Basically move useOAForPrincipal on
> nsIPrincipal and implement it in c++. That would work right?

Yes, that should work, though the implementation will still return undefined results (probably just the principal you pass in) for null principals or system principals. :-)

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #16)
> How is the cspJSON code implemented? That seems like it'd be sufficient,
> generally speaking...

That could work actually, we would just need to implement the inverse part for cspToJSON - so the part that generates a CSP from the json. I'll give that a try, but it might take some time since there are also other things I am working on at the moment.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_cloneprincipal_with_updated_oa.patch

Within this patch I am only fixing the *non* e10s parts. If you prefer, we could file a new bug and land this patch there so that this bug only contains the e10s changes. What do you think?

Anyway, I verified this changeset fixes all the testcases described within comment 7 and further that the automated test browser_oa_private_browsing_window.js still works, which is the test we introduced with the initial useOAForPrincipal() function implemenetation (See Bug 1348801).

Comment 19

[:Gijs (he/him)]

Comment on attachment 8860928 [details] [diff] [review]
bug_1358009_cloneprincipal_with_updated_oa.patch

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

The utilityOverlay.js changes look OK to me, but someone else should review the nsScriptSecurityManager changes.

For e10s, I don't mind much about whether we use this bug or another one, but we'll need to ship everything at the same time, I expect... (or be very circumspect about why we're making the changes in the other bug and not link it to this one) to be fair, this is a sec-moderate with significant user interaction, so I guess this might be OK.

Finally, I am still confused about how the e10s case works with target=_blank. The thing that's creating the about:blank content viewer there must be doing so with a principal with CSP. But I'm pretty sure that the "new window" case for target=_blank passes through the parent process (because we have to create a new browser window first), so I don't understand how we can be using an un-serialized/deserialized principal which has CSP.

In theory it should be possible to pass the principal around on the content process side only, but I don't know if that's feasible in practice - it would certainly require that we reuse the same content process for the "open link in new tab" case where we have items that inherit principals.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #19)
> For e10s, I don't mind much about whether we use this bug or another one,
> but we'll need to ship everything at the same time, I expect...

In the end it doesn't really matter I guess. So let's just wait till we have all the code ready within this patch and then get it landed alltogether.
 
> Finally, I am still confused about how the e10s case works with
> target=_blank. The thing that's creating the about:blank content viewer
> there must be doing so with a principal with CSP.

The target="_blank" uses a different codepatch then 'right-click->open-link-in-new-tab' and hence ends up with a CSP on the principal within docshell. When regular clicking a link we use the principal from nsContentUtils.cpp:5300 (see stacktrace underneath) which is then passed all the way to docshell which then blocks the load here:
  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9954


0x00007fffe8503075 in nsDocShell::OnLinkClick (this=0x7fffd3922000, aContent=0x7fffc7e54b80, aURI=0x7fffc716ad30, aTargetSpec=0x7fffc7c45348 u"_blank", aFileName=<gNullChar> u"", 
    aPostDataStream=0x0, aHeadersDataStream=0x0, aIsTrusted=true, aTriggeringPrincipal=0x7fffc7d9a920) at /home/ckerschb/source/mc/docshell/base/nsDocShell.cpp:14055
#8  0x00007fffe4715e29 in nsContentUtils::TriggerLink (aContent=0x7fffc7e54b80, aPresContext=0x7fffc7c33800, aLinkURI=0x7fffc716ad30, aTargetSpec=u"_blank", aClick=true, 
    aIsUserTriggered=true, aIsTrusted=true) at /home/ckerschb/source/mc/dom/base/nsContentUtils.cpp:5300

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

In general, I think we should try and fix the CSP serialization-problem on the pricnipal, because if not in this bug, then that serialization problem will come up again in the near future.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> In general, I think we should try and fix the CSP serialization-problem on
> the pricnipal, because if not in this bug, then that serialization problem
> will come up again in the near future.

If we do this I think we need to be careful that we don't end up propagating the CSP on the principal to places that shouldn't have the propagation.  Code may be relying on the serialization to filter it out without realizing it.  For example, I had a lot of trouble with this in worker code recently.

I would really love if we could move CSP off the principal as discussed in bug 965637.  Probably can't do it here, of course.  FWIW, though, I'm making something that can function as the "document settings object" over in bug 1293277 so it might be more possible after that lands.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #22)
> If we do this I think we need to be careful that we don't end up propagating
> the CSP on the principal to places that shouldn't have the propagation. 
> Code may be relying on the serialization to filter it out without realizing
> it.  For example, I had a lot of trouble with this in worker code recently.

What cases are those? I am not entirely sure but I would think that whenever we inherit the principal we should also inherit the CSP as well, no?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_test_data_inheritance.patch

Gijs, I am making on step at a time here. First I would like to generate the automated test to make sure we never regress that again. When applying the first patch from this bug, in other words when calling clonePrincipalWithUpdatedOA() and when running this test using: ./mach mochitest --disable-e10s .../browser_data_load_inherit_csp.js then it works.

When running with e10s disabled, then only the first test (the regular click) succeeds, but the other two (where we don't serialize the CSP on the principal) fail.

Ultimately I would like to remove the code duplication for checking the CSP on each subtest. I am pretty sure you can tell me how to do that, right?

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)
> What cases are those? I am not entirely sure but I would think that whenever
> we inherit the principal we should also inherit the CSP as well, no?

How is "inherit the principal" defined in code?  We pass nsIPrincipal* around all over the place, but that does not equate to inheriting the principal.  As an example, service workers are constrained to an origin, but the service worker itself should not inherit CSP from the document that registered the service worker.  Magical passing of CSP around in our origin identity type makes this really error prone.  Dedicated workers had the same problem for a long time as well.

Anyway, we can debate this over in bug 1293277.  Please just don't break any of the worker stuff I just spent a bunch of time fixing.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8861393 [details] [diff] [review]
bug_1358009_test_data_inheritance.patch

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

::: docshell/test/browser/browser_data_load_inherit_csp.js
@@ +1,5 @@
> +"use strict";
> +
> +const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
> +const HTML_PAGE = TEST_PATH + "file_data_load_inherit_csp.html";
> +const DATA_PAGE = "data:text/html;html,<html><body>foo</body></html>";

Nit: to avoid these getting out of sync, maybe set the attribute on the link from inside the test (ie this file) rather than having a copy in the test helper file (the html one) ?

@@ +6,5 @@
> +
> +add_task(function* test_data_csp_inheritance_regular_click() {
> +  yield BrowserTestUtils.withNewTab(HTML_PAGE, function*(browser) {
> +    let loadPromise = BrowserTestUtils.browserLoaded(browser, false, DATA_PAGE);
> +    let myLink = browser.contentDocument.getElementById("testlink");

You probably want to do the clicking inside the ContentTask as well, or use BrowserTestUtils.synthesizeMouseAtCenter() here too.

@@ +10,5 @@
> +    let myLink = browser.contentDocument.getElementById("testlink");
> +    myLink.click();
> +    yield loadPromise;
> +
> +    yield ContentTask.spawn(gBrowser.selectedBrowser, { DATA_PAGE }, function* ({DATA_PAGE}) {

To de-dupe these, you can simply declare the content task function at the toplevel and use that named function (passing it as an argument) instead. Same thing for assigning the data: URL if you wanted to do that, too.

@@ +20,5 @@
> +      let policies = cspOBJ["csp-policies"];
> +      is(policies.length, 1, "should be one policy");
> +      let policy = policies[0];
> +      dump("policy['default-src']: " + policy['default-src'] + "\n\n");
> +      dump("policy['script-src']: " + policy['script-src'] + "\n\n");

You probably want to remove the dump()s or replace them with info(). :-)

::: docshell/test/browser/file_data_load_inherit_csp.html^headers^
@@ +1,1 @@
> +Content-Security-Policy: default-src 'none'; script-src 'unsafe-inline';

Isn't this supposed to work as a <meta> tag, too? That seems simpler...

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_test_data_inheritance.patch

Gijs, thanks for the hints. I updated the following things:
* de-duped the data: URI by setting it from within the mainfile
* Using BrowserTestUtils.synthesizeMouseAtCenter() for the regular click() case
* de-duped the CSP verification code
* removed the dumps :-)
* using meta-csp instead of header CSP

Comment 28

[:Gijs (he/him)]

Comment on attachment 8861891 [details] [diff] [review]
bug_1358009_test_data_inheritance.patch

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

::: docshell/test/browser/browser_data_load_inherit_csp.js
@@ +3,5 @@
> +const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
> +const HTML_URI = TEST_PATH + "file_data_load_inherit_csp.html";
> +const DATA_URI = "data:text/html;html,<html><body>foo</body></html>";
> +
> +let verifyCSP = function(aTestName, aBrowser, aDataURI) {

Nit: just:

function verifyCSP(...) {
...
}

works.

@@ +22,5 @@
> +  yield BrowserTestUtils.withNewTab(HTML_URI, function*(browser) {
> +    let loadPromise = BrowserTestUtils.browserLoaded(browser, false, DATA_URI);
> +    // set the data href + simulate click
> +    let myLink = browser.contentDocument.getElementById("testlink");
> +    myLink.href = DATA_URI;

This is still using a CPOW. You'll want to use a ContentTask for this, here and in the other tests (which you may or may not want to abstract into another helper function):

yield ContentTask.spawn(browser, DATA_URI, function(uri) {
  let link = content.document.getElementById("testlink");
  link.href = uri;
});

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_test_data_inheritance.patch

(In reply to :Gijs from comment #28)
> @@ +22,5 @@
> > +    myLink.href = DATA_URI;
> 
> This is still using a CPOW.

Indeed - updated - thanks; carrying over r+.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8860928 [details] [diff] [review]
bug_1358009_cloneprincipal_with_updated_oa.patch

Boris, I chatted with bholley and he thinks you are the better reviewer for this stuff. Within Bug 1348801 we had to teach the principal about OriginAttributes, but we didn't propagate the CSP from the old principal on to the newly created one. The fact that we don't expose the CSP to JS caused me to expose that functionality on the scriptSecurityManager. Gijs already took a first look (comment 19). I verified that browser_oa_private_browsing_window.js (which we wrote for Bug 1348801) still works.

Further I verified that with this patch the newly crafted test for this bug (docshell/test/browser/browser_data_load_inherit_csp.js) works when e10s is disabled. The reason it doesn't work with e10s yet, is because we serialize the principal when ctrl-clicking but we don't serialize the CSP on the principal.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1358009_serialize_csp_within_principal.patch

Boris, here is a first dummy implementation of the CSP serialization. I wanted to make sure you are onboard with that. Or would we be better off moving the CSP somewhere else?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8860928 [details] [diff] [review]
bug_1358009_cloneprincipal_with_updated_oa.patch

>+  aPrincipal = ssm.clonePrincipalWithUpdatedOA(aPrincipal, attrs);
>+  aTriggeringPrincipal = ssm.clonePrincipalWithUpdatedOA(aTriggeringPrincipal, attrs);

This is kinda preexisting, but in the case when aPrincipal == aTriggeringPrincipal this will break the object identity of the two.  Which is mostly a problem for nullprincipals, since those care about object identity...

It might make sense to check for the aPrincipal == aTriggeringPrincipal case and only do the clone once then.

>+++ b/caps/nsIScriptSecurityManager.idl
>+     * Clone a Principal but teach it about the right OA to use, e.g. in case

s/a Principal/a principal/

>+     * Please note that this function only returns a new principal
>+     * in case principalToClone is a codebasePrincipal.

I don't think that's the right behavior, given that nullprincipals can have OA and CSP.  Yes, I know that's what the frontend code you're replacing did.  It was wrong.

I think what would make the most sense is that this function returns its argument if the originAttributes already match.  If not, we clone; that includes cloning for nullprincipals.  That will break the security-check object identity, but that should be ok in cases when the originAttributes don't match, because things with non-matching originAttributes should not be able to reach each other anyway.  I'm not sure how that should be reflected in the naming of the function; maybe ensureOriginAttributesForPrincipal?

>+++ b/caps/nsScriptSecurityManager.cpp
>+  if (!aPrincipalToClone) {
>+    return NS_OK;

That returns a random value.  You need to set *aPrincipal to null.

Comment 33

[Boris Zbarsky [:bzbarsky]]

For the needinfo, we may want move the CSP somewhere else in any case, because it's not inherited in quite the same set of cases as origin inheritance.  For example, consider this testcase:

  <iframe srcdoc="stuff" sandbox"></iframe>

Per spec, this inherits the CSP from the parent but does NOT inherit the principal.  I'm fairly sure we get this wrong right now.

Similarly, per current spec a javascript: navigation inherits the principal but NOT the CSP.  I filed https://github.com/whatwg/html/issues/2589 but it's not clear how that will get resolved.  Same for about:blank; filed <https://github.com/whatwg/html/issues/2592>.  And blobs; filed https://github.com/whatwg/html/issues/2593

And what bkelly said about workers: they can inherit origins, but not CSP....

Comment 34

[Frederik Braun [:freddy]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #33)
> For the needinfo, we may want move the CSP somewhere else in any case,
> because it's not inherited in quite the same set of cases as origin
> inheritance.  For example, consider this testcase:
> 
>   <iframe srcdoc="stuff" sandbox"></iframe>
> 
> Per spec, this inherits the CSP from the parent but does NOT inherit the
> principal.  I'm fairly sure we get this wrong right now.
> 

This particular issue was changed in bug 1073952, about a month ago.
The key change is that we create a NullPrincipal for sandboxed-loads but set the CSP if we are in a srcdoc.
That being said, it doesn't mean we get CSP inheritance right in the other cases you mentioned and I agree that it should not live on the principal.
(This would also help setting a CSP on privileged chrome pages, that share the same principal.)

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #32)

Boris, thanks for the notes. In fact, I agree to everything you said now that I think about it. The good news is that your comments are more or less easy to address once we figure a path forward with this bug. In fact, I think we can take that patch and fix that particular problem outside of this bug because those changes are orthogonal to the Principal/CSP inheritance problem we try to solve here.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #33)
> For the needinfo, we may want move the CSP somewhere else in any case,
> because it's not inherited in quite the same set of cases as origin
> inheritance.

Yes, I agree. There are quite so many cases where we should inherit the principal but not the CSP and the other way round. One of those edge cases we addressed within Bug 1073952, but I am sure there are more of those. The big question is, where should the CSP live? On the nsILoadInfo? But even if we move the CSP somewhere else I guess we still might have to serialize the CSP for some cases, but we could figure those cases along the way. The big question for now, where could the CSP live if not on the principal?

Comment 36

[Jordi Chancel]

A middle Click can also Bypass the Content-Security-Policy.

Comment 37

[Jordi Chancel]

This vulnerability is RESOVLED/FIXED in Firefox Quantum 57.0b3.

can you change the statut as RESOLVED/FIXED?

Comment 38

[Frederik Braun [:freddy]]

NB, this was likely fixed via bug 1381761.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Jordi, you're saying this bug no longer reproduces in the beta 3 Firefox 57 build?

Comment 40

[Jordi Chancel]

Yes, in Firefox 57b3 this bug is fixed.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #38)
> NB, this was likely fixed via bug 1381761.

Yes, this one was fixed by bug 1381761. Setting this to 'invalid', but could also live with 'worksforme'.

Comment 42

[Daniel Veditz [:dveditz]]

"invalid" means "invalid"--it's not a problem. If we actually _fixed_ this then that's a terrible resolution. "worksforme" is normally fine, but since this is a security bug it's usually worth figuring out what fixed it so we know it's not accidental or a side-effect that might go away in the future.

If we know exactly what fixed it then "fixed" is the right resolution (depends on the bug that fixed it). Especially true if there's a bounty nomination on the bug.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> If we know exactly what fixed it

WE do.  See comment 38 and comment 41.