1569493 - AddressSanitizer: heap-buffer-overflow READ of size 2 graphite2

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P3)

Description

Attachment: graphite2.ttf

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36

Steps to reproduce:

Can you check if Firefox is affected?

./gr2fonttest graphite2.ttf -auto

Actual results:

=================================================================
==13801==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000002a8 at pc 0x7fdde25ab64a bp 0x7ffc4196e2a0 sp 0x7ffc4196e290
READ of size 2 at 0x6020000002a8 thread T0
    #0 0x7fdde25ab649 in (anonymous namespace)::direct_run(bool, void* const*, unsigned char const*, int*, graphite2::Slot**&, unsigned char, graphite2::vm::Machine::status_t&, graphite2::SlotMap*) (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x5649)
    #1 0x7fdde25aab08 in graphite2::vm::Machine::run(void* const*, unsigned char const*, graphite2::Slot**&) (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x4b08)
    #2 0x7fdde25f09d6 in graphite2::Pass::findNDoRule(graphite2::Slot*&, graphite2::vm::Machine&, graphite2::FiniteStateMachine&) const (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x4a9d6)
    #3 0x7fdde25f6371 in graphite2::Pass::runGraphite(graphite2::vm::Machine&, graphite2::FiniteStateMachine&, bool) const (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x50371)
    #4 0x7fdde26033a6 in graphite2::Silf::runGraphite(graphite2::Segment*, unsigned char, unsigned char, int) const (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x5d3a6)
    #5 0x7fdde25d4a19 in graphite2::Face::runGraphite(graphite2::Segment*, graphite2::Silf const*) const (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x2ea19)
    #6 0x7fdde25b36d0 in gr_make_seg (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0xd6d0)
    #7 0x55b328a62642 in Parameters::testFileFont() const (/home/rs/dev/graphite/build/gr2fonttest/gr2fonttest+0x6642)
    #8 0x55b328a60640 in main (/home/rs/dev/graphite/build/gr2fonttest/gr2fonttest+0x4640)
    #9 0x7fdde20b1eca in __libc_start_main (/lib64/libc.so.6+0x23eca)
    #10 0x55b328a60c19 in _start (/home/rs/dev/graphite/build/gr2fonttest/gr2fonttest+0x4c19)

0x6020000002a8 is located 16 bytes to the right of 8-byte region [0x602000000290,0x602000000298)
allocated by thread T0 here:
    #0 0x7fdde270bbf0 in __interceptor_realloc /var/tmp/portage/sys-devel/gcc-8.3.0-r1/work/gcc-8.3.0/libsanitizer/asan/asan_malloc_linux.cc:105
    #1 0x7fdde25f911d in graphite2::Segment::newSlot() [clone .part.71] (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x5311d)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/rs/dev/graphite/build/src/libgraphite2.so.3+0x5649) in (anonymous namespace)::direct_run(bool, void* const*, unsigned char const*, int*, graphite2::Slot**&, unsigned char, graphite2::vm::Machine::status_t&, graphite2::SlotMap*)
Shadow bytes around the buggy address:
  0x0c047fff8000: fa fa 00 04 fa fa fd fd fa fa 00 02 fa fa 00 02
  0x0c047fff8010: fa fa 00 02 fa fa 00 02 fa fa fd fa fa fa 00 00
  0x0c047fff8020: fa fa 01 fa fa fa 00 04 fa fa 04 fa fa fa 00 00
  0x0c047fff8030: fa fa 00 00 fa fa 04 fa fa fa 00 04 fa fa 00 fa
  0x0c047fff8040: fa fa 02 fa fa fa 00 fa fa fa 01 fa fa fa 00 fa
=>0x0c047fff8050: fa fa 00 fa fa[fa]fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==13801==ABORTING

Comment 1

[Francisco A.]

Attachment: Asan log (symbols)

Comment 2

[martin_hosken]

Fixed in mozilla security repo at fd20fb69c3 please tell me when I can make this public. BTW mozilla security now equals master + this fix so can be pulled from. The bug was enabled with master revision a6d6346a8e475e565c3c36117525daa6d195543f committed on 12/Feb/2019. If firefox is using code before this (e.g. 1.3.13) it is not affected and I can release the fix publicly.

Comment 3

[Francisco A.]

Attachment: second_asan_log

Also:

Comment 4

[Francisco A.]

Attachment: graphite2_2.ttf (second stacktrace)

Comment 5

[Jonathan Kew [:jfkthame]]

AFAICS we're currently on 1.3.13 (from bug 1515629), so should be unaffected by this according to comment 2.

Comment 6

[Tyson Smith [:tsmith]]

Hi Martin, I can confirm with the patch applied to the main graphite2 repro the issue does not reproduce with the attached test cases.

Comment 7

[martin_hosken]

Thanks. Fix pushed to public master.

Comment 8

[Francisco A.]

Thank you, I'm glad it didn't land into a new release and caught it before firefox got affected by.

Comment 10

[Tyson Smith [:tsmith]]

Jonathan would this be mitigated by OTS?

Comment 11

[Francisco A.]

I believe it does not, unless I missed something. It was checked before bug report with OTS :

WARNING: maxp: Bad maxZones: 0
ERROR: cmap: Range glyph reference too high (6 > 5)
ERROR: cmap: Failed to parse format 4 cmap subtable 0
ERROR at ../src/ots.cc:697 (ProcessGeneric)
ERROR: cmap: Failed to parse table
Failed to sanitize file!

Comment 12

[Jonathan Kew [:jfkthame]]

Curious -- I see different error messages from OTS when I try running it on the two font files here:

$ ./ots-sanitize graphite2.ttf 
ERROR at ../src/ots.cc:223 (ProcessTTF)
WARNING: bad search range
ERROR at ../src/ots.cc:616 (ProcessGeneric)
ERROR: OS/O: invalid table offset
Failed to sanitize file!

$ ./ots-sanitize graphite2_2.ttf 
WARNING: Silf: SILSub: Nonzero attrMirroring (reserved before v4)
WARNING: Silf: SILSub: Nonzero attrSkipPasses (reserved2 before v4)
WARNING: Silf: SILSub: lbGID 65535 outside range 0..688, replaced with 0
File sanitized successfully!

Possibly a different OTS version? (I used 7.1.10, but I get the same result after reverting to 7.1.9, which is what we currently have in Gecko.)

So the first of the files is rejected as corrupt; the second is accepted with warnings and an on-the-fly fixup, but I don't know whether this would affect the issue here or not.

Comment 13

[Francisco A.]

I've tested against ots 7.1.10

$ sha1sum $HOME/graphite2_2.ttf fuzzer@fuzz01
fcb0529393473bd3abdaaeaf56c3dbd5af89739d /home/fuzzer/graphite2_2.ttf

$ ./build/ots-sanitize $HOME/graphite2_2.ttf result.ttf
WARNING: Silf: SILSub: Nonzero attrMirroring (reserved before v4)
WARNING: Silf: SILSub: Nonzero attrSkipPasses (reserved2 before v4)
WARNING: Silf: SILSub: lbGID 65535 outside range 0..688, replaced with 0
File sanitized successfully!

$ ./home/fuzzer/dev/graphite2-static/build/gr2fonttest/gr2fonttest result.ttf -auto -noprint

==29630==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000000398 at pc 0x7fb36828927e bp 0x7ffc89343140 sp 0x7ffc89343138
READ of size 4 at 0x611000000398 thread T0
... same stacktrace

Comment 14

[Jonathan Kew [:jfkthame]]

OK, that confirms that OTS wouldn't have helped in this case (if the buggy graphite commit had been pulled into gecko).

Comment 16

[Francisco A.]

(In reply to Daniel Veditz [:dveditz] from comment #15)

Tricky for bounty consideration because we don't currently have this code, but we would have, but it's likely our fuzzers would have found it because it was found multiple times when we did test with our fuzzers.

Sad to read that, because I understand that you prefer not to report a vulnerability until it landed in your code (when it was evident that it would happened). So this issue was introduced in February of this year, 5-6 months ago. I could have waited for the release and it may or may not be detected by your fuzzers (Many of the bugs reported by others could be detected by your fuzzers, but there are many cases in which that didn't happen. See previous bugs of mine where you use domato and me too). I don't think that should be taken into consideration whether or not you would have detected this. Because I believe also your team should be fuzzing both branches, development and release or monitor upstream projects commits.

Also I don't agree with sec-moderate, when seeing most of the previous (oobr, eg P0) graphite2 issues that besides being mitigated by OTS (in any case this is not) are sec-high.

Comment 17

[Tyson Smith [:tsmith]]

Regarging the fuzzers finding this, we have many fuzzers running against many third party libraries and with out getting too political resources are not free. We cannot run all our fuzzers against all of our targets all of the time. We do try to leverage other resources such as oss-fuzz where possible (https://github.com/google/oss-fuzz/pull/1824). However in this specific case the integration of that target is beyond the control of Mozilla. In that pull request there are two fuzzing targets one of which will find the bug almost immediately. Typically when a bug is open to integrate a new version of upstream code in to our code base we will allocate fuzzing resources. The amount of resources depends on a few factors such as attack surface, code churn and previous (if not ongoing) fuzzing. I'm not trying to make excuses I am just trying to be as open about our process as possible.

Comment 18

[Tyson Smith [:tsmith]]

Another thing to mention about the value of this bug is that it does highlight a weak spot in OTS. I would have thought with the recent graphite support work we would have had better protection.

Comment 19

[Francisco A.]

(In reply to Tyson Smith [:tsmith] from comment #18)

Another thing to mention about the value of this bug is that it does highlight a weak spot in OTS. I would have thought with the recent graphite support work we would have had better protection.

That's a plus of why sec-moderate still makes no sense.

I do not take credit apart from your fuzzers or your work/team/s, and I understand limit resources (as I've limited resources aswell). I just say that in this case, to take that into account as consideration for the bug bounty does not make sense, when I have done that task in advance and in addition giving your team two repros to make it clear that OTS did not mitigate this issue.

Comment 20

[Daniel Veditz [:dveditz]]

Small Out-of-bounds READs of non-user data in graphics code are quite often deemed sec-moderate. If it's a read that influences the location of a future write, or if it incorporates gobs of random memory (possibly sensitive user data) that can be recovered by the web page then it would be more severe.

Comment 21

[Daniel Veditz [:dveditz]]

(Many of the bugs reported by others could be detected by your fuzzers, but there are many cases in which that didn't happen. See previous bugs of mine where you use domato and me too)

That's the part that makes it tricky. There's no way to know.

Comment 22

[Francisco A.]

(In reply to Daniel Veditz [:dveditz] from comment #20)

Small Out-of-bounds READs of non-user data in graphics code are quite often deemed sec-moderate. If it's a read that influences the location of a future write, or if it incorporates gobs of random memory (possibly sensitive user data) that can be recovered by the web page then it would be more severe.

I would assume that not all of those cases are true. eg:

#1243482
#1243513
#1243526
#1243823
#1249338
#1249920
#1248804

Comment 23

[Francisco A.]

I don't see those issues checked against OTS anyway, at least not in the comments. So could have been mitigated or not. And I see OTS a factor to take in mind to rate an issue.

Comment 24

[Francisco A.]

Attachment: heap-buffer-8.ttf

==302==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000340 at pc 0x7fb77b2a7fb2 bp 0x7ffd45b7e500 sp 0x7ffd45b7e4f8
READ of size 8 at 0x602000000340 thread T0
#0 0x7fb77b2a7fb1 in graphite2::Slot::userAttrs() const /home/fuzzer/dev/graphite2-static/src/inc/Slot.h:112:39
#1 0x7fb77b2a4c9c in (anonymous namespace)::direct_run(bool, void* const*, unsigned char const*, int*, graphite2::Slot**&, unsigned char, graphite2::vm::Machine::status_t&, graphite2::SlotMap*) /home/fuzzer/dev/graphite2-static/src/inc/opcodes.h:257:40
#2 0x7fb77b2a7bd7 in graphite2::vm::Machine::run(void* const*, unsigned char const*, graphite2::Slot**&) /home/fuzzer/dev/graphite2-static/src/direct_machine.cpp:136:17
#3 0x7fb77b2d843b in graphite2::Pass::doAction(graphite2::vm::Machine::Code const*, graphite2::Slot*&, graphite2::vm::Machine&) const /home/fuzzer/dev/graphite2-static/src/Pass.cpp:685:26
#4 0x7fb77b2d692a in graphite2::Pass::findNDoRule(graphite2::Slot*&, graphite2::vm::Machine&, graphite2::FiniteStateMachine&) const /home/fuzzer/dev/graphite2-static/src/Pass.cpp:551:33
#5 0x7fb77b2d6352 in graphite2::Pass::runGraphite(graphite2::vm::Machine&, graphite2::FiniteStateMachine&, bool) const /home/fuzzer/dev/graphite2-static/src/Pass.cpp:420:13
#6 0x7fb77b2e2fba in graphite2::Silf::runGraphite(graphite2::Segment*, unsigned char, unsigned char, int) const /home/fuzzer/dev/graphite2-static/src/Silf.cpp:431:33
#7 0x7fb77b2aba46 in (anonymous namespace)::makeAndInitialize(graphite2::Font const*, graphite2::Face const*, unsigned int, graphite2::FeatureVal const*, gr_encform, void const*, unsigned long, int) /home/fuzzer/dev/graphite2-static/src/gr_segment.cpp:46:73
#8 0x7fb77b2ab95d in gr_make_seg /home/fuzzer/dev/graphite2-static/src/gr_segment.cpp:110:24
#9 0x4fdcc0 in Parameters::testFileFont() const /home/fuzzer/dev/graphite2-static/gr2fonttest/gr2FontTest.cpp:695:20
#10 0x4fe95a in main /home/fuzzer/dev/graphite2-static/gr2fonttest/gr2FontTest.cpp:800:23
#11 0x7fb77b0d3b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a)
#12 0x41d5f9 in _start (/home/fuzzer/dev/graphite2-static/build/gr2fonttest/gr2fonttest+0x41d5f9)

Address 0x602000000340 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/fuzzer/dev/graphite2-static/src/inc/Slot.h:112:39 in graphite2::Slot::userAttrs() const
Shadow bytes around the buggy address:
0x0c047fff8010: fa fa 00 06 fa fa 00 04 fa fa 00 04 fa fa 00 04
0x0c047fff8020: fa fa 00 04 fa fa 00 04 fa fa fd fa fa fa 00 00
0x0c047fff8030: fa fa 00 fa fa fa 01 fa fa fa 00 04 fa fa 04 fa
0x0c047fff8040: fa fa 00 00 fa fa 00 00 fa fa 04 fa fa fa 00 fa
0x0c047fff8050: fa fa 02 fa fa fa 00 fa fa fa 01 fa fa fa 00 fa
=>0x0c047fff8060: fa fa 00 fa fa fa fa fa[fa]fa fa fa fa fa fa fa
0x0c047fff8070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff8080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff8090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==302==ABORTING

Comment 25

[Tyson Smith [:tsmith]]

(In reply to Francisco A. from comment #23)

I don't see those issues checked against OTS anyway, at least not in the comments. So could have been mitigated or not. And I see OTS a factor to take in mind to rate an issue.

FWIW: Graphite support in OTS is relatively new (2017 I believe).

Comment 26

[Francisco A.]

(In reply to Tyson Smith [:tsmith] from comment #25)

(In reply to Francisco A. from comment #23)

I don't see those issues checked against OTS anyway, at least not in the comments. So could have been mitigated or not. And I see OTS a factor to take in mind to rate an issue.

FWIW: Graphite support in OTS is relatively new (2017 I believe).

You are right, commit of 21 Jul 2017 https://github.com/khaledhosny/ots/commit/db561dec42e100828f4bd376540fa5ec975109cb
However, it is clear that OTS is not useful in this case, and must be considered for a sec-rate.

Comment 27

[Daniel Veditz [:dveditz]]

(In reply to Francisco A. from comment #22)

I would assume that not all of those cases are true. eg:

Yup, those all look over-rated to me at a surface glance. Since Tyson works for us and bounties aren't involved we often don't go back and correct mistaken first guesses.

(In reply to Francisco A. from comment #26)

However, it is clear that OTS is not useful in this case, and must be considered for a sec-rate.

Yes. If OTS mitigated the issue then this would be a sec-other or sec-low at best (in Firefox, obviously worse for other graphite consumers).

Comment 28

[Francisco A.]

Okay, I see there isn't much else to discuss. The issue is fixed. Please remove the restrictions of this bug to make it public.

Comment 29

[Francisco A.]

(In reply to Daniel Veditz [:dveditz] from comment #27)

(In reply to Francisco A. from comment #22)

Yup, those all look over-rated to me at a surface glance. Since Tyson works for us and bounties aren't involved we often don't go back and correct mistaken first guesses.

Just a note: bug #1356607
Similar issue, sec-high rated by you and affecting git version.