Bug 1345461 (CVE-2017-5436)

Graphite2 FeatureRef::applyValToFeature heap overflow

VERIFIED FIXED in Firefox -esr45

Status

()

VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: hofusec, Assigned: jfkthame)

Tracking

(4 keywords)

unspecified
mozilla55
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr4553+ verified, thunderbird_esr45? fixed, thunderbird_esr52? fixed, firefox52 wontfix, firefox-esr5253+ verified, firefox53+ verified, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Posted file poc1.html
A graphite font with a specially crafted feature table can cause a heap overflow which is exploitable to archive remote code execution.

The vulnerability exists in FeatureRef::applyValToFeature in the graphite 2 library. Before accessing data FeatureRef::applyValToFeature tries to ensure that the capacity of the pDest Vector is sufficient through a call to Vector ::reserve but it reserves one too few because the variable m_index is used for the call to Vector::reserve and for accessing the Vector data. This leads to the possibility of a 1-32 bit overflow because the vector is used as a bit map.

The attached pocs work with https://ftp.mozilla.org/pub/firefox/releases/52.0/linux-x86_64/en-US/firefox-52.0.tar.bz2 and show the possibility to exploit the vulnerability to archive remote code execution:

- poc1 calculates the base address for the xul library
- poc2 crashes with a jump to 0x4141414141414141
(Reporter)

Comment 1

2 years ago
Posted file poc2.html (obsolete) —
Posted file log.txt
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, csectype-bounds, testcase
Hey Holger could you verify that the attached poc2.html file does repro the functionality you describe above?

I am seeing "SyntaxError: missing hexadecimal digits after '0x'  poc2.html:1" when I fix that I can trigger the same out of bounds read in log.txt but not the jump to 0x414141.... Any tips? I used the build you linked to above when trying to repro.
Flags: needinfo?(hofusec)
Group: core-security → gfx-core-security
Posted file test_case.otf
This is the standalone font file that triggers the crash.
(Reporter)

Comment 5

2 years ago
./poc2.html#123 should work with the linked build.
Flags: needinfo?(hofusec)
Martin, it looks clear that this is an error (fortunately with a simple fix). Do you see anything else needed besides this patch?
Attachment #8845629 - Flags: feedback?(martin_hosken)
Posted file gdb_log.txt
I could not trigger the crash @ 0x4141414141414141. I would open the file by entering "file:///path_to_test/poc2.html#123" in the address bar.

Comment 8

2 years ago
This fix looks good and sufficient. I think it was forgotten the a std::vector doesn't grow on [] access :(. Thanks

PS Why doesn't bugzilla offer an 'append my comment to the end of the bug' option for mid air collisions?
I'm going to tag this as sec-critical, given that it may apparently lead to native code execution (even though the current POC may not be reliable -- it probably depends on various details of the environment that affect exactly what the attacker is able to corrupt).

[Tracking Requested - why for this release]:
Potentially sec-critical OOB write, affecting all current products/branches.
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
status-thunderbird_esr45: --- → affected
status-thunderbird_esr52: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
tracking-thunderbird_esr45: --- → ?
tracking-thunderbird_esr52: --- → ?
Keywords: sec-critical
Comment on attachment 8845629 [details] [diff] [review]
Adjust vector allocation

This has just been fixed upstream (1ce331d5548b98ed8b818532b2556d6f2c7a3b83); given the potential risk of an OOB write, and the simplicity of the patch, I think we should go ahead and take the fix across all branches, without waiting for a new upstream release.

I don't know if we have plans for a dot-release for FF52, but if we do, I think this should be considered for inclusion.


[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch makes the off-by-one allocation error pretty obvious. Some understanding of the graphite font table formats would be needed to figure out how to trigger the OOB-write here and construct a font that translates this into an exploit, but it's certainly a risk.

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?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial; this code is the same across all branches AFAICS.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk, this just fixes an off-by-one error in the size of storage allocated for the vector. Hard to see how that could make anything worse. :)
Attachment #8845629 - Flags: sec-approval?
Attachment #8845629 - Flags: review?(jmuizelaar)
Attachment #8845629 - Flags: feedback?(martin_hosken)
Attachment #8845629 - Flags: approval-mozilla-release?
Attachment #8845629 - Flags: approval-mozilla-esr52?
Attachment #8845629 - Flags: approval-mozilla-esr45?
Attachment #8845629 - Flags: approval-mozilla-beta?
Attachment #8845629 - Flags: approval-mozilla-aurora?

Comment 11

2 years ago
I'm not sure which version of the code you are currently using, but in the changes since 1.3.9, there are 3 changes that affect code you use.

* Bug fix for collision avoidance. Therefore rarely used, only one font known to use this.
* One very simple fix to utf8 decoding to handle bad sequences better.
* The bug fix from this bug.

While you are welcome to cherry pick for this one bug, but I would suggest that the other fixes have a low risk of introducing bugs if taken as well. FWIW.

HTH.
Comment on attachment 8845629 [details] [diff] [review]
Adjust vector allocation

Review of attachment 8845629 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice if Graphite was written in Rust
Attachment #8845629 - Flags: review?(jmuizelaar) → review+
Can you ask for sec approval since this affects more than one branch? Thanks.
Flags: needinfo?(jfkthame)
sec-approval? was requested in comment 10.
Flags: needinfo?(jfkthame)
(Reporter)

Comment 15

2 years ago
Posted file poc2.html (obsolete) —
I have updated the poc2 to not require the #123.
Attachment #8844902 - Attachment is obsolete: true
(Reporter)

Comment 16

2 years ago
Posted file gdblog_poc2_ubuntu_16.04.1.LTS.txt (obsolete) —
I have ensured that the poc2 is working correctly with the build https://ftp.mozilla.org/pub/firefox/releases/52.0/linux-x86_64/en-US/firefox-52.0.tar.bz2 at least on ubuntu 16.04.1. The gdb log is attached.
(In reply to Holger Fuhrmannek from comment #16)
> Created attachment 8846793 [details]
> gdblog_poc2_ubuntu_16.04.1.LTS.txt
> 
> I have ensured that the poc2 is working correctly with the build
> https://ftp.mozilla.org/pub/firefox/releases/52.0/linux-x86_64/en-US/firefox-
> 52.0.tar.bz2 at least on ubuntu 16.04.1. The gdb log is attached.

Thanks for the update. To confirm I was able to repro this. Infact I did initially but I was looking for 0x414141... (from original report) not 0xaaaa...
(Reporter)

Comment 18

2 years ago
Posted file poc2.html
Sorry my fault. I have updated the poc2 to be in line with the original report.
Attachment #8846791 - Attachment is obsolete: true
Attachment #8846793 - Attachment is obsolete: true
Track 54+/55+ as sec-critical issue.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
status-firefox52: affected → wontfix
tracking-firefox52: ? → ---
tracking-firefox53: ? → +
tracking-firefox-esr45: ? → 53+
tracking-firefox-esr52: ? → 53+
Comment on attachment 8845629 [details] [diff] [review]
Adjust vector allocation

sec-approval+ for trunk. I've also approved for Aurora and Beta. I'll let Release Management decide about ESR branches.
Attachment #8845629 - Flags: sec-approval?
Attachment #8845629 - Flags: sec-approval+
Attachment #8845629 - Flags: approval-mozilla-beta?
Attachment #8845629 - Flags: approval-mozilla-beta+
Attachment #8845629 - Flags: approval-mozilla-aurora?
Attachment #8845629 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/6fe55ee72096
status-firefox55: affected → fixed
Target Milestone: --- → mozilla55
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Comment on attachment 8845629 [details] [diff] [review]
Adjust vector allocation

sec-crit fix for esr52.1 and esr45.9
Attachment #8845629 - Flags: approval-mozilla-esr52?
Attachment #8845629 - Flags: approval-mozilla-esr52+
Attachment #8845629 - Flags: approval-mozilla-esr45?
Attachment #8845629 - Flags: approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr52/rev/e6132f638311
https://hg.mozilla.org/releases/mozilla-esr45/rev/bcd5e51251dd
status-firefox-esr45: affected → fixed
status-firefox-esr52: affected → fixed
status-thunderbird_esr45: affected → fixed
status-thunderbird_esr52: affected → fixed
Jonathan, would this fix benefit from manual testing? Does it have automated coverage?
Flags: qe-verify?
Flags: needinfo?(jfkthame)
Verifying the fix with the PoC files attached would be great, thanks. (There isn't currently automated coverage, though once the fix is shipped to release I guess we can turn those into crashtests.)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #26)
> Verifying the fix with the PoC files attached would be great, thanks. (There
> isn't currently automated coverage, though once the fix is shipped to
> release I guess we can turn those into crashtests.)

Thank you for following up, Jonathan! We'll make sure to verify it on 52.0.2esr.
Flags: qe-verify? → qe-verify+
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Reproduced on Fx 52, Ubuntu 14.04 x64.
Verified fixed on Fx 53b9, 54.0a2 (2017-04-05), 55.0a1 (2017-04-05).
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox55: fixed → verified
I can still reproduce this on Fx 52.0.2 ESR.
bp-938a9918-f881-4105-ae71-f1b1b2170405
bp-61db2fb1-729a-4cae-8a87-f08d02170405
Is this expected, considering how the tracking flags are set?
tracking-firefox-esr52:	53+
status-firefox-esr52:	fixed
Flags: needinfo?(jfkthame)
That's correct. The fix is due to ship with the 52.1 release.
Flags: needinfo?(jfkthame)
Thanks, Ryan.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Alias: CVE-2017-5436
Paul, could you please verify this on 52.1esr as well?
Flags: needinfo?(paul.silaghi)
Comment on attachment 8845629 [details] [diff] [review]
Adjust vector allocation

This landed on m-b a few weeks ago so is fixed in 53. Clearing the m-r approval flag.
Attachment #8845629 - Flags: approval-mozilla-release? → approval-mozilla-release-
Verified fixed Fx 45.9.0 ESR, 52.1.0 ESR on Win 10, Ubuntu 14.04.
status-firefox-esr45: fixed → verified
status-firefox-esr52: fixed → verified
Flags: needinfo?(paul.silaghi)
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.