1073952 - (CVE-2017-7788) CSP can be bypassed with <iframe [sandbox] and [srcdoc]>

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.124 Safari/537.36

Steps to reproduce:

Open following link.
http://csp.csrf.jp/target.php?body=<a href="javascript:alert(1)">xss</a>

This page has an reflected xss in 'body' parameter in query string but it is protected by CSP "default-src 'self'".
Even if script code is injected via 'body' in URI, it's execution may be prevented by CSP.


Actual results:

If an attacker opens the page in <iframe sandbox> and he/she injects script code with <iframe srcdoc>, injected script is executed with bypassing it's CSP.

Please see the following proof of concept page. alert(1) may be executed even though the page opened in iframe is protected by CSP.
http://mallory.csrf.jp/cspbypass.html



Expected results:

Script execution in http://csp.csrf.jp/target.php has to be protected by this site's CSP.
Note that Google Chrome does prevent execution in same situation.

Comment 1

[Daniel Veditz [:dveditz]]

The csp.csrf.jp policy is "default-src 'self'" and srcdoc is supposed to be self. It should be blocking inline-script, but the error I see on the console is "Content Security Policy: The page's settings blocked the loading of a resource at self (""). about:srcdoc:1" Is that what we report for inline-script?

The inner srcdoc origin is the same as the outer origin so it should be inheriting the CSP along with the origin. (Not saying it does, this is my understanding of what we intend to happen.). I wonder if there's some oddness with sandbox unique origins such as they can't be inherited but instead a new unique origin is created.

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Sid, any input? Is this a limit of what we've implemented of the spec? Or a bug in our implementation?

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

It looks like loading the inner iframe in a non-sandboxed context works, so my gut feeling here is that the sandboxed iframe doesn't enforce CSP.  I'm not sure it is limited to srcdoc, since if you load the iframe by itself in a non-sandboxed environment, CSP seems to properly block things.  I think this is a bug in our implementation.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Or like dan said, maybe the sandboxed iframes don't pass their origins into srcdoc subframes like non-sandboxed ones do.  If that's the case, CSP needs to find the sandboxed iframe's principal for srcdoc subframes.

Comment 5

[Daniel Veditz [:dveditz]]

Calling this sec-low because this is an edge case on CSP (which is already a mitigation against site bugs, but the site has to have a bug first). At worst you get an XSS attack into sandboxed content. Means you can't trust what the content shows any more but it's not running in any context with valuable data.

Comment 6

[Muneaki Nishimura]

CSP level2 spec clarifies expected behavior of <iframe [srcdoc]> as follows.
http://www.w3.org/TR/CSP11/#processing-model-iframe-srcdoc

It seems UA should follow this spec unless protected document is in a sandoxed iframe.

Comment 7

[Muneaki Nishimura]

Typo: unless -> even if

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1073952_csp_iframe_srcdoc.patch

Hey Dan, I was looking at the part of the spec that defines the nested browsing context for iframe srcdoc [1] and wrote this test that verifies that we apply the top level CSP to the nested browsing context within an iframe srcdoc and also to an iframe srcdoc within an iframe srcdoc.

This bug was filed quite some time ago, so potentially something fixed the issue in the mean time. If I am not missing something, then we could land this test and mark the bug as a WORKSFORME.

[1] https://www.w3.org/TR/CSP/#processing-model-iframe-srcdoc

Comment 9

[Daniel Veditz [:dveditz]]

It's not WORKSFORME -- the original PoC still reproduces the bug: http://mallory.csrf.jp/cspbypass.html

In the PoC target.php has a CSP of "default-src 'self'"



blocked: http://csp.csrf.jp/target.php?body=<a href="javascript:alert(1)">xss</a>
blocked: http://csp.csrf.jp/target.php?body=%3Ciframe%20srcdoc=%27%3Cscript%3Ealert(0)%3E%3C/script%3E%27%3E%3C/iframe%3E

Not blocked: putting the above in a sandboxed iframe. Your test doesn't have any sandbox in it.

Comment 14

[Daniel Veditz [:dveditz]]

There are several dupes of this and now an academic paper and a post to the public-webappsec mailing list. We might as well unhide this.
https://lists.w3.org/Archives/Public/public-webappsec/2016Nov/0007.html

Comment 15

[Frederik Braun [:freddy]]

Attachment: 0001-wip.patch

Christoph, this wip patch is pretty small and I've no great confidence this is right, but hopefully will help discussing it further.

You suggested I take the branch that does the sandbox-check for me and then just probe for the srcdoc case. I got so far.

Then you told me to add the CSP to the newly created principal (line 298/299), but I have absolutely no clue how to do that. I did some search into other code paths that may or may not modify/add CSPs, but couldn't find anything useful.
(Among those: EnsureCSP (does not add, but get a CSP), nsDocument::InitCSP (private), CSP_AppendFromHeader (no header here), …)

Can you take a look?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8824419 [details] [diff] [review]
0001-wip.patch

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

Freddy, even though I said GetChannelResultPrincipal is the right place to add that code I took a second look and I think it's better to add the code within nsDocShell where we actually set up the iframe and the loadinfo.

if (isSandBoxed) {
  nsAutoCString spec;
  aUri->GetSpec(spec);
  if (spec.EqualsLiteral("about:srcdoc") {
    if (aPrincipalToInherit) {
        nsCOMPtr<nsIContentSecurityPolicy> csp;
        principalToInherit->GetCsp(getter_AddRefs(csp));
        if (csp) {
          loadingPrincipal->SetCSP(csp);
        } 
    }
  }
}

Your code is actually pretty close to mine and probably mine needs a little debugging as well. Not sure if about:srcdoc is actually passed as the regular URI here or whether you need to do some GetIntialURI or whathever URI modification methods we provide to actually get to the about:srcdoc. And probably we should only do that for TYPE_SUBDOCUMENT, right?

And now I also see your question, there is no SetCSP anymore, but only EnsureCSP(), which only accepts AppendPolicy. In other words we don't have a mechanism on the principal which would allow to actually 'copy' a CSP from one principal to the other.

As a first step you could try to remove the 'readonly' from
[noscript] readonly attribute nsIContentSecurityPolicy csp;
within nsIPrincipal.idl and then implement the setCSP method on the principal which would then allow us to copy the CSP from one principal to the other. If all of our reasoning is correct, we can then try to find a solution to actually copy the CSP between those principals.

[1] http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10884

Comment 17

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inhereit-CSP-into-iframe-sandbox-srcdoc.patch

Looking forward to hearing your feedback.

Comment 18

[Frederik Braun [:freddy]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3970a086952afb05dcc36fbfe0ba42c37c2eff3

Comment 19

[Frederik Braun [:freddy]]

Err https://treeherder.mozilla.org/#/jobs?repo=try&revision=e53364d4001065d0bfc656f437249f12adce5c52

Comment 20

[Frederik Braun [:freddy]]

All tests green except for a some known & unrelated intermittent failures.

Comment 21

[Frederik Braun [:freddy]]

Comment on attachment 8825818 [details] [diff] [review]
0001-Bug-1073952-inhereit-CSP-into-iframe-sandbox-srcdoc.patch

This needs a caps peer review.
Dan, can you take a look?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8825818 [details] [diff] [review]
0001-Bug-1073952-inhereit-CSP-into-iframe-sandbox-srcdoc.patch

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

Overall that already looks pretty good. I would like to see it one more time especially because GetChannelResultPrincipal() is a crucial method in our security system.

::: caps/BasePrincipal.cpp
@@ +504,5 @@
>  NS_IMETHODIMP
> +BasePrincipal::SetCsp(nsIContentSecurityPolicy* aCsp)
> +{
> +  // If CSP was already set, it should not be destroyed!  Instead, it should
> +  // get set anew when a new principal is created.

Please update the comment to:
Never destroy an existing CSP on the principal. This method should only be called in rare cases.

@@ +507,5 @@
> +  // If CSP was already set, it should not be destroyed!  Instead, it should
> +  // get set anew when a new principal is created.
> +  if (mCSP) {
> +    return NS_ERROR_ALREADY_INITIALIZED;
> +    }

MOZ_ASSERT(!mCSP, "do not destroy an existing CSP"):

if (mCSP) {
  return NS_ERROR_ALREADY_INITIALIZED;
}

::: caps/nsIPrincipal.idl
@@ +140,4 @@
>       *
>       * Use this function to query the associated CSP with this principal.
>       */
> +    [noscript] attribute nsIContentSecurityPolicy csp;

Please update the comment on top to:

 * A Content Security Policy associated with this principal.
 * Use this function to query the associated CSP with this principal.
 * Please *only* use this function to *set* a CSP when you know exactly what you are doing. Most likely you want to call ensureCSP instead of setCSP.

::: caps/nsNullPrincipal.cpp
@@ +110,5 @@
>  
>  NS_IMETHODIMP
> +nsNullPrincipal::SetCsp(nsIContentSecurityPolicy* aCsp) {
> +  // If CSP was already set, it should not be destroyed!  Instead, it should
> +  // get set anew when a new principal is created.

Please use the same comment and assertion from BasePrincipal.cpp within this method.

::: caps/nsScriptSecurityManager.cpp
@@ +298,5 @@
> +            if (!principalToInherit) {
> +              principalToInherit = loadInfo->TriggeringPrincipal();
> +            }
> +            nsCOMPtr<nsIContentSecurityPolicy> originalCsp;
> +            principalToInherit->GetCsp(getter_AddRefs(originalCsp));

I think you can move this code down after the NullPrincipal creation, so that everything CSP related is blocked together.

@@ +310,4 @@
>                pAttrs.InheritFromNecko(nAttrs);
>                prin = nsNullPrincipal::Create(pAttrs);
>              }
> +            if (originalCsp) {

Please add a comment explaining what we are doing here.

::: caps/nsSystemPrincipal.cpp
@@ +72,5 @@
>  
>  NS_IMETHODIMP
> +nsSystemPrincipal::SetCsp(nsIContentSecurityPolicy* aCsp)
> +{
> +  // CSP on a system principal makes no sense

please add the assertion here as well, we should never even try to set a CSP on the systemPrincipal.

::: dom/security/nsCSPService.cpp
@@ +73,4 @@
>    if (NS_SUCCEEDED(rv) && match) {
>      return true;
>    }
> +  // about:srcdoc *is* subject to CSP

Generally about: is not subject to CSP, but "about:srcdoc" is.

@@ +77,5 @@
> +  nsAutoCString uriSpec;
> +  aURI->GetSpec(uriSpec);
> +  if (uriSpec.EqualsLiteral("about:srcdoc")) {
> +    return true;
> +  }

That part probably needs to be properly rebased after Bug 1330035 has merged into central.

::: dom/security/test/csp/file_iframe_sandbox_srcdoc.html
@@ +5,5 @@
> +  <title>Bug 1073952 - CSP should restrict scripts in srcdoc iframe even if sandboxed</title>
> +</head>
> +<body>
> +<iframe srcdoc="<img src=x onerror='alert(9);parent.postMessage({result: `unexpected-csp-violation`}, `*`);'>"
> +        sandbox="allow-scripts allow-modals"></iframe>

why do you need the alert(9); I think that will cause trouble, please remove.

::: dom/security/test/csp/test_iframe_sandbox_srcdoc.html
@@ +10,5 @@
> +<p id="display">Bug 1073952</p>
> +<div id="content" style="display: none">
> +
> +
> +</div>

nit: please move ending tag on the same line as starting tag or remove that whole div block.

@@ +27,5 @@
> +
> +    if(topic === "csp-on-violate-policy") {
> +      var violationString = SpecialPowers.getPrivilegedProps(SpecialPowers.
> +                             do_QueryInterface(subject, "nsISupportsCString"), "data");
> +      if (!violationString.includes("Inline Script")) {

Please add a small comment why this works.

@@ +30,5 @@
> +                             do_QueryInterface(subject, "nsISupportsCString"), "data");
> +      if (!violationString.includes("Inline Script")) {
> +        return
> +      }
> +        window.testResult("inline script");

nit: indendation

In general I think you are better off doing the actual assertion here and just have a finish() function which is shared by the two states.
ok(true, "CSP inherited into sandboxed srcdoc iframe, script blocked.");

@@ +51,5 @@
> +}
> +
> +addEventListener("message", function(e) {
> +  ok(false, "We should not execute JS in srcdoc iframe.");
> +  testResult(e.data);

and then also from here you could just call finish().

Comment 23

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc.patch

Next iteration. Please take a look.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8826149 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc.patch

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

close, please just fix the coding style within GetChannelResultPrincipal;
1) add comment explaining what you are doing
2) write all the code that belongs to applying the CSP to the freshly created NullPrincipal within one block.

::: caps/nsScriptSecurityManager.cpp
@@ +314,5 @@
> +            uri->GetSpec(URISpec);
> +            // if sandboxed, about:srcdoc inherits CSP from parent.
> +            if (URISpec.EqualsLiteral("about:srcdoc")) {
> +              nsCOMPtr<nsIContentSecurityPolicy> originalCsp;
> +              principalToInherit->GetCsp(getter_AddRefs(originalCsp));

move the whole querying of the principalToInherit within this if() {} block. No need to query the principalToInherit for non about:srcdoc cases.

::: dom/security/test/csp/test_iframe_sandbox_srcdoc.html
@@ +44,5 @@
> +
> +window.finish = function() {
> +  window.examiner.remove();
> +  SimpleTest.finish();
> +}

just curious: is there any reason you define finish on the window instead of just using
function finish() {}??

Comment 25

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> close, please just fix the coding style within GetChannelResultPrincipal;

Done.

> 1) add comment explaining what you are doing

I had some comments and added some more. I wasn't exactly sure which part you were referring to.

> 2) write all the code that belongs to applying the CSP to the freshly
> created NullPrincipal within one block.

Done.

> ::: caps/nsScriptSecurityManager.cpp
> @@ +314,5 @@
> move the whole querying of the principalToInherit within this if() {} block.
> No need to query the principalToInherit for non about:srcdoc cases.

Done
 
> ::: dom/security/test/csp/test_iframe_sandbox_srcdoc.html
> @@ +44,5 @@
> just curious: is there any reason you define finish on the window instead of
> just using
> function finish() {}??

That was copied from some other test. I changed it.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8826510 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc.patch

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

Code looks good but I think you have to update the commit message before landing.

::: caps/BasePrincipal.cpp
@@ +441,5 @@
> +
> +  MOZ_ASSERT(!mCSP, "do not destroy an existing CSP");
> +  if (mCSP) {
> +    return NS_ERROR_ALREADY_INITIALIZED;
> +    }

nit: indendation, did I missed that the first time around?

Comment 27

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> about:srcdoc. And probably we should only do that for TYPE_SUBDOCUMENT, right?

What about TYPE_DOCUMENT that has a CSP sandbox attribute? Do we have "self" tests for those?

What if the TYPE_DOCUMENT was opened sandboxed from a sandboxed about:srcdoc frame? What if that document is a data: document which inherits the about:srcdoc URL?

Comment 28

[Frederik Braun [:freddy]]

(In reply to Daniel Veditz [:dveditz] from comment #27)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> > about:srcdoc. And probably we should only do that for TYPE_SUBDOCUMENT, right?
> What about TYPE_DOCUMENT that has a CSP sandbox attribute? Do we have "self"
> tests for those?

My current patch adds the CSP to the principal regardless of the load type, as long as it's sandboxed.
We have tests for the CSP sandbox directive in http://searchfox.org/mozilla-central/source/dom/security/test/csp/test_sandbox.html, but they run in a frame (SUBDOCUMENT). But this shouldn't matter given my patch does not look at the load type.

> What if the TYPE_DOCUMENT was opened sandboxed from a sandboxed about:srcdoc
> frame? What if that document is a data: document which inherits the
> about:srcdoc URL?

We have tests for nesting in bug 1330920, I will add more to those interesting cases you mention.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 8826510 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc.patch

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

Ouch -- thought I submitted this Friday. Thank goodness BMO saves drafts automatically!

review below is
 - 2 comment nits
 - concern about giving all about: urls a CSP-free-pass
 - not sure the test adequately covers the actual bug, just checks the one initial symptom

For the test case I suppose it's good enough to file another bug saying we need more tests. Or better, add them to the web-platform tests and ensure all the browsers behave the same way.

I'm on the fence about the about: thing being serious enough to nix this review, but I guess that's a "pre-existing condition" and in America that means "it's not my problem" (or something like that). We're running into problems with URI_IS_LOCAL_RESOURCE in other bugs, too; maybe we should only be giving a free pass to  URI_IS_UI_RESOURCE instead. Some about: URLs would inherit that from their underlying chrome/resource base, and others would not. I think about:srcdoc is one that would not which might be a win here -- you wouldn't have to special case it for whitelisting (though we'd still have to do the special "copy the CSP" bit).

Guess that could go in a new but, too.

r=dveditz

::: caps/nsIPrincipal.idl
@@ +140,3 @@
>       * Use this function to query the associated CSP with this principal.
> +     * Please *only* use this function to *set* a CSP when you know exactly what you are doing.
> +     * Most likely you want to call ensureCSP instead of setCSP.

OK on removing the blank line, but I'd add one before the new stuff to make it stand out more. And pare down the opening (no "please") so the important stuff pops out before people stop paying attention :-)

* DO NOT *set* a CSP unless you know exactly what you are doing (implementing CSP)
* Most likely (etc.)

::: caps/nsSystemPrincipal.cpp
@@ +73,5 @@
>  NS_IMETHODIMP
> +nsSystemPrincipal::SetCsp(nsIContentSecurityPolicy* aCsp)
> +{
> +  // Never destroy an existing CSP on the principal.
> +  // This method should only be called in rare cases.

This is the wrong comment for this case. Maybe "System Principal ignore CSP" or something? Or at least "Cannot set a CSP on the system principal"

::: dom/security/nsCSPService.cpp
@@ +73,4 @@
>    if (NS_SUCCEEDED(rv) && match) {
>      return true;
>    }
> +  // about is not subject to CSP (futher below), but about:srcdoc *is*.

I'm not sure I'm comfortable giving a blanket pass to about: -- there's no guarantee that it's a "safe local" kind of thing, and in fact it can be remote content (like the content-services plan for about:remote-newtab). Who is going to be 1) including about:, 2) using CSP, and 3) can't add about: to the whitelist?

That's a separate issue from special cases for about:srcdoc though.

::: dom/security/test/csp/file_iframe_sandbox_srcdoc.html^headers^
@@ +1,1 @@
> +content-security-policy: default-src *;

This seems an extremely narrow test. The bug was originally concerned about a script bypass (though the content was still sandboxed so it wasn't XSS), but the investigation showed that sandboxed srcdoc got no CSP at all. Shouldn't we test more than just scripts? What if scripts get blocked for some unrelated reason but we don't enforce the content loading rules? Might also want to add a test that we _didn't_ break srcdoc CSP in the non-sandboxed case.

Have you tested this test on a build without the patch so we know that it fails as expected? Sorry for being picky, but I've been burned by tests that turned out not to be testing anything because of some bug in the test, or because some future regression broke the test into a false-passing state.

Comment 30

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

The comments you, Dan, raised were very helpful and fixed a CSP bypass that I would have left in.
This also means that I had to change and increase test coverage to verify this is all working well.

For this reason I am merging bug 1330920 back into this.

Please review the next iteration, you will note that I added a data URI check and removed the changes to CSPService::subjectToCSP, which makes one of your concerns obsolete.

Comment 31

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-tests-for-iframe-sandbox-srcdoc-and-data.patch

This is the test that ckerschb originally wrote in this bug.
It now comes with a data URI test case added (and the sandbox attribute was missing).

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8829841 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

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

Besides GetChannelResultPrincipal (and also nsCSPService) nothing changed right?

::: caps/nsScriptSecurityManager.cpp
@@ +301,5 @@
> +          OriginAttributes attrs;
> +          loadInfo->GetOriginAttributes(&attrs);
> +          attrs.StripAttributes(OriginAttributes::STRIP_ADDON_ID);
> +          prin = nsNullPrincipal::Create(attrs);
> +        }

it seems the whole function uses 4 space indendation, right? please change that back so it's easier to pinpoint your changes.

@@ +313,5 @@
> +        nsresult rv = uri->SchemeIs("data", &isDataURL);
> +        bool isDataDocument = NS_SUCCEEDED(rv) && isDataURL &&
> +                              loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT;
> +        // detecting loads ofiframe[srcdoc] and inline documents created with data:
> +        if (URISpec.EqualsLiteral("about:srcdoc") || isDataDocument) {

I am slightly confused about all that. So you have to explain. My biggest question is: If a load is of type TYPE_DOCUMENT, then it's a top level load, right? So where should it inherit the CSP from?

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8829843 [details] [diff] [review]
0001-Bug-1073952-tests-for-iframe-sandbox-srcdoc-and-data.patch

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

Can you verify that actually all of your tests run? Since we added back the contentPolicy check within docshell I am worried that for a top_level loads we get 'allowed' twice, once from within docshell and once from within the contentSecuritymanager which would bring your counter out of order.

tests itself look fine, r=me

::: dom/security/test/csp/file_iframe_srcdoc.sjs
@@ +35,5 @@
> +
> +
> +const INNER_DATAURI_IFRAME = `
> +  <iframe sandbox='allow-scripts' src='data:text/html,<script>
> +      alert(1);debugger;parent.parent.parent.postMessage({result: &quot;allowed&quot;}, &quot;*&quot;);

please remove alert(1) and debugger

@@ +52,5 @@
> +function handleRequest(request, response) {
> +  const query = new URLSearchParams(request.queryString);
> +
> +  response.setHeader("Cache-Control", "no-cache", false);
> +  response.setHeader("Content-Security-Policy", query.get("csp"), false);

nit: please check if query.get("csp") actually returns a string, otherwise you are sending the CSP header but no value. not the end of the world but probably confusing

@@ +60,5 @@
> +    response.write(SIMPLE_IFRAME_SRCDOC);
> +    return;
> +  }
> +
> +  else if (query.get("action") === "nested_iframe_srcdoc") {

nit: remove the else, you already return if the first branch is taken

@@ +65,5 @@
> +    response.write(NESTED_IFRAME_SRCDOC);
> +    return;
> +  }
> +
> +  else if (query.get("action") === "nested_iframe_srcdoc_datauri") {

same here, remove the else.

Comment 35

[Frederik Braun [:freddy]]

> Comment on attachment 8829841 [details] [diff] [review]
> 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch
> 
> Review of attachment 8829841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Besides GetChannelResultPrincipal (and also nsCSPService) nothing changed
> right?

Correct

> 
> ::: caps/nsScriptSecurityManager.cpp
> @@ +301,5 @@
> > +          OriginAttributes attrs;
> > +          loadInfo->GetOriginAttributes(&attrs);
> > +          attrs.StripAttributes(OriginAttributes::STRIP_ADDON_ID);
> > +          prin = nsNullPrincipal::Create(attrs);
> > +        }
> 
> it seems the whole function uses 4 space indendation, right? please change
> that back so it's easier to pinpoint your changes.
> 

OK.

> @@ +313,5 @@
> > +        nsresult rv = uri->SchemeIs("data", &isDataURL);
> > +        bool isDataDocument = NS_SUCCEEDED(rv) && isDataURL &&
> > +                              loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT;
> > +        // detecting loads ofiframe[srcdoc] and inline documents created with data:
> > +        if (URISpec.EqualsLiteral("about:srcdoc") || isDataDocument) {
> 
> I am slightly confused about all that. So you have to explain. My biggest
> question is: If a load is of type TYPE_DOCUMENT, then it's a top level load,
> right? So where should it inherit the CSP from?

Sorry. My local file said TYPE_SUBDOCUMENT, but I forgot to commit the change to include it in the patch file.

Comment 36

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

Comment 37

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1073952-tests-for-iframe-sandbox-srcdoc-and-data.patch

Carrying over ckerschb's r+

Comment 38

[Frederik Braun [:freddy]]

Attachment: 0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

This patch should carry the whitespace changes only.
I tested by with "git diff -w", which omits whitespace changes and thus shows nothing.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8830287 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

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

::: caps/nsScriptSecurityManager.cpp
@@ +312,5 @@
> +            bool isDataURL = false;
> +            nsresult rv = uri->SchemeIs("data", &isDataURL);
> +            bool isDataDocument = NS_SUCCEEDED(rv) && isDataURL &&
> +                                  loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_SUBDOCUMENT;
> +            // detecting iframe[srcdoc] and inline documents created with data:

I think this is still not entirely correct. What if a page has a CSP and loads a new window of about:srcdoc. The triggeringPrincipal in that case would be the page that holds the CSP and as such the CSP would be added to the about:srcdoc page which is *not* what we want, right?

I *think* (not entirely certain) that we want a TYPE_SUBDOCUMENT check surrounding all of the CSP quirks code.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8830289 [details] [diff] [review]
0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

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

Please ask dveditz to review that since I am not a caps peer and this change does not affect csp handling.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39)
> I think this is still not entirely correct. What if a page has a CSP and
> loads a new window of about:srcdoc. The triggeringPrincipal in that case
> would be the page that holds the CSP and as such the CSP would be added to
> the about:srcdoc page which is *not* what we want, right?

Certainly, about:srcdoc can not be loaded by itself in a new window (exhausting day), but what I am worried about is that GetChannelResultPrincipal() is called frequently and adding a lot of code (e.g. string comparison) that is *not* needed in 99,9% of the time worries me. Maybe we can find some precondition which needs to be met before we do all of that csp quirks.

Comment 42

[Frederik Braun [:freddy]]

For more context, the patch is inserted in a line [1] which is only executed for sandboxed load:
>         if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {

I don't think this codepath is really hot, but I'm up to modify the patch to check for whichever case is rarest first.

[1] http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/caps/nsScriptSecurityManager.cpp#305

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

I am slightly worried about adding code to scriptsecurityManger::GetChannelResultPrincipal(). Freddy convinved me that the code is not code because we only execute it for sandboxed loads, but it's still security critical code, so let's make sure we get it right.

Vidyo/IRC discussion notes:

* please surround all of the CSP quirks with if (contentTYpe == TYPE_SUBDOCUMENT)
* bool isData = (NS_SUCCEEDED(aURI->SchemeIs("data", &isData)) && isData;
* please add more documentation, especially for the data: case, at the moment we only talk about about:srcdoc
* then I am willing to r+ - thanks

Comment 44

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

Updated patch to take feedback from comment #43

Comment 45

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1073952-tests-for-iframe-sandbox-srcdoc-and-data.patch

Updated test (carrying over r+)

Comment 46

[Frederik Braun [:freddy]]

Attachment: 0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

whitespace-change only, so the function is properly using 2 spaces for indent. Confirmed through "diff -w".

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8830668 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

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

Great, I like that way more. Thanks for updating!

::: caps/nsScriptSecurityManager.cpp
@@ +318,5 @@
> +                  principalToInherit = loadInfo->TriggeringPrincipal();
> +                }
> +                nsCOMPtr<nsIContentSecurityPolicy> originalCsp;
> +                principalToInherit->GetCsp(getter_AddRefs(originalCsp));
> +                // if the principalToInherit had a CSP, we add it to the newly created NullPrincipal

nit: 80 chars line

Comment 48

[Daniel Veditz [:dveditz]]

Comment on attachment 8830671 [details] [diff] [review]
0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

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

r=dveditz

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 8830668 [details] [diff] [review]
0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

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

r=dveditz

Comment 50

[Frederik Braun [:freddy]]

Thanks for the speedy review!
Successful try run: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e6186b48b1d4cf9273e6ad365d9fd96249a1582&selectedJob=72180732

I'll only take ckerschb's nit for the 80 character then.

Comment 51

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1073952-inherit-CSP-into-iframe-sandbox-srcdoc-r.patch

rebased, carrying over r+

Comment 52

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1073952-tests-for-iframe-sandbox-srcdoc-and-data.patch

rebased, carrying over r+

Comment 53

[Frederik Braun [:freddy]]

Attachment: 0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

rebased, carrying over r+

Comment 54

[Frederik Braun [:freddy]]

Please check in. Filenames denote the order they are in (0001, 0002 and then 0003).

Thank you!

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e631015acc8
inherit CSP into iframe sandbox srcdoc r=ckerschb,dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5534087efac3
Part 2 -  tests for iframe sandbox srcdoc and data URIs with CSP r=ckerschb,dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63233859ee1
Part 3 -  proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal r=ckerschb,dveditz

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for frequent but intermittent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=72616140&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/cabc87c3fa0a22f3650bde884e884ee4ea91046b

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> Can you verify that actually all of your tests run? Since we added back the
> contentPolicy check within docshell I am worried that for a top_level loads
> we get 'allowed' twice, once from within docshell and once from within the
> contentSecuritymanager which would bring your counter out of order.

Probably the intermittent is due do what I mentioned in comment 34 - the two content policy checks for loads of TYPE_SUBDOCUMENT might screw with your counter.

Comment 58

[Frederik Braun [:freddy]]

Haven't had the time to look into this.
Repushed to try and looking other intermittent failures, I can still not reproduce on a Mac OS desktop, but only on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10be4e1e955af952b9f71de16808f64f2dde91a0&selectedJob=83706303

Will debate next-steps with ckerschb.

Comment 59

[Frederik Braun [:freddy]]

ckerschb had a hunch that the two tests would interact with each other - poorly.
I have indeed been able to push to try only one test enabled and it worked out.
I found some areas that were likely culprits and have pushed to try successfully.

I triggered a re-run for the previous failures (Mac OS X 10.10 opt), in case I just got lucky on this specific push. Will check results tomorrow.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3871d87accd6730299dee4fdcd8b236b3478e31&selectedJob=84295977

Comment 60

[Frederik Braun [:freddy]]

Attachment: Bug 1073952: inherit CSP into iframe sandbox srcdoc

Review commit: https://reviewboard.mozilla.org/r/121392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121392/

Comment 61

[Frederik Braun [:freddy]]

Attachment: Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

Review commit: https://reviewboard.mozilla.org/r/121394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121394/

Comment 62

[Frederik Braun [:freddy]]

Attachment: Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

Review commit: https://reviewboard.mozilla.org/r/121396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121396/

Comment 63

[Frederik Braun [:freddy]]

Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

Reuploaded test (now through mozreview instead of Bugzilla).
Carrying over review.

Comment 64

[Frederik Braun [:freddy]]

Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

Reuploaded test (now through mozreview instead of Bugzilla).
Carrying over review.

Comment 65

[Frederik Braun [:freddy]]

Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

Reuploaded patch (now through mozreview instead of Bugzilla).
Carrying over review.

Comment 66

[Frederik Braun [:freddy]]

Green on try, see mozreview. Please check in.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this because it doesn't have r+ set in MozReview.

Comment 68

[Frederik Braun [:freddy]]

Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

https://reviewboard.mozilla.org/r/121396/#review123676

Comment 69

[Frederik Braun [:freddy]]

Ah, carrying over r+ from Bugzilla into MozReview doesn't work?

Comment 70

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

https://reviewboard.mozilla.org/r/121394/#review123832

r=me

Comment 71

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

https://reviewboard.mozilla.org/r/121392/#review123836

Comment 72

[Carsten Book [:Tomcat]]

Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

https://reviewboard.mozilla.org/r/121396/#review123842

r=dveditz

Comment 73

[Carsten Book [:Tomcat]]

Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

https://reviewboard.mozilla.org/r/121396/#review123846

Comment 74

[Carsten Book [:Tomcat]]

Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

https://reviewboard.mozilla.org/r/121394/#review123850

Comment 75

[Carsten Book [:Tomcat]]

Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

https://reviewboard.mozilla.org/r/121394/#review123852

Comment 76

[Carsten Book [:Tomcat]]

Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

https://reviewboard.mozilla.org/r/121392/#review123854

Comment 77

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a8f304394417 -d f7541b903b15: rebasing 382824:a8f304394417 "Bug 1073952: inherit CSP into iframe sandbox srcdoc r=ckerschb,Tomcat"
rebasing 382825:6f45e1f5c070 "Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP r=ckerschb,Tomcat"
merging dom/security/test/csp/mochitest.ini
warning: conflicts while merging dom/security/test/csp/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 78

[Frederik Braun [:freddy]]

Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121392/diff/1-2/

Comment 79

[Frederik Braun [:freddy]]

Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121394/diff/1-2/

Comment 80

[Frederik Braun [:freddy]]

Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121396/diff/1-2/

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37d0954bf143
inherit CSP into iframe sandbox srcdoc r=ckerschb,Tomcat
https://hg.mozilla.org/integration/autoland/rev/461141e9b3c9
tests for iframe sandbox srcdoc and data URIs with CSP r=ckerschb,Tomcat
https://hg.mozilla.org/integration/autoland/rev/bf127c7f5a9b
proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal r=Tomcat

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37d0954bf143
https://hg.mozilla.org/mozilla-central/rev/461141e9b3c9
https://hg.mozilla.org/mozilla-central/rev/bf127c7f5a9b

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/06db7fb30689
https://hg.mozilla.org/releases/mozilla-esr52/rev/bdc731ce02c6

Comment 84

[Boris Zbarsky [:bzbarsky]]

Fwiw, the "proper indentation" changeset clearly mis-indented this code:

      if (loadInfo->GetLoadingSandboxed() &&
        loadInfo->GetForceInheritPrincipalDropped()) {
        forceInherit = true;
      }

in a really confusing way...