1809902 - Fluent translateRoots trips innerHTML "auditing" MOZ_CRASH if used with l10n ids whose messages contain markup

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[:Gijs (he/him)]

Over in bug 1807249 I ran into fluent hitting the DOM fragment parser and then the innerHTML auditing, for code in videocontrols.js that was invoking fluent on its shadow DOM - which caused crashes (but only in debug mode...) that broke tests.

This is a result of some unfortunate circumstances:

• a11y mochitests run with system principal [this is unfortunate and we should ideally fix it, but that doesn't address the fundamental issue here]
• DOMParser as invoked by UA widgets like videocontrols.js inherits that document principal
• Fluent inherits that same principal
• Fluent uses the fragment parser when inserting localized content that contains markup. It passes no sanitizer flags to ParseFragmentHTML.
• The fragment parser invokes DOMSecurityMonitor::AuditParsingOfHTMLXMLFragments which will crash on debug builds if there is JS on the stack that is not in the allowlist, and we're in a system principal document
• Fluent's translateRoots hits the fragment parsing code synchronously from some JS. This is in contrast to calling document.l10n.setAttributes to get the same translation to happen, where the element gets added to a queue that is emptied asynchronously, which means there is no JS on the stack.

So the outcome is clearly unfortunate. The question is how it should be addressed. Some underlying assumptions I'm aware of:

• fluent needs to parse HTML
• fluent already has builtin safeguards to restrict what HTML it allows, and those are generally stricter than those of a sanitizer (I think?). We already use it to insert markup into system principal docs (like browser.xhtml) and that isn't going to be something we (can) change. So although trying to e.g. change the principal of these nodes is technically possible, that doesn't really address the root issue.
• we don't like assigning to innerHTML, or the C++ equivalent, happening for chrome (system principal) code, because it often causes XSS style security issues. So we want to minimize this as much as possible and we need the audit code for that. Although we have linters, they don't run against all files and they may not find all patterns. I'm still calling this an assumption because in fact the sanitizer is normally run anyway for these patterns. So other than discouraging new uses, I'm not sure how much it's getting us.
• it's confusing/problematic that the exact same behaviour crashes on debug some but not all of the time, depending on whether the JS that invokes fluent is still on the stack by the time we hit the auditing code, as it's functionally doing the same thing. Either the auditing code should be invoked all or none of the time, but not somewhere in-between.

Based on this, a number of solutions (some mutually exclusive, some not) occur to me:

1. remove the debug-only crashing - the linter + sanitizer should be enough to help save our bacon here.
2. also crash if there's no JS on the stack because C++ also shouldn't be allowed to do dumb XSS-y innerHTML-like things (and we have no linting for that at the moment). It's not clear to me how much stuff this would break, other than fluent.
3. make fluent pass sanitizer flags to ParseFragmentHTML. This may have a performance implication; I don't know how frequently we hit this code in general, I expect it's limited in most usecases.
4. add some other argument to ParseFragmentHTML that exempts a caller from this crashing, without also requiring a sanitizer. This feels very yucky but accomplishes something similar to (3) without the perf drawback. There's precedent for this because we already have an exception inside the auditing code for about:newtab (that I've followed up elsewhere because I think it should go away...).

I could be persuaded for either 1 or 3/4. If we don't do (1), I think we should [also] try doing (2) because the inconsistency is both confusing and somewhat worrying. Note that either way the C++ / async consumers of ParseFragmentHTML are already getting the sanitizer anyway, we just don't crash.

Freddy/Christoph/Eemeli, wdyt?

Comment 1

[Eemeli Aro [:eemeli]]

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

• fluent already has builtin safeguards to restrict what HTML it allows, and those are generally stricter than those of a sanitizer (I think?). We already use it to insert markup into system principal docs (like browser.xhtml) and that isn't going to be something we (can) change. So although trying to e.g. change the principal of these nodes is technically possible, that doesn't really address the root issue.

Yeah, there are explicit whitelists of allowed elements and their attributes.

3. make fluent pass sanitizer flags to ParseFragmentHTML. This may have a performance implication; I don't know how frequently we hit this code in general, I expect it's limited in most usecases.

All of these messages trigger this code. OTOH, I'm not sure what fraction of these use a system principal, where that performance seems already mandated?

Comment 2

[:Gijs (he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #1)

3. make fluent pass sanitizer flags to ParseFragmentHTML. This may have a performance implication; I don't know how frequently we hit this code in general, I expect it's limited in most usecases.

All of these messages trigger this code. OTOH, I'm not sure what fraction of these use a system principal, where that performance seems already mandated?

Oh, good point. Yes, the performance won't change for system principal or about: page consumers, so I guess then it doesn't really make a significant difference - we don't really use fluent elsewhere, I think (there are constraints on where we expose document.l10n that are pretty similar to "system principal and about: pages")

That makes me lean towards fluent explicitly passing flags to do sanitization (option (3) in comment 0), potentially adding option (2) and investigating other C++ consumers of this code for system principal docs.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1809902 - make fluent sanitization of innerHTML assignments explicit to avoid DEBUG MOZ_CRASH, r?eemeli

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3647a38e0ff
make fluent sanitization of innerHTML assignments explicit to avoid DEBUG MOZ_CRASH, r=eemeli

Comment 5

[Serban Stanca [:SerbanS]]

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