Closed
Bug 1270381
(CVE-2016-2819)
Opened 8 years ago
Closed 8 years ago
HTML5 parser heap-buffer-overflow
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
People
(Reporter: firehack0r, Assigned: hsivonen)
Details
(4 keywords, Whiteboard: [adv-main47+][adv-esr45.2+] btpp-active)
Crash Data
Attachments
(4 files, 1 obsolete file)
|
25.34 KB,
text/plain
|
Details | |
|
326 bytes,
text/html
|
Details | |
|
660 bytes,
text/html
|
Details | |
|
8.77 KB,
patch
|
hsivonen
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36 Steps to reproduce: Using Firefox to load proof-of-concept code Actual results: Firefox crashed.
Crash Signature: ==951==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000381af8 at pc 0x7f9144523dac bp 0x7ffd92ccbe90 sp 0x7ffd92ccbe88
READ of size 8 at 0x615000381af8 thread T0 (Web Content)
#0 0x7f9144523dab in nsHtml5TreeBuilder::endTag(nsHtml5Elem…
Keywords: sec-critical
Hardware: Unspecified → All
|
||
Comment 3•8 years ago
|
||
Henri, can you look into this?
(I'm tidying up the signature field; the full stack is also in attachment 8749033 [details].)Group: firefox-core-security → core-security
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: ==951==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000381af8 at pc 0x7f9144523dac bp 0x7ffd92ccbe90 sp 0x7ffd92ccbe88
READ of size 8 at 0x615000381af8 thread T0 (Web Content)
#0 0x7f9144523dab in nsHtml5TreeBuilder::endTag(nsHtml5Elem… → [@ nsHtml5TreeBuilder::endTag]
Component: Untriaged → HTML: Parser
Ever confirmed: true
Flags: needinfo?(hsivonen)
Product: Firefox → Core
|
||
Updated•8 years ago
|
Flags: sec-bounty?
|
Assignee | |
Comment 4•8 years ago
|
||
I'll take a look.
Assignee: nobody → hsivonen
Flags: needinfo?(hsivonen)
|
|
Assignee | |
Comment 5•8 years ago
|
||
Root cause: Fragment parsing in the spec does not put the context node at the start of the stack. Instead, it always puts an <html> node there. Our parser implementation puts the context node there in order to have stuff appended to the context node without special casing what note to append to. Algorithms in the spec use the guaranteed presence of the <html> as a sentinel when walking the stack. Even though this has been taken into account in the algorithms in the implementation where the spec checks for <html>, it hadn't been taken into account in cases where the spec checks for "special". <html> is "special" but e.g. <svg> as a context node isn't. Fix: Adding stack position checks to isSpecial() checks. Additional info: After applying the patch, Firefox and Chrome produce different trees. (I was unable to test in Edge, because innerHTML on <svg> doesn't appear to work in Edge.) Once it's okay to disclose the PoC, we should follow up and figure out why Firefox and Chrome behave differently here. On the topic of disclosing the PoC: Reporter, once Mozilla's security folks mark this bug as publicly visible, are you OK with contributing the PoC to https://github.com/html5lib/html5lib-tests/ ? If you don't want to go through the trouble of preparing a pull request in the right format, I can prepare the PR (once it's time to disclose) if you like.
Flags: needinfo?(firehack0r)
Attachment #8749604 -
Flags: review?(wchen)
(In reply to Henri Sivonen (:hsivonen) from comment #5) > > On the topic of disclosing the PoC: Reporter, once Mozilla's security folks > mark this bug as publicly visible, are you OK with contributing the PoC to > https://github.com/html5lib/html5lib-tests/ ? If you don't want to go > through the trouble of preparing a pull request in the right format, I can > prepare the PR (once it's time to disclose) if you like. Yes, please do a pull request if you think it's need. I'm not understanding parser and spec enough to do. Thank you.
Flags: needinfo?(firehack0r)
|
||
Updated•8 years ago
|
Group: core-security → dom-core-security
|
||
Updated•8 years ago
|
Whiteboard: btpp-active
|
||
Comment 7•8 years ago
|
||
Comment on attachment 8749604 [details] [diff] [review] Treat the root of the stack as being "special" per spec Review of attachment 8749604 [details] [diff] [review]: ----------------------------------------------------------------- ::: parser/html/javasrc/TreeBuilder.java @@ +2187,1 @@ > } else if (node.isSpecial() I think it would be easier to understand that the (eltPos == 0) statement and the (node.isSpecial() && ...) statement are actually handling the same situation if you combined them. i.e. ((eltPos == 0 || node.isSpecial()) && ...). Normally when I see eltPos == 0 by itself in the code I expect that it's because the spec explicitly requires special handling for the root element.
Attachment #8749604 -
Flags: review?(wchen) → review+
|
|
Assignee | |
Comment 8•8 years ago
|
||
Comment on attachment 8749604 [details] [diff] [review] Treat the root of the stack as being "special" per spec [Security approval request comment] > How easily could an exploit be constructed based on the patch? It's obvious from the patch what the core problem is, but it takes a bit more than trivial effort to work out how to get the parser into the problematic state. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? There's a new assertion that makes it pretty clear what the core problem is, but that's apparent from the code changes, too. > Which older supported branches are affected by this flaw? All still supported ones. > If not all supported branches, which bug introduced the flaw? (Either foreign fragment parsing or the combination of templates on top of foreign fragment parsing.) > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply (possibly with trivial edits). > How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely to regress anything.
Attachment #8749604 -
Flags: sec-approval?
|
|
||
Comment 9•8 years ago
|
||
sec-approval+ for trunk. We'll want Aurora, Beta, and ESR45 patches made and nominated to go in following trunk landing. If you can get them nom'd soon, we can make next week's beta.
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox49:
--- → +
tracking-firefox-esr45:
--- → 47+
|
|
||
Updated•8 years ago
|
Attachment #8749604 -
Flags: sec-approval? → sec-approval+
|
|
Assignee | |
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f64adf2773bb89ba6867f60906f390266b6ab4f Bug 1270381. r=wchen.
|
|
Assignee | |
Comment 11•8 years ago
|
||
The patch applies to branches as-is.
Attachment #8749604 -
Attachment is obsolete: true
Attachment #8752585 -
Flags: review+
|
||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5f64adf2773b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
|
|
||
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
|
|
Assignee | |
Comment 13•8 years ago
|
||
Comment on attachment 8752585 [details] [diff] [review] Patch as landed with review comment addressed Approval Request Comment > [Feature/regressing bug #]: SVG fragment parsing combined with HTML templates. > [User impact if declined]: Security risk. > [Describe test coverage new/current, TreeHerder]: Landed uneventfully on trunk. > [Risks and why]: About as low as code change risk can be. > [String/UUID change made/needed]: None. [Approval Request Comment] > If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical. > User impact if declined: Security risk. > Fix Landed on Version: 49 > Risk to taking this patch (and alternatives if risky): About as low as code change risk can be. > String or UUID changes made by this patch: None.
Attachment #8752585 -
Flags: approval-mozilla-esr45?
Attachment #8752585 -
Flags: approval-mozilla-beta?
Attachment #8752585 -
Flags: approval-mozilla-aurora?
Comment on attachment 8752585 [details] [diff] [review] Patch as landed with review comment addressed Sec-crit, Aurora48+, Beta47+, ESR45+
Attachment #8752585 -
Flags: approval-mozilla-esr45?
Attachment #8752585 -
Flags: approval-mozilla-esr45+
Attachment #8752585 -
Flags: approval-mozilla-beta?
Attachment #8752585 -
Flags: approval-mozilla-beta+
Attachment #8752585 -
Flags: approval-mozilla-aurora?
Attachment #8752585 -
Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d583c1845ca https://hg.mozilla.org/releases/mozilla-beta/rev/c9199c063d5b https://hg.mozilla.org/releases/mozilla-esr45/rev/072992bf176d
|
|
||
Updated•8 years ago
|
Group: core-security-release
|
|
||
Updated•8 years ago
|
Group: dom-core-security
|
||
Updated•8 years ago
|
Flags: qe-verify+
|
|
||
Updated•8 years ago
|
Whiteboard: btpp-active → [adv-main47+][adv-esr45.2+] btpp-active
|
||
Comment 16•8 years ago
|
||
Reproduced the initial crash on Windows 7 64bit using old Nightly from 2016-05-04 : bp-e1212068-c791-45d2-92ec-c78b82160530 Verified that using Firefox 47beta9, latest Developer Edition 48.0a2, latest Nightly 49.0a1 and preESR build 45.1.2 on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.10.5 the crash does not reproduce anymore.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
|
|
||
Updated•8 years ago
|
Alias: CVE-2016-2819
|
|
Assignee | |
Comment 17•8 years ago
|
||
https://hg.mozilla.org/projects/htmlparser/rev/2fa40a1fc9bc9204e939b65ee4d66bb4f486706c Mozilla bug 1270381. r=wchen.
|
|
Assignee | |
Comment 18•8 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #17) > https://hg.mozilla.org/projects/htmlparser/rev/ > 2fa40a1fc9bc9204e939b65ee4d66bb4f486706c > Mozilla bug 1270381. r=wchen. I forgot to push the change to the htmlparser repo back in May. The corresponding landing to Firefox repos was already handled in comment 12 and in comment 15.
|
|
||
Updated•8 years ago
|
Group: core-security-release
|
|
||
Updated•8 years ago
|
Keywords: csectype-bounds
You need to log in
before you can comment on or make changes to this bug.
Description
•