2035756 - Fission Escape via Unvalidated PrincipalToInherit in nsDocShellLoadState: Compromised Content Process can load about:srcdoc for arbitrary principal and data

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Vincent Hilla [:vhilla]]

Attachment: patch

I'm reporting a bug that allows a compromised content process to load arbitrary code in a different origin via about:srcdoc, i.e. escape site isolation.

Bug 2034682 reports a supposed UXSS because a compromised content process could forge nsDocShellLoadState::PrincipalToInherit and the parent doesn't verify it. That bug uses about:blank and Nika points out that this might not be exploitable because it's just

loading a non-controllable empty about:blank document into an arbitrary origin's content process

This changes if we load about:srcdoc and I attached a PoC, just build and run MOZ_POC_UXSS=1 ./mach run poc_principal_inherit.html. I suppose the suggested fix in the original bug might still apply here too.

I filed this as a separate bug because I'm unsure how aware the original reporter is of this possibility.

Comment 1

[Vincent Hilla [:vhilla]]

I still want to test whether this could load a system principal, though I think we do check that content can't load system? Edit: Correct, trying to use a system principal gets us a null principal.

Comment 2

[Andrew McCreight [:mccr8]]

The original bug does mention the possibility of about:srcdoc, though it does not go into any detail about how to do anything with it, so maybe we should give the other bug the "proper" rating and then leave this as sec-other? I'll switch it to "depends on".

4. Affects all inheritable URIs:
   Not just about:blank — also about:srcdoc, and URIs with URI_INHERITS_SECURITY_CONTEXT.

Comment 3

[Vincent Hilla [:vhilla]]

Just a note on the sec rating from a discussion I had. The exploit allows a compromised content to host about:srcdoc with arbitrary code in a different non-compromised process and origin. The PoC embeds a target page, but interesting targets like bank pages won't let themselves be embedded. But executing code in a different process could allow an attacker to compromise other processes too. So sec-high makes sense.

Comment 4

[Vincent Hilla [:vhilla]]

Reproduced on mozilla-central and esr115 using localhost:8000/poc_principal_inherit.html.

Comment 5

[Vincent Hilla [:vhilla]]

Attachment: poc3.diff

ValidatePrincipalCouldPotentiallyBeLoadedBy in D299974 might not suffice. I created a third PoC where we jump from example.com via about:reader to example.org. Because an about: process has no origin suffix and falls through this check. Edit: actually, it gets put into the shared "web" remote type process. But still, it doesn't have an origin suffix and is less locked down.

Comment 6

[Vincent Hilla [:vhilla]]

We currently have three related bugs. The initial bug 2034682, forging mPrincipalToInherit to load about:blank in a different origin. This bug 2035756 reported by me to show this can be used to execute arbitrary code in that process / origin. And bug 2040160 discovered independently to show this can be used to gain system principal.

Bug 2040160 is simple to fix by adding some checks for about:srcdoc.

I have a patch in bug 2034682 to validate the principal, but it can be circumvented (poc3.diff). From discussion with Nika, the patch is hard to uplift and locking the exploit down completely is tricky right now. See bug 2027207, there has been recent work to improve process isolation checks, some not yet in release. Uplifting a fix to a version without that recent work bears the risk of blocking / crashing legitimate loads.

My suggestion is

• Bug 2040160 gets fixed and uplifted as usual, without mentioning the principal issue.
• D299974 lands as enhancement and doesn't get uplifted. It fixes the straightforward version from the attached patch file. Having some validation hopefully makes it less likely someone looks and discovers something like poc3.diff. By now, two reporters found the lack of validation independently via LLMs.
• The three bugs remain private. Unsure if that requires setting something on bug 2040160 if it gets fixed.

ni? Nika to make sure I got things right. Do you have something D299974 could land together with? Otherwise I'd file a bug and phrase it as follow-up to bug 2027207. Or bundle it in bug 2039808, but the mTriggeringWindowId part would need some debugging.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Sorry for the slow response. I think adding more validation like that as a normal enhancement makes sense. I'm also landing other principal validation patches as enhancements on nightly, and it's a known gap in our security that we don't defend peer content processes very well against cross-origin attacks. The big things which are notable here are any cases where we can escalate to the privilegedabout process, or to the parent process.

Comment 8

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox152 (beta) and tracked for firefox153 (nightly). However, the bug still isn't assigned.

:hsinyi, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 11

[Vincent Hilla [:vhilla]]

The current situation is that

• A patch is landing and will be uplifted to block cross process javascript: loads (bug 2043820).
• A patch was uplifted to block srcdoc in top-level or on non-srcdoc URIs (bug 2040160).
• A patch is riding the train to verify principals in nsDocShellLoadState (bug 2045635).

What is left is that

• The patch to verify mPrincipalToInherit can't be uplifted and doesn't cover poc3.diff. We could look into asserting that for about:srcdoc, the embedder subsumes the srcdoc principal.
• One can forge principals on a SH entry without touching nsDocShellLoadState (bug 2046558).
• mResultPrincipalURI isn't verified and can be used similar to mPrincipalToInherit with about:srcdoc or data: URIs (bug 2049792).
• Blob URI registration verifies principals, but a workaround like poc3.diff might be possible.
• One can navigate to a top level chrome URI (bug 2048793).

Comment 12

[Vincent Hilla [:vhilla]]

Attachment: (secure)

Comment 13

[Vincent Hilla [:vhilla]]

Marking 154 as affected because bug 2045635 has the poc3.diff gap.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0a35fc23e24e
https://hg.mozilla.org/integration/autoland/rev/08a8b825bcbb
Verify srcdoc and data result principal. r=nika

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/08a8b825bcbb

Comment 16

[Vincent Hilla [:vhilla]]

:dveditz, I think this should be csectype-sop, but not csectype-sandbox-escape.

A compromised content process can execute code in another origin, but not in the parent process (unlike bug 2040160). Bug 2043820 is pretty similar and got sec-moderate due to requiring the compromised content.

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D309545

Comment 18

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high: compromised content process can execute code in other origins
• Code covered by automated testing?: unknown
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: To simulate a compromised content process, Firefox needs to be built with the attached patch. Then set MOZ_POC_UXSS=1 and open ./poc_principal_inherit.html. The iframe should be blank and not contain the text origin: example.org, or crash on debug.
• Risk associated with taking this patch: low
• Explanation of risk level: In general, about:srcdoc should be embedded same-origin and data: URIs should have a null principal. Adding checks for that should thus be low-risk.

The patch covers two edge cases and I don't expect there to be more, but if there are, I don't think they would break much.

• String changes made/needed?: No
• Is Android affected?: yes

Comment 19

[Vincent Hilla [:vhilla]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D309545

Comment 20

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high: compromised content process can execute code in other origins
• Code covered by automated testing?: unknown
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: To simulate a compromised content process, Firefox needs to be built with the attached patch. Then set MOZ_POC_UXSS=1 and open ./poc_principal_inherit.html. The iframe should be blank and not contain the text origin: example.org, or crash on debug.
• Risk associated with taking this patch: low
• Explanation of risk level: In general, about:srcdoc should be embedded same-origin and data: URIs should have a null principal. Adding checks for that should thus be low-risk.

The patch covers two edge cases and I don't expect there to be more, but if there are, I don't think they would break much.

• String changes made/needed?: No
• Is Android affected?: yes

Comment 21

[Vincent Hilla [:vhilla]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D309545

Comment 22

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high: compromised content process can execute code in other origins
• Code covered by automated testing?: unknown
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: To simulate a compromised content process, Firefox needs to be built with the attached patch. Then set MOZ_POC_UXSS=1 and open ./poc_principal_inherit.html. The iframe should be blank and not contain the text origin: example.org, or crash on debug.
• Risk associated with taking this patch: low
• Explanation of risk level: In general, about:srcdoc should be embedded same-origin and data: URIs should have a null principal. Adding checks for that should thus be low-risk.

The patch covers two edge cases and I don't expect there to be more, but if there are, I don't think they would break much.

• String changes made/needed?: No
• Is Android affected?: yes

Comment 23

[Pascal Chevrel (relman team) -> :pascalc]

sec-high with the csectype-sandbox-escape keyword require sec-aproval

Comment 24

[Vincent Hilla [:vhilla]]

Comment on attachment 9602920 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It requires a compromised content process.
  If that's given, I think it's simple to find out how to forge the principal on data or srcdoc and get them to execute code in a different origin.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: In general, about:srcdoc should be embedded same-origin and data: URIs should have a null principal. Adding checks for that should thus be low-risk.
  The patch covers two edge cases and I don't expect there to be more, but if there are, I don't think they would break much.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 25

[Pascal Chevrel (relman team) -> :pascalc]

I am uplifting to branches even though it doesn't have sec-approval+ yet as it already landed in main and we are nearing the end of the beta cycle.

Comment 26

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/d5ae8d2b6bda
https://hg.mozilla.org/releases/mozilla-beta/rev/e3c9c5b42e75

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/d6d980d7d215

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/12c084a81ee5

Comment 29

[Pascal Chevrel (relman team) -> :pascalc]

Vincent, your patch is causing failures:
https://treeherder.mozilla.org/jobs?repo=mozilla-beta&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&selectedTaskRun=bjVIT1PzRPmTUeA496K64A.0&searchStr=Windows%2C11%2C25H2%2Cdebug%2CMochitests%2Cfrom%2Cmsix%2Cpackages%2Ctest-windows11-64-25h2%2Fdebug-mochitest-browser-chrome-msix%2Cbc38

I am not backing out yet because 153 beta 11 is already building, but we need a follow up fix across branches for next week in our last betas/RC

Comment 30

[Vincent Hilla [:vhilla]]

( clearing my ni? and working on this in bug 2054139 )

Comment 32

[Vincent Hilla [:vhilla]]

Because this is fixing https://bugzilla.mozilla.org/show_bug.cgi?id=2049792#c19, I guess sandbox escape makes sense now.

Comment 34

[Tom Ritter [:tjr]]

Comment on attachment 9602920 [details]
(secure)

Clearing flag or retroactively granting, whichever