Closed Bug 1345461 (CVE-2017-5436) Opened 4 years ago Closed 4 years ago
Ref::apply Val To Feature heap overflow
320.68 KB, text/html
33.64 KB, text/plain
135.84 KB, application/vnd.oasis.opendocument.formula-template
1.04 KB, patch
|Details | Diff | Splinter Review|
19.09 KB, text/plain
320.40 KB, text/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
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.
This is the standalone font file that triggers the crash.
./poc2.html#123 should work with the linked build.
Martin, it looks clear that this is an error (fortunately with a simple fix). Do you see anything else needed besides this patch?
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.
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.
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: 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?
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.
sec-approval? was requested in comment 10.
I have updated the poc2 to not require the #123.
Attachment #8844902 - Attachment is obsolete: true
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...
Sorry my fault. I have updated the poc2 to be in line with the original report.
Track 54+/55+ as sec-critical issue.
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8845629 [details] [diff] [review] Adjust vector allocation sec-crit fix for esr52.1 and esr45.9
Jonathan, would this fix benefit from manual testing? Does it have automated coverage?
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.)
(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+
Reproduced on Fx 52, Ubuntu 14.04 x64. Verified fixed on Fx 53b9, 54.0a2 (2017-04-05), 55.0a1 (2017-04-05).
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
That's correct. The fix is due to ship with the 52.1 release.
Status: RESOLVED → VERIFIED
Paul, could you please verify this on 52.1esr as well?
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.
You need to log in before you can comment on or make changes to this bug.