1918874 - (CVE-2024-9394) Same-site cross-origin JSON data leak via application/x-mixed-replace response

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect, P1)

Description

[Masato Kinugawa]

Attachment: fx_x-mixed-replace_same-site_x-origin_json_leak.php

This is a very similar issue to Bug 1918301. It is possible to execute arbitrary JavaScript in the context of the JSON viewer origin (resource://devtools origin) via a crafted application/x-mixed-replace response. Specifically, if an application/vnd.mozilla.json.view response is retrurned first, and then an text/html response, the JavaScript written in the text/html page will be executed in the resource://devtools origin's context. This can be used to read same-"site" cross-"origin" JSON contents.

Steps to reproduce:

1. Navigate to https://vulnerabledoma.in/fx_x-mixed-replace_same-site_x-origin_json_leak.php . A few seconds later, it will display the HTML page.
2. On the displayed HTML page, check that window.origin or document.domain return resource://devtools or devtools. This suggests that there is a confusion in the execution context.
3. Click "Read same-site cross-origin JSON" button. A JSON page placed in https://www.vulnerabledoma.in origin will be opened in a new window. (Please Notice non-"www" vs "www")
4. Wait until the JSON window is closed. After being closed, the JSON data and request/response headeres will be displayed on the opener's page.

Frefiox Android does not affect this issue because it does not have the JSON viewer.

Comment 1

[Masato Kinugawa]

As can be seen, it leaks not only JSON data but also request and response headers, although Cookie, Set-Cookie header etc. don't seem to be listed there.
But it seems Authorization header for a basic auth or custom headers can be listed there.

Comment 2

[Daniel Veditz [:dveditz]]

On the displayed HTML page, check that window.origin or document.domain return resource://devtools or devtools. This suggests that there is a confusion in the execution context.

It's not so much "confusion" as it is a badly executed lie. Like bug 1918301, the "real" document is an internal renderer, but users could get confused if we showed that in the addressbar (and it would be inconvenient for sharing or modifying the URL) so we lie and show the original address to users. These viewers are supposed to be showing STATIC content; the execution context is that of the browser code rendering the display. The CONTENT is not supposed to need an execution context.

I was 99% sure the media viewer document that displays image/, audio/, and video/* content would have this same issue, but MediaDocument and the imgLoader seem to handle it. Didn't have the time to prove audio/video were OK.

I noticed Masato's testcase uses application/vnd.mozilla.json.view and not application/json. Does that cause us to load the JSON viewer differently?

Valentin: about your bug 1918301 comment 3 -- are you sure there's no way to prevent this centrally? There's a good chance we'll invent more special-purpose content viewers in the future, and it's asking a lot that every new person knows that multipart/x-mixed-replace even exists. Is there some way for a multi-part channel to decide that if the registered handler for the different parts are not the same then abort? (I think it's contractID-based though, and it goes against the design of that system to care if two different contractIDs are handled by the same or different code.) Or could we require that a handler that knows about multipart replace declares itself, and then the part of Necko that hands responses off based on content-type would require that if the content-type was inside a multipart replace channel? Or if the channel origin is not an http(s) URL then fail?

I don't know what kind of options are reasonable, but it's clearly a big foot-gun.

Comment 4

[Valentin Gosu [:valentin]]

I think this is a consequence of how these specific content viewers are set up.
For the JSON one for example, when the resource is "application/json" we actually use the sniffer to change the content type then install the JSONStreamConverter.

For the multipart mime test case, we instantiate the JSON viewer then several other onStart/onData/onStop flow into the same viewer - for the other parts of the message. The problem is that these are injected into the viewer, instead of being ignored.

I suspect the reason why that other viewers don't have this issue is that they don't inject the data into a HTML page, or that they release the listener after the first onStopRequest (I haven't checked).

What we might be able to do is to have the multipart converter only deliver the first part unless the final listener also implements nsIMultiPartChannelListener indicating it actually supports multipart channels and multiple onStart/Stop/DataAvailable.

Comment 5

[Julian Descottes [:jdescottes]]

We should reuse the same approach as bug 1918301, Nicolas will follow up.

Comment 6

[Calixte Denizet (:calixte)]

Attachment: Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

Comment 7

[Alexandre Poirot [:ochameau]]

It is a bit unfortunate that the fix crash the tab load and nothing appears at all.
You can only see an error in the browser console.

Is it the same behavior for PDF.js? (I don't have access to bug 1918301)

Ideally, the Sniffer should be the one to reject the request, over there:

    if (request instanceof Ci.nsIChannel) {
      // JSON View is enabled only for top level loads only.
      if (
        gPrefs.gEnabled &&
        request.loadInfo?.isTopLevelLoad &&
        request.loadFlags & Ci.nsIChannel.LOAD_DOCUMENT_URI &&
        getContentDisposition(request) != Ci.nsIChannel.DISPOSITION_ATTACHMENT
      ) {
        // Check the response content type and if it's a valid type
        // such as application/json or application/manifest+json
        // change it to new internal type consumed by JSON View.
        if (/^application\/(?:.+\+)?json$/.test(request.contentType)) {
          return JSON_VIEW_MIME_TYPE;
        }
      } else if (request.contentType === JSON_VIEW_MIME_TYPE) {
        return "application/json";
      }

But it doesn't reject the load because request.contentType here is "multipart/x-mixed-replace".
And there is nothing suspicious here. We can only only return a distinct mime type, so we can't do much from here.

The "mixed replace" allows to bypass the sniffer, and all its conditions.
The else if branch is the one that is meant to prevent websites from using JSON viewer magic mime type.
It would have been the check that would reject the second document in this mixed replace request.

I'm wondering if the other checks: top level, DOCUMENT_URI as well as the pref should also be checked from the converter.
After all, the convert is always registered and never check for the pref.
AFAIK, the "mixed replace" is the only way to reach the converter without passing by the Sniffer... but just in case some other trick gets there.

Comment 8

[Andrew McCreight [:mccr8]]

I've CC'ed you on the other bug.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9425552 [details]
Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

For a sec-high bug we need a clean patch with just the fix— no testcase. That can be checked in 4-6 weeks after release

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

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

Comment on attachment 9425552 [details]
Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

For a sec-high bug we need a clean patch with just the fix— no testcase. That can be checked in 4-6 weeks after release

I removed the test from the patch

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

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

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: sec-high bug
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: load the test case in the bug
• Risk associated with taking this patch: small
• Explanation of risk level: avoid opening JSONViewer in a very specific context
• String changes made/needed: -
• Is Android affected?: no

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

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

Comment 14

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: sec-high bug
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: load the test case in the bug
• Risk associated with taking this patch: low
• Explanation of risk level: avoid opening JSONViewer in a very specific context
• String changes made/needed: -
• Is Android affected?: no

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1918874 - Don't display json in the json viewer coming from a multipart response. r=#devtools-reviewers.

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

Comment 16

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: sec-high bug
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: load the test case in the bug
• Risk associated with taking this patch: low
• Explanation of risk level: avoid opening JSONViewer in a very specific context
• String changes made/needed: -
• Is Android affected?: no

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3fdeb6ea57e
Don't display json in the json viewer coming from a multipart response. r=devtools-reviewers,ochameau.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b103b98e814c
Don't display json in the json viewer coming from a multipart response. r=devtools-reviewers,ochameau.

Comment 19

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

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/7720da05df83

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/69d7b2277422

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4065c36f8011

Comment 23

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

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

Comment 24

[Bogdan Maris, Desktop Test Engineering]

Reproduced the issue described in comment 0 using an old Nightly build from 2024-09-14. Verified that using latest Nightly 132.0a1, latest Beta 131, latest esr115 and latest esr128 from Treeherder across platform (Windows 11, macOS 13 and Ubuntu 22.04) the .php file is now downloaded and no content is shown in the tab (like in the affected build).

Comment 25

[Tom Ritter [:tjr]]

While we appreciate the investigation and report, as this was filed after our comment also describing the issue in Bug 1918301 and that this would fall under a category of reasonable due diligence in fixing the other bug, we will mark this as bounty-

Comment 26

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 27

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 28

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

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-11-12] .

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

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1918874 - [devtools] Add a test to check that the JSONViewer isn't used when the JSON is in a multipart response. r=#devtools

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

I'm trying to add a test (D260968), but it's leaking.

Valentin, would you know what's happening there? It looks like the page never loads after the JSON file is downloaded, so I'm not sure our original fix for this bug was complete? Note that with https://vulnerabledoma.in/fx_x-mixed-replace_same-site_x-origin_json_leak.php , the tab gets closed, but I tested with the attached PHP file and it doesn't, so I'm not sure what's different between the 2.

Comment 31

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 32

[Valentin Gosu [:valentin]]

Attachment: Bug 1918874 - Use NS_DispatchToCurrentThread instead of NS_DispatchToMainThread in HttpChannelChild::Release to avoid shutdown leak r=#necko

We are guaranteed to be on the main thread in HttpChannelChild::Release() as
we always dispatch to MT otherwise. So we can use NS_DispatchToCurrentThread
to enqueue the DoNotifyListener runnable, instead of NS_DispatchToMainThread
which throws an NS_ASSERTION during process shutdown and leaks the runnable.

Comment 33

[Valentin Gosu [:valentin]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #30)

I'm trying to add a test (D260968), but it's leaking.

Valentin, would you know what's happening there? It looks like the page never loads after the JSON file is downloaded, so I'm not sure our original fix for this bug was complete? Note that with https://vulnerabledoma.in/fx_x-mixed-replace_same-site_x-origin_json_leak.php , the tab gets closed, but I tested with the attached PHP file and it doesn't, so I'm not sure what's different between the 2.

The tab getting closed would kill the process, which triggered the assertion.
I did a try push - hoping this doesn't regress anything else 🙂 https://treeherder.mozilla.org/jobs?repo=try&revision=5bc78481a1b885f04ce0ca73b4aa7c9a8bf12ee4

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #33)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #30)

I'm trying to add a test (D260968), but it's leaking.

Valentin, would you know what's happening there? It looks like the page never loads after the JSON file is downloaded, so I'm not sure our original fix for this bug was complete? Note that with https://vulnerabledoma.in/fx_x-mixed-replace_same-site_x-origin_json_leak.php , the tab gets closed, but I tested with the attached PHP file and it doesn't, so I'm not sure what's different between the 2.

The tab getting closed would kill the process, which triggered the assertion.
I did a try push - hoping this doesn't regress anything else 🙂 https://treeherder.mozilla.org/jobs?repo=try&revision=5bc78481a1b885f04ce0ca73b4aa7c9a8bf12ee4

awesome, thanks Valentin

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/38392cfd5cbf
https://hg.mozilla.org/integration/autoland/rev/3c0314ede684
[devtools] Add a test to check that the JSONViewer isn't used when the JSON is in a multipart response. r=devtools-reviewers,bomsy
https://github.com/mozilla-firefox/firefox/commit/bcf25e974f83
https://hg.mozilla.org/integration/autoland/rev/42f4f33bf8eb
Use NS_DispatchToCurrentThread instead of NS_DispatchToMainThread in HttpChannelChild::Release to avoid shutdown leak r=necko-reviewers,kershaw

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6e09e127bb36
https://hg.mozilla.org/integration/autoland/rev/eaf180d51dc7
Revert "Bug 1918874 - Use NS_DispatchToCurrentThread instead of NS_DispatchToMainThread in HttpChannelChild::Release to avoid shutdown leak r=necko-reviewers,kershaw" for causing devtools failures.

Comment 37

[Serban Stanca [:SerbanS]]

Reverted this because it was causing devtools failures.

• Revert link
• Push with failures
• Failure Log
• Failure line: SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/nsISupportsImpl.h:423:19 in get

Also, please check these devtools failures.

• Failure Log

Comment 38

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Serban Stanca [:SerbanS] from comment #37)

Reverted this because it was causing devtools failures.

• Revert link
• Push with failures
• Failure Log
• Failure line: SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/nsISupportsImpl.h:423:19 in get

Also, please check these devtools failures.

• Failure Log

Indeed, this still seems to fail on asan: https://treeherder.mozilla.org/jobs?repo=try&revision=fa449fedcb1c1bf2698b91303da202b8499e2c21&selectedTaskRun=ce0sm0pHRkOskmzDmpqlDA.0

Valentin, sorry to bother you again, would you know what's happening?

Comment 39

[Valentin Gosu [:valentin]]

Ah, yes - Rookie mistake.
I've updated the revision and did another try push https://treeherder.mozilla.org/jobs?repo=try&revision=f31a0f1fa5cb72b920308ec552148db03de340f9

Comment 40

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #39)

Ah, yes - Rookie mistake.
I've updated the revision and did another try push https://treeherder.mozilla.org/jobs?repo=try&revision=f31a0f1fa5cb72b920308ec552148db03de340f9

great, TRY looks okay on asan now, let's land this again

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/39a6f571d856
https://hg.mozilla.org/integration/autoland/rev/69685f4c4091
Use NS_DispatchToCurrentThread instead of NS_DispatchToMainThread in HttpChannelChild::Release to avoid shutdown leak r=necko-reviewers,kershaw
https://github.com/mozilla-firefox/firefox/commit/3180589ff4bb
https://hg.mozilla.org/integration/autoland/rev/53ef48b16f85
[devtools] Add a test to check that the JSONViewer isn't used when the JSON is in a multipart response. r=devtools-reviewers,bomsy

Comment 42

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/69685f4c4091
https://hg.mozilla.org/mozilla-central/rev/53ef48b16f85