1747388 - (CVE-2022-31743) Able to escape HTML comments by using a comment within a comment

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Linus Särud]

I just used the following trick to fool a parser and got an XSS on a Bug Bounty-target. Firefox parsing differ from Chrome and Safari.

POC: https://playground.zulln.se/poc/firefox/commentwithincomment.html

Source code of POC:

Visible in all browsers.  <!-- <br>Not visible in any browser <!--k-> <br>Visible in Firefox. --> <br> Visible in all browsers.

The letter k within the comment in the middle of the poc can be any character.

Comment 1

[Frederik Braun [:freddy]]

I seem to recall we had a similar issue with comment parsing, but can't find the exact issue.
Anyhow, it seems that other browsers require a whitespace after the "<!--" and we do not?

Henri, is that a known difference?

Comment 2

[Henri Sivonen (:hsivonen)]

(In reply to Frederik Braun [:freddy] - out until Jan 4th. from comment #1)

I seem to recall we had a similar issue with comment parsing, but can't find the exact issue.
Anyhow, it seems that other browsers require a whitespace after the "<!--" and we do not?

Henri, is that a known difference?

Not a known difference. An actual bug. Reporter, thanks for reporting!

Our "Comment less-than sign bang dash dash state" differs from the one in the spec. In general, our comment states inline some transitions into the state being transitioned from instead of reconsuming in another state. In this case, we don't appear to perform the inlining as if "k" got reconsumed the way it's supposed to.

Comment 3

[Andrew McCreight [:mccr8]]

Henri, any updates here? It has been a few months. Thanks.

Comment 4

[Linus Särud]

The bug is still in place, it has been some time now so just want to make sure it is not forgotten about.

Comment 5

[Henri Sivonen (:hsivonen)]

I have not forgotten about this, but there are more tests to write (and to figure out how to disclose) than just the reported case, because I found similar instances of adjacent brokenness.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Tokenizer tests

Attaching tokenizer tests, to be upstreamed after the fix has been disclosed.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Tree builder tests

Attaching the same tests as tree builder tests tests, because WPT doesn't run tokenizer tests. To be upstreamed after the fix has been disclosed.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1747388.

Comment 9

[Henri Sivonen (:hsivonen)]

CCing sideshowbarker for opinion on what disclosure/advisory steps are necessary on the Java side.

Comment 10

[Michael[tm] Smith [:sideshowbarker]]

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

CCing sideshowbarker for opinion on what disclosure/advisory steps are necessary on the Java side.

I don’t recall ever before having a case where we needed to publish a disclosure/advisory for the Java parser or the validator, or to do a security release. But I suppose for the Java parser and the validator, we should wait until Firefox release management does whatever announcement and release they’ll be doing for this, and then for the Java parser and validator, we can follow with a release and some announcement.

Comment 11

[Frederik Braun [:freddy]]

This bug being rated sec-moderate and externally reported means we will issue a CVE once this goes to release, which will be June 28, 2022 if this patch makes into the current Nightly cycle.
If there's any extra urgency, we can of course consider uplifting to the beta branch (i.e., going to release 4 weeks earlier).

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 9275176 [details]
Bug 1747388.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Extremely easily. (Which is why I'm requesting sec-approval just to be sure, even though this has been rated as sec-moderate.)
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Release, beta
• If not all supported branches, which bug introduced the flaw?: Bug 1319410
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect the same patch to apply.
• How likely is this patch to cause regressions; how much testing does it need?: Relatively unlikely.
• Is Android affected?: Yes

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9275176 [details]
Bug 1747388.

Approved to land and consider for uplift

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Can you please suggest what you'd want a relnote to look like for this bug?

Comment 15

[Tom Ritter [:tjr]]

I've never done a relnote before, but it seemed like this could possibly be a developer note like 'Parsing of HTML comments, and specifically beginning comment tags inside of a comment tag, was aligned with the implementation of other browsers.'? I don't feel strongly that it should be a relnote, I just saw a reminder recently to set the flag :)

Comment 16

[Henri Sivonen (:hsivonen)]

I think this doesn't need a relnote, since this doesn't typically affect end users or Web developers, but I think this should go into a security advisory instead.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

I think this doesn't need a relnote, since this doesn't typically affect end users or Web developers, but I think this should go into a security advisory instead.

Agreed.

Comment 18

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

r=smaug
https://hg.mozilla.org/integration/autoland/rev/0794b6683ca59b39a521be2799116a6c174c43dd
https://hg.mozilla.org/mozilla-central/rev/0794b6683ca5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you're comfortable doing so.

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 9275176 [details]
Bug 1747388.

Beta/Release Uplift Approval Request

• User impact if declined: XSS in case a site doesn't remove attacker-provided HTML comments but passes them through to the browser in attacker-provided content.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is assumed to be low-risk, because the fix adheres to the expected structure of the tokenizer and has been tested (albeit outside our CI).
• String changes made/needed: None
• Is Android affected?: Yes

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9275176 [details]
Bug 1747388.

Approved for 101.0b6.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8e251b5c5ceb

Comment 23

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64, Ubuntu 20.04 x64 and on macOS 11.6.

Comment 24

[Henri Sivonen (:hsivonen)]

https://github.com/validator/htmlparser/commit/0d330112715f5d1bc8b206c08d2a64a2aab6fd82

Comment 25

[Tom Ritter [:tjr]]

Attachment: advisory.txt