Bug 1073952 (CVE-2017-7788)

CSP can be bypassed with <iframe [sandbox] and [srcdoc]>

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: sdna.muneaki.nishimura, Assigned: freddyb)

Tracking

(Blocks 1 bug, {sec-low})

35 Branch
mozilla55
x86_64
Windows 8
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 fixed, firefox55 fixed)

Details

(Whiteboard: [reporter-external], [domsecurity-backlog][adv-main55+])

Attachments

(3 attachments, 16 obsolete attachments)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
(Reporter)

Description

5 years ago
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.
Component: Untriaged → DOM: Security
Product: Firefox → Core
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.
Flags: sec-bounty?
Whiteboard: [reporter-external]
Sid, any input? Is this a limit of what we've implemented of the spec? Or a bug in our implementation?
Flags: needinfo?(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.
Flags: needinfo?(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-low
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.
(Reporter)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
Typo: unless -> even if
Flags: sec-bounty? → sec-bounty-

Updated

4 years ago
Group: core-security → dom-core-security
Whiteboard: [reporter-external] → [reporter-external], [domsecurity-backlog]
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
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
Duplicate of this bug: 1280308
Duplicate of this bug: 1295489
Duplicate of this bug: 1305076
Duplicate of this bug: 1315952
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
Group: dom-core-security
(Assignee)

Updated

2 years ago
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
(Assignee)

Comment 15

2 years ago
Posted patch 0001-wip.patch (obsolete) — Splinter Review
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?
Attachment #8824419 - Flags: feedback?(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
Attachment #8824419 - Flags: feedback?(ckerschb)
(Assignee)

Comment 17

2 years ago
Looking forward to hearing your feedback.
Attachment #8748864 - Attachment is obsolete: true
Attachment #8824419 - Attachment is obsolete: true
Attachment #8825818 - Flags: review?(ckerschb)
(Assignee)

Comment 20

2 years ago
All tests green except for a some known & unrelated intermittent failures.
(Assignee)

Comment 21

2 years ago
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?
Attachment #8825818 - Flags: review?(dveditz)
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().
Attachment #8825818 - Flags: review?(ckerschb)
(Assignee)

Comment 23

2 years ago
Next iteration. Please take a look.
Attachment #8825818 - Attachment is obsolete: true
Attachment #8825818 - Flags: review?(dveditz)
Attachment #8826149 - Flags: review?(dveditz)
Attachment #8826149 - Flags: review?(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() {}??
Attachment #8826149 - Flags: review?(ckerschb)
(Assignee)

Comment 25

2 years ago
(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.
Attachment #8826149 - Attachment is obsolete: true
Attachment #8826149 - Flags: review?(dveditz)
Attachment #8826510 - Flags: review?(dveditz)
Attachment #8826510 - Flags: review?(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?
Attachment #8826510 - Flags: review?(ckerschb) → review+
(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?
(Assignee)

Comment 28

2 years ago
(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 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.
Attachment #8826510 - Flags: review?(dveditz) → review+
(Assignee)

Comment 30

2 years ago
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.
Attachment #8829841 - Flags: review?(dveditz)
Attachment #8829841 - Flags: review?(ckerschb)
(Assignee)

Updated

2 years ago
Attachment #8826510 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
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).
Attachment #8829843 - Flags: review?(dveditz)
Attachment #8829843 - Flags: review?(ckerschb)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1330920
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?
Attachment #8829841 - Flags: review?(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.
Attachment #8829843 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 35

2 years ago
> 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.
(Assignee)

Comment 36

2 years ago
Attachment #8829841 - Attachment is obsolete: true
Attachment #8829841 - Flags: review?(dveditz)
Attachment #8830287 - Flags: review?(ckerschb)
(Assignee)

Comment 37

2 years ago
Carrying over ckerschb's r+
Attachment #8830288 - Flags: review?(dveditz)
(Assignee)

Comment 38

2 years ago
This patch should carry the whitespace changes only.
I tested by with "git diff -w", which omits whitespace changes and thus shows nothing.
Attachment #8829843 - Attachment is obsolete: true
Attachment #8829843 - Flags: review?(dveditz)
Attachment #8830289 - Flags: review?(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.
Attachment #8830287 - Flags: review?(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.
Attachment #8830289 - Flags: review?(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.
(Assignee)

Comment 42

2 years ago
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
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
(Assignee)

Comment 44

2 years ago
Updated patch to take feedback from comment #43
Attachment #8830287 - Attachment is obsolete: true
Attachment #8830668 - Flags: review?(dveditz)
Attachment #8830668 - Flags: review?(ckerschb)
(Assignee)

Comment 45

2 years ago
Updated test (carrying over r+)
Attachment #8830670 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8830288 - Attachment is obsolete: true
Attachment #8830288 - Flags: review?(dveditz)
(Assignee)

Comment 46

2 years ago
whitespace-change only, so the function is properly using 2 spaces for indent. Confirmed through "diff -w".
Attachment #8830289 - Attachment is obsolete: true
Attachment #8830671 - Flags: review?(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]:
-----------------------------------------------------------------

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
Attachment #8830668 - Flags: review?(ckerschb) → review+
Comment on attachment 8830671 [details] [diff] [review]
0003-Bug-1073952-proper-indentation-for-nsScriptSecurityM.patch

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

r=dveditz
Attachment #8830671 - Flags: review?(dveditz) → review+
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
Attachment #8830668 - Flags: review?(dveditz) → review+
(Assignee)

Comment 50

2 years ago
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.
(Assignee)

Comment 51

2 years ago
rebased, carrying over r+
Attachment #8830668 - Attachment is obsolete: true
Attachment #8831069 - Flags: review+
(Assignee)

Comment 52

2 years ago
rebased, carrying over r+
Attachment #8830670 - Attachment is obsolete: true
Attachment #8831070 - Flags: review+
(Assignee)

Comment 53

2 years ago
rebased, carrying over r+
Attachment #8830671 - Attachment is obsolete: true
Attachment #8831072 - Flags: review+
(Assignee)

Comment 54

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

Thank you!
Keywords: checkin-needed

Comment 55

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(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.
(Assignee)

Comment 58

2 years ago
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.
Flags: needinfo?(fbraun)
(Assignee)

Comment 59

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8831069 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8831070 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8831072 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 63

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

Reuploaded test (now through mozreview instead of Bugzilla).
Carrying over review.
Attachment #8848471 - Flags: review?(dveditz)
Attachment #8848471 - Flags: review?(ckerschb)
Attachment #8848471 - Flags: review+
(Assignee)

Comment 64

2 years ago
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.
Attachment #8848472 - Flags: review?(dveditz)
Attachment #8848472 - Flags: review?(ckerschb)
Attachment #8848472 - Flags: review+
(Assignee)

Comment 65

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

Reuploaded patch (now through mozreview instead of Bugzilla).
Carrying over review.
Attachment #8848473 - Flags: review?(dveditz)
Attachment #8848473 - Flags: review?(ckerschb)
Attachment #8848473 - Flags: review+
(Assignee)

Comment 66

2 years ago
Green on try, see mozreview. Please check in.
Keywords: checkin-needed
Autoland can't push this because it doesn't have r+ set in MozReview.
Keywords: checkin-needed
(Assignee)

Comment 68

2 years ago
mozreview-review
Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

https://reviewboard.mozilla.org/r/121396/#review123676
(Assignee)

Comment 69

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

Comment 70

2 years ago
mozreview-review
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
Attachment #8848472 - Flags: review+

Comment 71

2 years ago
mozreview-review
Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

https://reviewboard.mozilla.org/r/121392/#review123836
Attachment #8848471 - Flags: review+

Comment 72

2 years ago
mozreview-review
Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

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

r=dveditz
Attachment #8848473 - Flags: review+

Comment 73

2 years ago
mozreview-review
Comment on attachment 8848473 [details]
Bug 1073952: proper indentation for nsScriptSecurityManager::GetChannelResultPrincipal

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

Comment 74

2 years ago
mozreview-review
Comment on attachment 8848472 [details]
Bug 1073952: tests for iframe sandbox srcdoc and data URIs with CSP

https://reviewboard.mozilla.org/r/121394/#review123850
Attachment #8848472 - Flags: review+

Comment 75

2 years ago
mozreview-review
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

2 years ago
mozreview-review
Comment on attachment 8848471 [details]
Bug 1073952: inherit CSP into iframe sandbox srcdoc

https://reviewboard.mozilla.org/r/121392/#review123854
Attachment #8848471 - Flags: review+

Comment 77

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 81

2 years ago
Pushed by cbook@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37d0954bf143
https://hg.mozilla.org/mozilla-central/rev/461141e9b3c9
https://hg.mozilla.org/mozilla-central/rev/bf127c7f5a9b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [reporter-external], [domsecurity-backlog] → [reporter-external], [domsecurity-backlog][adv-main55+]
Alias: CVE-2017-7788
Fwiw, the "proper indentation" changeset clearly mis-indented this code:

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

in a really confusing way...
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.