Bug 1270381 (CVE-2016-2819)

HTML5 parser heap-buffer-overflow

VERIFIED FIXED in Firefox 47

Status

()

Core
HTML: Parser
--
critical
VERIFIED FIXED
a year ago
26 days ago

People

(Reporter: firehack, Assigned: hsivonen)

Tracking

(4 keywords)

46 Branch
mozilla49
All
Unspecified
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ verified, firefox48+ verified, firefox49+ verified, firefox-esr4547+ verified)

Details

(Whiteboard: [adv-main47+][adv-esr45.2+] btpp-active, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8749033 [details]
Detail report: minimized PoC, demo exploit, asan 49 log, brief of analysis

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

Updated

a year ago
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(nsHt…
Keywords: sec-critical
Hardware: Unspecified → All
(Reporter)

Comment 1

a year ago
Created attachment 8749050 [details]
Minimized PoC
(Reporter)

Comment 2

a year ago
Created attachment 8749052 [details]
Firefox 46.0.1 crash with register value 0x41414141

Comment 3

a year 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(nsHt… → [@ nsHtml5TreeBuilder::endTag]
Component: Untriaged → HTML: Parser
Ever confirmed: true
Flags: needinfo?(hsivonen)
Keywords: crash, testcase
Product: Firefox → Core
Flags: sec-bounty?
(Assignee)

Comment 4

a year ago
I'll take a look.
Assignee: nobody → hsivonen
Flags: needinfo?(hsivonen)
(Assignee)

Comment 5

a year ago
Created attachment 8749604 [details] [diff] [review]
Treat the root of the stack as being "special" per spec

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)
(Reporter)

Comment 6

a year ago
(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)
Group: core-security → dom-core-security
Whiteboard: btpp-active
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

a year 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?
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+
Attachment #8749604 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 10

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f64adf2773bb89ba6867f60906f390266b6ab4f
Bug 1270381. r=wchen.
(Assignee)

Comment 11

11 months ago
Created attachment 8752585 [details] [diff] [review]
Patch as landed with review comment addressed

The patch applies to branches as-is.
Attachment #8749604 - Attachment is obsolete: true
Attachment #8752585 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5f64adf2773b
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

11 months ago
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 13

11 months 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 14

11 months ago
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+

Comment 15

11 months ago
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
status-firefox47: affected → fixed
status-firefox48: affected → fixed
status-firefox-esr45: affected → fixed

Updated

11 months ago
Group: core-security-release

Updated

11 months ago
Group: dom-core-security
Flags: qe-verify+

Updated

11 months ago
Whiteboard: btpp-active → [adv-main47+][adv-esr45.2+] btpp-active
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
status-firefox47: fixed → verified
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox-esr45: fixed → verified
Flags: qe-verify+

Updated

11 months ago
Alias: CVE-2016-2819
(Assignee)

Comment 17

8 months ago
https://hg.mozilla.org/projects/htmlparser/rev/2fa40a1fc9bc9204e939b65ee4d66bb4f486706c
Mozilla bug 1270381. r=wchen.
(Assignee)

Comment 18

8 months 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.
Group: core-security-release
Keywords: csectype-bounds
You need to log in before you can comment on or make changes to this bug.