Closed Bug 1747388 (CVE-2022-31743) Opened 3 years ago Closed 2 years ago

Able to escape HTML comments by using a comment within a comment

Categories

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

defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
relnote-firefox --- -
firefox-esr91 --- unaffected
firefox100 --- wontfix
firefox101 + verified
firefox102 + verified

People

(Reporter: bugzilla, Assigned: hsivonen)

Details

(Keywords: csectype-other, reporter-external, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main101+])

Attachments

(4 files)

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.

Flags: sec-bounty?
Group: firefox-core-security → core-security
Component: Security → DOM: HTML Parser
Product: Firefox → Core
Group: core-security → dom-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true

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?

Flags: needinfo?(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.

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)
Severity: -- → S3
Priority: -- → P2

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

Flags: needinfo?(hsivonen)

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

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.

Attached file Tokenizer tests

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

Attached file 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.

Attached file Bug 1747388.

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

Flags: needinfo?(hsivonen) → needinfo?(mike)

(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.

Flags: needinfo?(mike)

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 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
Attachment #9275176 - Flags: sec-approval?

Comment on attachment 9275176 [details]
Bug 1747388.

Approved to land and consider for uplift

Attachment #9275176 - Flags: sec-approval? → sec-approval+

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

Flags: needinfo?(tom)

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 :)

Flags: needinfo?(tom)

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.

(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.

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
Flags: sec-bounty? → sec-bounty+

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

Flags: needinfo?(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
Flags: needinfo?(hsivonen)
Attachment #9275176 - Flags: approval-mozilla-beta?

Comment on attachment 9275176 [details]
Bug 1747388.

Approved for 101.0b6.

Attachment #9275176 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]
QA Whiteboard: [qa-triaged]

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main101+]
Alias: CVE-2022-31743
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: