Closed Bug 1683627 Opened 5 years ago Closed 5 years ago

Serialization bug in <noscript> may lead to sanitizers bypass and XSS

Categories

(Core :: DOM: Serializers, defect, P3)

Product:
Core
Component:
DOM: Serializers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox-esr78 86+ fixed
firefox84 --- wontfix
firefox85 --- wontfix
firefox86 + fixed

People

(Reporter: michal.bentkowski, Assigned: hsivonen)

References

Details

(Keywords: reporter-external, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86-][adv-esr78.8-])

Attachments

(1 file)

Bug 1683627 - Make <noscript> escaping conditional on whether scripting is enabled.
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr78+
Details | Review

According to HTML standard [1], the serialization of text nodes within NOSCRIPT element depends on whether scripting is enabled or not.

If scripting is enabled, then the textContent is serialized literally; otherwise it is escaped ("<" to "<", and "&" to "&"). Similarly, when NOSCRIPT is parsed: if scripting is enabled then it is treated as rawtext, and like a "transparent" element if it is disabled.

There appears to be a bug in Chromium's serializer, that noscript is always serialized assuming that scripting is enabled even if it was parsed in a context where scripting was disabled.

Consider the following example:

const doc = 
new DOMParser().parseFromString('<body><noscript>&lt;/noscript&gt;&lt;img src onerror=alert(1)>', 'text/html');
console.log(doc.body.innerHTML);
// will print:
//   <noscript></noscript><img src onerror=alert(1)></noscript>

Note that the original markup was harmless because the alert(1) was treated just as text. However, the serialized form contains an additional <img> tag that was not there initially.

The correct serialization of the markup should be:

<noscript>&lt;/noscript&gt;&lt;img src onerror=alert(1)&gt;</noscript>

I'm reporting this as a security bug because this issue allowed me to bypass DOMPurify[2], the popular HTML sanitizer.

Interestingly, the same serialization bug is present in Chromium, but not in Safari.

[1] https://html.spec.whatwg.org/multipage/parsing.html#serialising-html-fragments:html-fragment-serialisation-algorithm:~:text=If%20current%20node%20is%20a%20Text%20node
[2] https://github.com/cure53/DOMPurify

Flags: sec-bounty?

Also interestingly, if you use Firefox Sanitizer's sanitizeToString, then <noscript> text nodes are serialized correctly.

(In reply to Michał Bentkowski from comment #0)

There appears to be a bug in Chromium's serializer, that noscript is always serialized assuming that scripting is enabled even if it was parsed in a context where scripting was disabled.

I assume this is a copy-paste error and you are in fact reporting that the same issue exists in Firefox? Can you link to the relevant Chrome ticket so we don't 0-day them (and we find out if they publish their ticket) ?

Group: firefox-core-security → dom-core-security
Type: task → defect
Component: Security → DOM: Serializers
Flags: needinfo?(michal.bentkowski)
Product: Firefox → Core

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

I assume this is a copy-paste error and you are in fact reporting that the same issue exists in Firefox? Can you link to the relevant Chrome ticket so we don't 0-day them (and we find out if they publish their ticket) ?

Oops, sorry! Yes, I reported the same bug to Chromium before and forgot to change this bit. The same bug appears in Chromium and Firefox. The issue in their bugtracker is: https://bugs.chromium.org/p/chromium/issues/detail?id=1160635

Flags: needinfo?(michal.bentkowski)

No worries.

As a note, it's quite possible there won't be significant movement on this ticket until January 2021 because a lot of Mozilla folks are on Christmas break. Is that OK or are you aware of this being exploited in the wild or other significant reasons this can't wait 2 weeks?

Flags: needinfo?(michal.bentkowski)
See Also: → https://bugs.chromium.org/p/chromium/issues/detail?id=1160635

This looks like a duplicate of bug 1528997.

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

No worries.

As a note, it's quite possible there won't be significant movement on this ticket until January 2021 because a lot of Mozilla folks are on Christmas break. Is that OK or are you aware of this being exploited in the wild or other significant reasons this can't wait 2 weeks?

I think that's fine; the issue it caused was fixed in DOMPurify right away.

Anyway, the issue is now essentially public since Chromium folks decided to derestrict the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1160635

Flags: needinfo?(michal.bentkowski)

(In reply to Michał Bentkowski from comment #6)

Anyway, the issue is now essentially public since Chromium folks decided to derestrict the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1160635

Ah, hm. Interesting decision...

(In reply to Anne (:annevk) from comment #5)

This looks like a duplicate of bug 1528997.

Yeah, this looks likely - though it's interesting because in that bug, Henri suggests in comment 11 that:

(In reply to Henri Sivonen (:hsivonen) (away from Bugzilla until 2021-01-11) from comment #3)

Our behavior is compliant to the spec:
https://w3c.github.io/DOM-Parsing/#the-domparser-interface

It seems that Blink, WebKit and EdgeHTML don't comply with the spec. annevk, I guess the other engines have quorum and we should treat this as a spec bug?

But the spec has moved. And from this bug and/or its Chrome equivalent, it sounds like Blink, at least, has changed what they're doing to match us, but the spec has also changed? So this seems messy and I'm guessing we should align with Blink and other engines on a response - but this really isn't my area.

See Also: → CVE-2021-23974

The specification moved to https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-domparser-parsefromstring but nothing materially changed. And there probably is still a problem there in that it implies scripting is disabled yet bug 1528997 comment 6 remains true. But the subsequent comments about serialization in Firefox are also (likely, haven't checked) still true.

Severity: -- → S3
Flags: needinfo?(hsivonen)
Priority: -- → P3
Assignee: nobody → hsivonen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(hsivonen)

The comment explaining our deviation from the spec is from bug 744830.

(In reply to Henri Sivonen (:hsivonen) from comment #9)

Our differences from Chrome's wpt contribution can be seen at https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/syntax/serializing-html-fragments/escaping.html.ini

Let's defer the remaining failing tests there to a follow-up. Let's also defer https://searchfox.org/mozilla-central/source/dom/serializers/nsHTMLContentSerializer.cpp#240 to a follow-up.

tjr, considering that the Chromium bug and the test case are already public, is it OK to push this to try?

Flags: needinfo?(tom)

I don't believe this justifies as sec-high, so it's implicitly OK to push to try and OK to land without sec-approval.
Please also see Fixing Security Bugs for general reasoning and guidance beyond this specific bug.

Flags: needinfo?(tom)
Keywords: sec-moderate
See Also: → 1687303

(In reply to Henri Sivonen (:hsivonen) from comment #12)

(In reply to Henri Sivonen (:hsivonen) from comment #9)

Our differences from Chrome's wpt contribution can be seen at https://searchfox.org/mozilla-central/source/testing/web-platform/meta/html/syntax/serializing-html-fragments/escaping.html.ini

Let's defer the remaining failing tests there to a follow-up. Let's also defer https://searchfox.org/mozilla-central/source/dom/serializers/nsHTMLContentSerializer.cpp#240 to a follow-up.

Filed bug 1687304 and bug 1687303.

https://hg.mozilla.org/integration/autoland/rev/0d0455bee1a4b23f4d6e7e329f075260e0e1ef4a
https://hg.mozilla.org/mozilla-central/rev/0d0455bee1a4

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

It turns out this is essentially a duplicate of bug 1528997 which normally disqualifies it from the bounty, but this bug ties the impact into the Santizer we use as an exploit mitigation ourselves so we've decided to split the bounty between the two bugs.

Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]

Please nominate this for ESR78 approval when you get a chance. We may want to consider uplifting bug 1684116 alongside this, also.

Flags: needinfo?(hsivonen)

Comment on attachment 9197708 [details]
Bug 1683627 - Make <noscript> escaping conditional on whether scripting is enabled.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sanitizers building on browser-provided functionality may end up relying on what Gecko does in a way that leads to XSS.
  • User impact if declined: Potential for XSS
  • Fix Landed on Version: 86
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This changes the serialization of a relatively rare element.

Note: The patch changes the expectations for test from bug 1684116, so either that change needs to be uplifted underneath this one or the test expectation changes need to be dropped from the uplift.

  • String or UUID changes made by this patch: None
Flags: needinfo?(hsivonen)
Attachment #9197708 - Flags: approval-mozilla-esr78?

Comment on attachment 9197708 [details]
Bug 1683627 - Make <noscript> escaping conditional on whether scripting is enabled.

Approved for 78.8esr.

Attachment #9197708 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+][adv-esr78.8+]

This will actually be included in Bug 1528997's

Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86+][adv-esr78.8+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main86-][adv-esr78.8-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: