1880879 - (CVE-2024-10941) Parent process crash with iframe with src=file:. or src=file:..

Status: RESOLVED FIXED

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

Description

[malware0x0]

Attachment: bug.png

Steps to reproduce:

A remote attacker could create a specially-crafted Web page containing
<iframe src=file:.></iframe> or <iframe src=file:..></iframe>
which would cause a segmentation fault error.

Actual results:

When you open the file (http://localhost/file.php), this causes the browser to crash.
If someone hosts a website with this and sends the link to another person, it would cause the victim's Web browser to crash once the website tries to get the value from "file:.".
I can reproduce the bug on Windows, Linux, and Android.
the report id: bp-b979de7a-388f-489a-ab4d-468df0240219

Comment 1

[malware0x0]

Similar bug reference:
https://exchange.xforce.ibmcloud.com/vulnerabilities/18401

Comment 2

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6f6e201853c8f5053d6f8837acf89e76f5a334c5&tochange=3bc0d683a41cb63c83cb115d1b6a85d50013d59e

Surprisingly this appears to have been broken for a long time.

The regression window is for when this started hanging for a bit (and then crashing) on Windows, as opposed to crashing immediately (not 100% sure what's going on there given I used mozregression).

Comment 3

[:Gijs (he/him)]

We started crashing more quickly (instead of hanging) in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f8ec5d8b4718a6456446b56c40ea32c50c631d48&tochange=ea48938d4cf677c4e00e093c2c92d7da5b52b0eb .

Anyway, off-hand it seems we're breaking in principal deserialization, in the middle of feature policy deserialization, which somewhat suggests that the child process has managed to send a principal that we don't know how to deserialize, which is bad. Considering time of day and the fact that the US/Canada are out today, perhaps when it's morning over here, Christoph has ideas?

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

I don't really have a clue right now. Looking at the DomTypes.ipdl and comparing that with the error message of "Error deserializing 'defaultOrigin' (nsIPrincipal) member of 'FeaturePolicyInfo'" I could imagine that we generate a have baked URI within the principal based on file:?

Though someone would need to actually debug that.

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to malware0x0 from comment #1)

Similar bug reference:
https://exchange.xforce.ibmcloud.com/vulnerabilities/18401

That was a null-pointer deref that was fixed in 2004 (bug 272381) -- completely unrelated. We didn't have multiple processes then so no need for "IPC" functionality or any of this code.

Comment 6

[Tom Schuster (MoCo)]

We fail to de-serialize the principal due to a parsing difference in PrincipalInfoToPrincipal. In particular this expression: info.originNoSuffix().Equals(originNoSuffix) fails.

When crashing: info.originNoSuffix() = "file:///." vs originNoSuffix = "file:///". We create the originNoSuffix from parsing info.spec() = "file:///." as an URI, but that results in an URI that is just "file:///" (probably because the . is meaningless here?)

I feel like I might have investigated a similar issue before.

Comment 7

[Tom Schuster (MoCo)]

So. I think this comes down to our URL parser for file URL not following the spec.

Input	URL.href (in Firefox)	According to Live URL Viewer
file:.	file:///.	file:///
file:///	file:///	file:///
file:///.	file:///	file:///

We don't correctly round trip in this case. So we parse file. in the child and produce file:///., and then parse file:///. in the parent and produce file:///.

I am not sure if this kind of round tripping for parsing is supposed to work according to the URL spec at all, but at least in this case we are clearly doing something wrong. Valentin is this something that we could try to fix, or is that just not worth it?

I guess if this round trip problem was more common we would see a lot more crashes like this? Sadly the signature mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | IPC::MessageReader::FatalError is somewhat useless, because it's very generic. Including the next frame IPC::ParamTraits<mozilla::dom::FeaturePolicyInfo>::Read(IPC::MessageReader*) would have been a lot more useful.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

(In reply to [OOO] Tom Schuster (MoCo) from comment #7)

So. I think this comes down to our URL parser for file URL not following the spec.

Input	URL.href (in Firefox)	According to Live URL Viewer
file:.	file:///.	file:///
file:///	file:///	file:///
file:///.	file:///	file:///

We don't correctly round trip in this case. So we parse file. in the child and produce file:///., and then parse file:///. in the parent and produce file:///.
I am not sure if this kind of round tripping for parsing is supposed to work according to the URL spec at all, but at least in this case we are clearly doing something wrong. Valentin is this something that we could try to fix, or is that just not worth it?

Yup, that's a bug, and we should try to fix it. Probably doesn't have much effect on interop, but round-tripping is important for cases like this.

I guess if this round trip problem was more common we would see a lot more crashes like this? Sadly the signature mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | IPC::MessageReader::FatalError is somewhat useless, because it's very generic. Including the next frame IPC::ParamTraits<mozilla::dom::FeaturePolicyInfo>::Read(IPC::MessageReader*) would have been a lot more useful.

Since this mismatch seems to cause issues later, I'm thinking we should add the round-tripping check we do here to PrincipalToPrincipalInfo too. That way we crash immediately and don't have to hunt down the cause.

Comment 9

[Tom Schuster (MoCo)]

Attachment: Bug 1880879 - Diagnostic patch for bad principal URI. r?nika

Comment 10

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

Based on comment #2, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:tschuster, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 11

[Tom Schuster (MoCo)]

The root cause of this crash will be fixed by bug 1887614. In the meantime I am waiting for feedback on D204932, which is trying to move this crash (and potentially other similar crashes) from the parent into the child.

Comment 12

[Tom Schuster (MoCo)]

This crash has been fixed by bug 1887614.

Comment 13

[Tom Schuster (MoCo)]

See previous comment. I originally kept this bug open to find some way to move other similar potential crashes from the parent to the child, but we consider this to have too much of an perfomance overhead.

Comment 14

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

Comment 15

[malware0x0]

Could you please confirm if this bug qualifies for a CVE assignment? If so, would it be possible for you to initiate the process or guide me on how to proceed with the CVE request?

Comment 16

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

While this bug does not qualify for a bounty, I'm flagging it as such so we can discuss it at the next meeting.

Comment 17

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

We discussed this and while we do not always assign CVEs for crashes, in this instance we will issue one and include it in the historical advisory.

Comment 18

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

https://github.com/mozilla/foundation-security-advisories/commit/b9f0167cf9102ce7b430346634554f5631ffa8f1