1802346 - (CVE-2023-23604) DOMParser.parseFromSafeString creates multiple SystemPrincipal instances

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Since bug 1467970, DOMParser::ParseFromSafeString creates a brand new SystemPrincipal instance when being used to parse with a non-system principal. This is bad, as lots of code in gecko assumes that there is only a single system principal instance, and so will check if a principal is the system principal by pointer identity. For example, nsContentUtils::IsChromeDoc uses pointer identity to check if a principal is the system principal.

I discovered this because I fixed this issue as part of my work on bug 1443925, and it started causing screenshots test failures. It turns out that this method right now is used to parse a safe string for a non-system document in ScreenshotsOverlayChild.sys.mjs since bug 1786558 , and then the parsed node is adopted into the content document on the next line. Normally, this would be blocked by the check in nsINode, however the nsContentUtils::IsChromeDoc check failed to notice the chrome -> content adoption due to the system principal having a different pointer identity.

After fixing the code to use a single common system principal, tests started failing as the adoption in ScreenshotsOverlayChild no longer worked.

I'm marking this as a security bug, as there may be other security checks bypassed or broken by having multiple system principals, and I don't know what sorts of issues it could cause.

Comment 1

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

Adding bug 1786558 as a regressed-by as well, as that bug is the reason why the fix here isn't simple.

Comment 2

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

Attachment: Bug 1802346 - Use the owner principal in ParseFromSafeString, r=smaug!

This is done rather than using the system principal to avoid potential
cross-docgroup adoption issues.

Comment 3

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

Came up with a reasonable solution, where we change the semantics of DOMParser::ParseFromSafeString to always mimic the target document's principal, rather than hard-coding SystemPrincipal. This avoids the adoption issues with the caller, while also no longer creating multiple SystemPrincipal instances.

Comment 4

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

Attachment: Bug 1802346 - Part 2: Add principal tests to test_domparsing.xhtml, r=smaug!

Depends on D163017

Comment 5

[:Gijs (he/him)]

Is it possible to either MOZ_CRASH or even add compile-time restrictions that enforce the single-ton-ness of the system principal? That would help prevent reintroductions of this issue.

Comment 6

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

(In reply to :Gijs (he/him) from comment #5)

Is it possible to either MOZ_CRASH or even add compile-time restrictions that enforce the single-ton-ness of the system principal? That would help prevent reintroductions of this issue.

I'm going to hide the ability to construct new instances of the system principal in my patches for bug 1443925 (specifically https://phabricator.services.mozilla.com/D163035, where I hide the ability to construct one, and make it a hard singleton with assertions). If I wasn't imminently doing that, I'd want to add more assertions to the actual class itself, but assuming I can get those patches landed somewhat quickly, I'm not sure if it's worth it.

Comment 7

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

Neither :smaug nor I think this is sec-high, and as I can't imagine a way someone could manipulate these very short-lived chrome-only documents with a duplicate system principal off I'm inclined to think it isn't even sec-moderate. Marking as sec-low.

Comment 8

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

Use the owner principal in ParseFromSafeString, r=smaug
https://hg.mozilla.org/integration/autoland/rev/dcf80a1e5e45ab6cc6ed6a611ad20687437ae5b4
https://hg.mozilla.org/mozilla-central/rev/dcf80a1e5e45

Comment 9

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

I was looking at the bugmail and noticed that this is actually in a wrong component.

Comment 10

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

Part 2: Add principal tests to test_domparsing.xhtml, r=smaug
https://hg.mozilla.org/integration/autoland/rev/03a29bff819faafc59db1c15877b7aa542cacc14
https://hg.mozilla.org/mozilla-central/rev/03a29bff819f

Comment 11

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt