1432170 - CSP sandbox bypass with Blob

Status: RESOLVED FIXED

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

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

1. Go to https://test.shhnjk.com/sandboxed_blob.php


Actual results:

Modal dialog showed up. When clicking a link, popup is also allowed.


Expected results:

https://test.shhnjk.com/sandboxed_blob.php has "Content-Security-Policy: sandbox allow-scripts" which should prevent modals and popups.

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

Why is it sec-low? If you check the origin of popup (bing.com), it isn't "null". Which warrants that this is full sandbox bypass.

Comment 2

[Daniel Veditz [:dveditz]]

according to https://w3c.github.io/webappsec-csp/#security-inherit-csp (section 7.8 in my version) blob should inherit the CSP like data: documents.  Chrome seems to behave the same on this testcase. If you've filed a bug for them please add a link to it in this bug. Even if it's private now it will be a handy reference in the future, and will give us something we can point them to when we talk to them about it.

(In reply to Jun from comment #1)
> Why is it sec-low? If you check the origin of popup (bing.com), it isn't
> "null". Which warrants that this is full sandbox bypass.

If my theory that we're not inheriting CSP at all into the blob document is correct I'll give it a sec-moderate, but the sandbox bypass itself is not that interesting.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

FWIW, looking at the spec [1] we currently inherit the CSP for data: and about:srcdoc, but we should do the same for blob. I think the relevant code lives here [2]. The reason the same trick does not work for data: is because we block top-level data: URI navigations. Maybe we should do the same for blob eventually and also block top-level navigations to blob:.

[1] https://w3c.github.io/webappsec-csp/#security-inherit-csp
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#227

Comment 4

[Daniel Veditz [:dveditz]]

What happens if you pass a blob: url to a window with a different origin (through postMessage?). Could that other origin now open a document using that blob? Hopefully it's not same-origin to the second window, but should it really inherit the CSP in that case? Then one origin's CSP is being imposed on another origin's document. On the other hand the origin of the blob had to be cooperative in sharing that URL so maybe it's fine anyway.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Vino, as described within comment 3, I think the problem can (hopefully) be resolved within InheritAndSetCSPOnPrincipalIfNeeded. I think we not only have to check for TYPE_SUBDOC but also for TYPE_DOC and additional add isBlob to isSrcDoc and isData. Can you take a look at that please?

[1] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#227

Comment 6

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: 1432170.patch

Comment 7

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #6)
> Created attachment 8953987 [details] [diff] [review]
> 1432170.patch

Actual problem occurred here,
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2969
We check only if CSP is present in Response headers and add on policy and return InitCSP() function early, we ignore the CSP present in NodePrincipal().

So I added fix for it.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8953987 [details] [diff] [review]
1432170.patch

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

Please answer my questions and we can chat about next steps to fix that bug - thanks!

::: caps/nsScriptSecurityManager.cpp
@@ +230,2 @@
>  
> +  if (!isSrcDoc && !isData && !isBlob) {

yeah, that part makes sense to me.

::: dom/base/nsDocument.cpp
@@ +2954,5 @@
> +    principal->GetCsp(getter_AddRefs(nodePrincipalCSP));
> +    if (nodePrincipalCSP) {
> +      applyNodePrincipalCSP = true;
> +    }
> +  }

I don't understand what that code does here. Can you please explain why that principal already has a CSP attached? What kind of pricnipal is it? Is that the case only for NullPrincipals from a blob?

Comment 9

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug1432170.patch

Comment 10

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Comment on attachment 8953987 [details] [diff] [review]
> 1432170.patch
> 
> Review of attachment 8953987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please answer my questions and we can chat about next steps to fix that bug
> - thanks!
> 
> ::: caps/nsScriptSecurityManager.cpp
> @@ +230,2 @@
> >  
> > +  if (!isSrcDoc && !isData && !isBlob) {
> 
> yeah, that part makes sense to me.
> 
> ::: dom/base/nsDocument.cpp
> @@ +2954,5 @@
> > +    principal->GetCsp(getter_AddRefs(nodePrincipalCSP));
> > +    if (nodePrincipalCSP) {
> > +      applyNodePrincipalCSP = true;
> > +    }
> > +  }
> 
> I don't understand what that code does here. Can you please explain why that
> principal already has a CSP attached? What kind of pricnipal is it? Is that
> the case only for NullPrincipals from a blob?

This principal is a NullPrincipal. 
This is set by the InheritAndSetCSPOnPrincipalIfNeeded [1] method specifically for blob URIs and also for Data URIs.
Let me know your comments.

[1] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#243

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8955986 [details] [diff] [review]
Bug1432170.patch

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

Please address my suggestion, provide an automated test for that scenario, and flag me again for review. Thanks!

::: caps/nsScriptSecurityManager.cpp
@@ +212,5 @@
>  
>    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
>    if (!loadInfo ||
> +      (loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_SUBDOCUMENT
> +      && loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT)) {

by now we have a loadInfo on all channels so you can remove the !loadInfo check, but please query the content type only once from the loadinfo and then compare that variable to TYPE_DOC and TYPE_SUBDOC as you do.

::: dom/base/nsDocument.cpp
@@ +2997,5 @@
>    auto addonPolicy = BasePrincipal::Cast(principal)->AddonPolicy();
>  
> +  // Check if NullPrincipal has CSP.
> +  bool applyNullPrincipalCSP = false;
> +  if(principal) {

A few things we have to update here:
* Please expand the document and describe why we are doing this.
* Please also check that we are actually loading a blob URI
* nit: please use space after if (here and underneath)

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

see above.

Comment 13

[Phabricator Automation]

Attachment: Bug 1432170

Comment 14

[Phabricator Automation]

Attachment: Bug 1432170 - Test files added

Comment 15

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

Comment on attachment 8958043 [details]
Bug 1432170

Since I don't quite understand why bloburi + nullprincipal needs to be special cased in nsDocument, r-.

Why is nullprincipal so special?
Or why is bloburi handled differently to say, about:.

I need some explanation.

Comment 16

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8958043 [details]
> Bug 1432170
> 
> Since I don't quite understand why bloburi + nullprincipal needs to be
> special cased in nsDocument, r-.
> 
> Why is nullprincipal so special?
> Or why is bloburi handled differently to say, about:.
> 
> I need some explanation.

 Here a URL(https://test.shhnjk.com/sandboxed_blob.php) load happens and blob URI navigation occurs via a script in the parent document. This blob URI is of null origin and the parent CSP is propagated to Blob URI via the null principal in this case. Hence specific check is added for null principal here.

All other cases in nsDocument, CSP is either loaded from CSP headers of the document. But for blob URI loads with null origin triggered from another document, CSP from parent document need to be handled. Hence this specific case for bloburi + nullprincipal.

:ckerschb correct me if I am wrong.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Smaug, CSP is generally not inherited for new top-level loads. For this particular case we want to inherit the CSP for blob URIs however. Hence we need to special case blob URIs within InitCSP() because otherwise we would bail early and wouldn't apply settings from the inherited CSP on the document. There is nothing special about NullPrincipals in this case however. Please note that about: pages do not inherit the CSP. For data: URIs we would face the same problem, but we don't allow top-level data: URI navigations, hence this problems only occurs for blob URIs.

I guess we can remove the NullPrincipal part from the patch and just turn it into an assertion. Something like:

bool applyCSPSettingsForToplevelBlob = false;
nsCOMPtr<nsIURI> potentialBlobURI;
rv = aChannel->GetURI(getter_AddRefs(potentialBlobURI));
NS_ENSURE_SUCCESS(rv, rv);
bool isBlob = (NS_SUCCEEDED(uri->SchemeIs("blob", &isBlob)) && isBlob);
if (isBlob && principal) {
  // Apply settings from CSP inherited for new top-level blob loads.
  MOZ_ASSERT(principal->GetIsNullPrincipal(),
             "inheriting the CSP only on a valid NullPrincipal");
  nsCOMPtr<nsIContentSecurityPolicy> inheritedCSPForToplevelBlob;
  principal->GetCsp(getter_AddRefs(inheritedCSPForToplevelBlob));
  if (inheritedCSPForToplevelBlob) {
    applyCSPSettingsForToplevelBlob = true;
  }
}

Does that sound acceptable?

Comment 18

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

I'm still having trouble to under to understand the big picture, and why blob gets special cased in nsDocument.
What if a top level page has a sandboxed iframe and load a blob url there, and that iframe opens a new about:blank window. Would that about:blank get handled differently comparing to opening bloburi? If so why?

Comment 19

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Olli Pettay [:smaug] from comment #18)
> I'm still having trouble to under to understand the big picture, and why
> blob gets special cased in nsDocument.
> What if a top level page has a sandboxed iframe and load a blob url there,
> and that iframe opens a new about:blank window. Would that about:blank get
> handled differently comparing to opening bloburi? If so why?

Yes it is handled differently. CSP is not inherited for about: pages.
Blob URIs are tied to the document in the window on which it was created.[1] Hence we must inherit the CSP of the document that created the blob URI and this CSP is passed via the principal for the blob top level load. Hence a special case is made in nsDocument to check CSP only for blob URIs.


[1] https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL

Comment 20

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

Why about: doesn't inherit CSP? about:blank inherits the principal, and if the contents of about:blank document is modified to load stuff, why that isn't controlled by CSP?

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Why about: doesn't inherit CSP? about:blank inherits the principal, and if
> the contents of about:blank document is modified to load stuff, why that
> isn't controlled by CSP?

We should test that, because I am pretty sure in that case it's not working properly either. We are inheriting the CSP, because it's on the principal, right. But I am pretty sure we don't call ApplySettingsFromCSP() on the new document because InitCSP() bails early in case the CSP is not delivered through the header.

All I am saying is, we need a reliable way to apply settings from a CSP on the new document. The inheritance of the CSP works, because of the principal inheritance but the CSP setup doesn't, because InitCSP() seems somehow broken to me.

Comment 22

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8958043 [details]
Bug 1432170

Main problem arrived because the function returned early without applying the CSP in the principal. Previously check was made only if CSP is present as addon policy or as CSP headers, and CSP present only in principal is not considered.
Hence that return early part is now removed.
No special case is now made for blob URI alone, but CSP will be applied if present in principal also.

Comment 23

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8958043 [details]
Bug 1432170

Current changes fixes this bug but breaks the test_data_doc_ignore_meta_csp.html and test_meta_csp_self.html tests, both related to data URI.
I will check and get back on this.
So cancelling the review request now.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8959495 [details]
Bug 1432170 - Test files added

Clearing r? till we have a valid patch.

Comment 25

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

(I don't really see review requests in phabricator.)

Comment 26

[Neha Kochar [:neha]]

Christoph, could you please re-review this sec bug in light of latest CSP changes and assign this to someone who can look at the 3-year old patches here?

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

I fairly confident the problem reported here got fixed when we moved the CSP from the Principal into the Client within Bug 965637. In more detail, visual inspection of CSP_ShouldResponseInheritCSP tells me that we account for blob: URLs.

Niklas, can you test that and potentially write an automated test for it based on the STRs?

Comment 28

[Niklas]

Attachment: Bug 1432170 - Add tests for CSP sandbox bypass with Blob. r=ckerschb

Comment 29

[Daniel Veditz [:dveditz]]

Unhiding bug: our version of the vulnerability has been fixed since Firefox 69 (comment 27) and the chromium bug has been public for a while

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d96de82d4d0
Add tests for CSP sandbox bypass with Blob. r=ckerschb,dveditz

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8daa0a5b09d1
fix lint failure. CLOSED TREE

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4d96de82d4d0
https://hg.mozilla.org/mozilla-central/rev/8daa0a5b09d1