Closed Bug 1432170 Opened 7 years ago Closed 3 years ago

CSP sandbox bypass with Blob

Categories

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

defect

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox92 --- fixed

People

(Reporter: s.h.h.n.j.k, Assigned: n.goeggi)

References

Details

(Keywords: csectype-other, reporter-external, sec-moderate, Whiteboard: [domsecurity-active][adv-main92-])

Attachments

(3 files, 2 obsolete files)

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.
Whiteboard: [Embargo until Chrome and Edge fix the bug]
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Flags: sec-bounty?
Product: Firefox → Core
Keywords: sec-low
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.
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
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
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.
Assignee: nobody → cegvinoth
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [Embargo until Chrome and Edge fix the bug] → [Embargo until Chrome and Edge fix the bug][domsecurity-backlog1]
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
Flags: needinfo?(cegvinoth)
Attached patch 1432170.patch (obsolete) — Splinter Review
Flags: needinfo?(cegvinoth)
Attachment #8953987 - Flags: review?(ckerschb)
(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.
Flags: sec-bounty? → sec-bounty+
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?
Attachment #8953987 - Flags: review?(ckerschb)
Attachment #8953987 - Attachment is obsolete: true
Attached patch Bug1432170.patch (obsolete) — Splinter Review
Attachment #8955986 - Flags: review?(ckerschb)
(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
Flags: needinfo?(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)
Attachment #8955986 - Flags: review?(ckerschb)
see above.
Flags: needinfo?(ckerschb)
Attached file Bug 1432170
Attachment #8955986 - Attachment is obsolete: true
Attachment #8958043 - Flags: review?(bugs)
Attachment #8958043 - Flags: review?(bugs) → review-
Attachment #8959495 - Flags: review?(ckerschb)
Attachment #8958043 - Flags: review?(bugs)
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.
Attachment #8958043 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(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?
Flags: needinfo?(ckerschb) → needinfo?(bugs)
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?
Flags: needinfo?(bugs)
(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
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?
(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 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.
Attachment #8958043 - Flags: review- → review?(bugs)
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.
Attachment #8958043 - Flags: review?(bugs)
Comment on attachment 8959495 [details] Bug 1432170 - Test files added Clearing r? till we have a valid patch.
Attachment #8959495 - Flags: review?(ckerschb)
(I don't really see review requests in phabricator.)
Assignee: cegvinoth → nobody

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?

Flags: needinfo?(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?

Flags: needinfo?(ckerschb) → needinfo?(ngogge)
Assignee: nobody → ngogge
Status: NEW → ASSIGNED
Flags: needinfo?(ngogge)

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

Group: dom-core-security
Whiteboard: [Embargo until Chrome and Edge fix the bug][domsecurity-backlog1] → [domsecurity-active]
Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/autoland/rev/4d96de82d4d0 Add tests for CSP sandbox bypass with Blob. r=ckerschb,dveditz
Regressions: 1720332
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main92-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: