Closed Bug 1584216 (CVE-2019-11763) Opened 1 year ago Closed 1 year ago

Handle zero bytes after <!- and & correctly

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + fixed
firefox71 + fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main70+][adv-esr68.2+][post-critsmash-triage], [wptsync upstream])

Attachments

(3 files, 1 obsolete file)

This just got dropped in a conference:

<!-[x00][x00][x00][x00][x00]- ><div title="--><img src=1 onerror=alert(1)>"></div>
Group: core-security → dom-core-security
Keywords: sec-moderate
See Also: → 1540675

Freddy: what's bug 1540675? not a dupe, are you suggesting it's a possible regressor? Have you checked a pre-68 build that doesn't have this bug and that's why you suspect that one?

Flags: needinfo?(fbraun)

I'm at a conference and didn't find the time to properly up until now.
I just meant to tag it as "See Also" because it's a similar comment parsing bug. They are also both reported by Gareth Heyes.

He just dropped this in his presentation slides.

Flags: needinfo?(fbraun)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Henri, there seem to be some related null byte issues according to https://portswigger.net/web-security/cross-site-scripting/cheat-sheet

  • Firefox allows NULLS after &, e.g. go to data:text/html,<a href="javascript&%00%23x3a;alert(1)">Firefox</a>
  • Firefox allows NULLs inside named entities, e.g., data:text/html,<a href="javascript&%00co%00lon;alert(1)">Firefox</a>

Would this patch address those cases, if not, would it make sense to do so?

(In reply to Frederik Braun [:freddyb] from comment #4)

Would this patch address those cases,

No, but I noticed those bugs while fixing this one.

if not, would it make sense to do so?

I was thinking of filing another bug so that thing would be disclosable independently of those, but now that it's all here and already disclosed anyway, it doesn't matter for disclosure. I'll fix those cases, too.

Attachment #9097337 - Attachment description: Bug 1584216 - Drop special case from markup declaration hyphen state. → Bug 1584216 - Adjust tokenization of U+0000.
Attachment #9097342 - Attachment description: Bug 1584216 test - Test U+0000 after <!- → Bug 1584216 test - Test U+0000 after <!- and after &
Summary: Handle zero bytes between comment-opening hyphens correctly → Handle zero bytes after <!- and & correctly

The new patch addresses the other cases, too.

Comment on attachment 9097337 [details]
Bug 1584216 - Adjust tokenization of U+0000.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Extremely easily, but the issue has already been publicly disclosed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet and https://twitter.com/garethheyes/status/1177296295947636737 , so there isn't much point in delaying landing.
  • 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?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: The same patch applies.
  • How likely is this patch to cause regressions; how much testing does it need?: Relatively unlikely, since the test case in the other patch tests the relevant cases rather well. Also, if this was to regress, it would unlikely be workflow-breaking for users, since this affects rare cases. (U+0000 does not legitimately appear in HTML and especially not in these positions.)
Attachment #9097337 - Flags: sec-approval?

FWIW bug that are sec-moderate or lower do not need sec-approval as per https://wiki.mozilla.org/Security/Bug_Approval_Process
Generally, the idea of the approval process is to prevent us from 0daying by shipping things and not making a release for a very long while. In that case, people would be able to look at the commits and attack our whole user base (since everyone is still affected).

All of this doesn't apply here: The bug is already public, we're not too afraid of people being exploited by this in the wild (and I also think this is more of a sec-low than a sec-moderate :-)). Even if they were exploited, we can't help them since the underlying bug is XSS in the web site they are visiting.

Comment on attachment 9097337 [details]
Bug 1584216 - Adjust tokenization of U+0000.

Does not need approval per comment 9.

Attachment #9097337 - Flags: sec-approval?

(In reply to Frederik Braun [:freddyb] from comment #9)

FWIW bug that are sec-moderate or lower do not need sec-approval as per https://wiki.mozilla.org/Security/Bug_Approval_Process

OK. I queued a landing.

What should I do about landing the test?

Flags: needinfo?(fbraun)
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Please nominate this for Beta and ESR68 approval when you get a chance. And IMO, if this is already a publicly-disclosed issue (and the severity of it is pretty low), there doesn't seem to be much harm in landing the testcase too.

Comment on attachment 9097337 [details]
Bug 1584216 - Adjust tokenization of U+0000.

Beta/Release Uplift Approval Request

  • User impact if declined: User could become a victim of XSS on a site that does XSS filtering in a manner that is just on the right side of safe spec-wise but that doesn't use reasonable XSS filtering practices that have more security margin, such as re-serializing the input as parsed by the XSS filter. In other words, the circumstances where bad things don't happen with the patch but do happen without the patch are rather narrow.
  • 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): This is relatively unrisky, since the test case in the other patch tests the relevant cases rather well. Also, if this was to regress, it would unlikely be workflow-breaking for users, since this affects rare cases. (U+0000 does not legitimately appear in HTML and especially not in these positions.)

(I marked "covered by automated tests" as "No", since the test hasn't landed, but a test does exist in unlanded form.)

  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Simple security patch for a publicly-disclosed bug.
  • User impact if declined: User could become a victim of XSS on a site that does XSS filtering in a manner that is just on the right side of safe spec-wise but that doesn't use reasonable XSS filtering practices that have more security margin, such as re-serializing the input as parsed by the XSS filter. In other words, the circumstances where bad things don't happen with the patch but do happen without the patch are rather narrow.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is relatively unrisky, since the test case in the other patch tests the relevant cases rather well. Also, if this was to regress, it would unlikely be workflow-breaking for users, since this affects rare cases. (U+0000 does not legitimately appear in HTML and especially not in these positions.)
  • String or UUID changes made by this patch: None
Flags: needinfo?(hsivonen)
Attachment #9097337 - Flags: approval-mozilla-esr68?
Attachment #9097337 - Flags: approval-mozilla-beta?

Let's hold off on the test, it can wait till we release 70 and its corresponding ESR.

Comment on attachment 9097337 [details]
Bug 1584216 - Adjust tokenization of U+0000.

Fix for sec-moderate issue, public disclosure means we'd like to bring this to ESR as well as beta.
OK for uplift for beta 13.

Attachment #9097337 - Flags: approval-mozilla-esr68?
Attachment #9097337 - Flags: approval-mozilla-esr68+
Attachment #9097337 - Flags: approval-mozilla-beta?
Attachment #9097337 - Flags: approval-mozilla-beta+

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

What should I do about landing the test?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

Please nominate this for Beta and ESR68 approval when you get a chance. And IMO, if this is already a publicly-disclosed issue (and the severity of it is pretty low), there doesn't seem to be much harm in landing the testcase too.

(In reply to Liz Henry (:lizzard) from comment #15)

Let's hold off on the test, it can wait till we release 70 and its corresponding ESR.

I agree with Ryan here: It's not likely that attackers will jump on this attack vector: It requires an XSS vulnerability in a web pages and a very particular kind of ugly band-aid on top. This attack isn't even needed for most XSS vulnerabilities. The likelihood of attackers being blocked by bad filtering is negligible.

Let's land the test.

Flags: needinfo?(fbraun)
Whiteboard: [adv-main70+] → [adv-main70+][adv-esr68.2+]
Flags: qe-verify-
Whiteboard: [adv-main70+][adv-esr68.2+] → [adv-main70+][adv-esr68.2+][post-critsmash-triage]

Might be relevant to mention in the advisory that a different U+0000 bug also fixed here allowed HTML entities to be masked from filters enabling the use of entities to mask the actual characters of interest from filters.

Flags: needinfo?(tom)
Attached file advisory.txt
Attachment #9100586 - Attachment is obsolete: true
Flags: needinfo?(tom)

Did this test ever end up landing?

Flags: needinfo?(hsivonen)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)

Did this test ever end up landing?

No, but the bug is still hidden, too. Should I land the test even though the bug remains hidden?

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

Go ahead :)

Group: core-security-release
Flags: needinfo?(ryanvm)
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/973934de64d5
test - Test U+0000 after <!- and after & r=alchen
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21218 for changes under testing/web-platform/tests
Whiteboard: [adv-main70+][adv-esr68.2+][post-critsmash-triage] → [adv-main70+][adv-esr68.2+][post-critsmash-triage], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.