1584216 - (CVE-2019-11763) Handle zero bytes after <!- and & correctly

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

This just got dropped in a conference:

<!-[x00][x00][x00][x00][x00]- ><div title="--><img src=1 onerror=alert(1)>"></div>

Comment 1

[Daniel Veditz [:dveditz]]

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?

Comment 2

[Frederik Braun [:freddy]]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1584216 - Adjust tokenization of U+0000.

Comment 4

[Frederik Braun [:freddy]]

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?

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1584216 test - Test U+0000 after <!- and after &

Comment 6

[Henri Sivonen (:hsivonen)]

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

Comment 7

[Henri Sivonen (:hsivonen)]

The new patch addresses the other cases, too.

Comment 8

[Henri Sivonen (:hsivonen)]

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

Comment 9

[Frederik Braun [:freddy]]

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 10

[Henri Sivonen (:hsivonen)]

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

Does not need approval per comment 9.

Comment 11

[Henri Sivonen (:hsivonen)]

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

Comment 12

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

https://hg.mozilla.org/integration/autoland/rev/4db16a46ddce1f0dc77fc0b9ec087e5bc55a0c12
https://hg.mozilla.org/mozilla-central/rev/4db16a46ddce

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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 14

[Henri Sivonen (:hsivonen)]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/0b47d14dc42ff43dd27252fdf8831d4c7b22509f
https://hg.mozilla.org/releases/mozilla-esr68/rev/4b018793bf9f703ce2c85672a500c38d2fca588b

Comment 18

[Frederik Braun [:freddy]]

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

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 20

[Henri Sivonen (:hsivonen)]

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.

Comment 21

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 22

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/794a7772d112338be2eabda05752e616f1a7cb8d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Did this test ever end up landing?

Comment 24

[Henri Sivonen (: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?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Go ahead :)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/973934de64d5
test - Test U+0000 after <!- and after & r=alchen

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21218 for changes under testing/web-platform/tests

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/973934de64d5

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot