Handle zero bytes after <!- and & correctly
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
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)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr68+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
418 bytes,
text/plain
|
Details |
This just got dropped in a conference:
<!-[x00][x00][x00][x00][x00]- ><div title="--><img src=1 onerror=alert(1)>"></div>
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
Comment 4•5 years ago
•
|
||
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?
Assignee | ||
Comment 5•5 years ago
|
||
Assignee | ||
Comment 6•5 years ago
|
||
(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.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
The new patch addresses the other cases, too.
Assignee | ||
Comment 8•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
Comment on attachment 9097337 [details]
Bug 1584216 - Adjust tokenization of U+0000.
Does not need approval per comment 9.
Assignee | ||
Comment 11•5 years ago
|
||
(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•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/4db16a46ddce1f0dc77fc0b9ec087e5bc55a0c12
https://hg.mozilla.org/mozilla-central/rev/4db16a46ddce
Comment 13•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
•
|
||
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•5 years ago
|
||
Let's hold off on the test, it can wait till we release 70 and its corresponding ESR.
Comment 16•5 years ago
|
||
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•5 years ago
|
||
uplift |
Comment 18•5 years ago
|
||
(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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
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•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
Assignee | ||
Comment 24•5 years ago
|
||
(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 26•5 years ago
|
||
Comment 29•5 years ago
|
||
bugherder |
Description
•