1345461 - (CVE-2017-5436) Graphite2 FeatureRef::applyValToFeature heap overflow

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Holger Fuhrmannek]

Attachment: 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

Comment 1

[Holger Fuhrmannek]

Attachment: poc2.html

Comment 2

[Tyson Smith [:tsmith]]

Attachment: log.txt

Comment 3

[Tyson Smith [:tsmith]]

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.

Comment 4

[Tyson Smith [:tsmith]]

Attachment: test_case.otf

This is the standalone font file that triggers the crash.

Comment 5

[Holger Fuhrmannek]

./poc2.html#123 should work with the linked build.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Adjust vector allocation

Martin, it looks clear that this is an error (fortunately with a simple fix). Do you see anything else needed besides this patch?

Comment 7

[Tyson Smith [:tsmith]]

Attachment: 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

[martin_hosken]

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?

Comment 9

[Jonathan Kew [:jfkthame]]

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 10

[Jonathan Kew [:jfkthame]]

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. :)

Comment 11

[martin_hosken]

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 12

[Jeff Muizelaar [:jrmuizel]]

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you ask for sec approval since this affects more than one branch? Thanks.

Comment 14

[Jonathan Kew [:jfkthame]]

sec-approval? was requested in comment 10.

Comment 15

[Holger Fuhrmannek]

Attachment: poc2.html

I have updated the poc2 to not require the #123.

Comment 16

[Holger Fuhrmannek]

Attachment: 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.

Comment 17

[Tyson Smith [:tsmith]]

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

Comment 18

[Holger Fuhrmannek]

Attachment: poc2.html

Sorry my fault. I have updated the poc2 to be in line with the original report.

Comment 19

[Gerry Chang [:gchang]]

Track 54+/55+ as sec-critical issue.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe55ee72096
https://hg.mozilla.org/releases/mozilla-aurora/rev/1831b8da127f
https://hg.mozilla.org/releases/mozilla-beta/rev/7f5193633d2e

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fe55ee72096

Comment 23

[Julien Cristau [:jcristau]]

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

sec-crit fix for esr52.1 and esr45.9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e6132f638311
https://hg.mozilla.org/releases/mozilla-esr45/rev/bcd5e51251dd

Comment 25

[Andrei Vaida [:avaida]]

Jonathan, would this fix benefit from manual testing? Does it have automated coverage?

Comment 26

[Jonathan Kew [: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.)

Comment 27

[Andrei Vaida [:avaida]]

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

Comment 28

[Paul Silaghi, QA [:pauly]]

Reproduced on Fx 52, Ubuntu 14.04 x64.
Verified fixed on Fx 53b9, 54.0a2 (2017-04-05), 55.0a1 (2017-04-05).

Comment 29

[Paul Silaghi, QA [:pauly]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

That's correct. The fix is due to ship with the 52.1 release.

Comment 31

[Paul Silaghi, QA [:pauly]]

Thanks, Ryan.

Comment 32

[Andrei Vaida [:avaida]]

Paul, could you please verify this on 52.1esr as well?

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 34

[Paul Silaghi, QA [:pauly]]

Verified fixed Fx 45.9.0 ESR, 52.1.0 ESR on Win 10, Ubuntu 14.04.