ASAN: heap-buffer-overflow in harfbuzz indic cluster machine

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Atte Kettunen, Assigned: jfkthame)

Tracking

({regression, testcase, valgrind})

Trunk
mozilla13
x86_64
Linux
regression, testcase, valgrind
Points:
---

Firefox Tracking Flags

(firefox12 unaffected)

Details

(Whiteboard: [asan][sg:nse])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 599700 [details]
Repro-file

Repro-file as attachment.

==7601== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f2f80e5c708 at pc 0x7f2faa51e202 bp 0x7fff15e84bf0 sp 0x7fff15e84be8
READ of size 4 at 0x7f2f80e5c708 thread T0
    #0 0x7f2faa51e202 in set_cluster /home/attekett/src/objdir-ff-asan/gfx/harfbuzz/src/hb-ot-shape-complex-indic-machine.rl:90
    #1 0x7f2faa51c4a9 in initial_reordering /home/attekett/src/gfx/harfbuzz/src/hb-ot-shape-complex-indic.cc:674
    #2 0x7f2faa518625 in hb_ot_map_t::apply(unsigned int, int (*)(void*, _hb_buffer_t*, unsigned int, unsigned int), void*, _hb_buffer_t*) const /home/attekett/src/gfx/harfbuzz/src/hb-ot-map.cc:86
0x7f2f80e5c708 is located 8 bytes to the right of 640-byte region [0x7f2f80e5c480,0x7f2f80e5c700)
allocated by thread T0 here:
    #0 0x40a4d9 in realloc ??:0
    #1 0x7f2faa4ef57b in _hb_buffer_t::enlarge(unsigned int) /home/attekett/src/gfx/harfbuzz/src/hb-buffer.cc:103
==7601== ABORTING
Stats: 116M malloced (131M for red zones) by 334463 calls
Stats: 7M realloced by 20139 calls
Stats: 87M freed by 219595 calls
Stats: 0M really freed by 0 calls
Stats: 280M (71715 full pages) mmaped in 70 calls
  mmaps   by size class: 8:262128; 9:49146; 10:20475; 11:16376; 12:3072; 13:2048; 14:1792; 15:384; 16:384; 17:160; 18:64; 19:8; 20:8;
  mallocs by size class: 8:249537; 9:45967; 10:17925; 11:14718; 12:2219; 13:1677; 14:1523; 15:319; 16:374; 17:132; 18:60; 19:7; 20:5;
  frees   by size class: 8:151108; 9:36934; 10:15074; 11:12044; 12:1534; 13:848; 14:1336; 15:267; 16:308; 17:121; 18:14; 19:4; 20:3;
  rfrees  by size class:
Stats: malloc large: 204 small slow: 1735
Shadow byte and word:
  0x1fe5f01cb8e1: fa
  0x1fe5f01cb8e0: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x1fe5f01cb8c0: 00 00 00 00 00 00 00 00
  0x1fe5f01cb8c8: 00 00 00 00 00 00 00 00
  0x1fe5f01cb8d0: 00 00 00 00 00 00 00 00
  0x1fe5f01cb8d8: 00 00 00 00 00 00 00 00
=>0x1fe5f01cb8e0: fa fa fa fa fa fa fa fa
  0x1fe5f01cb8e8: fa fa fa fa fa fa fa fa
  0x1fe5f01cb8f0: fa fa fa fa fa fa fa fa
  0x1fe5f01cb8f8: fa fa fa fa fa fa fa fa
  0x1fe5f01cb900: fa fa fa fa fa fa fa fa
(Reporter)

Comment 1

6 years ago
I'll minimize the reprofile ASAP.
Firefox version 13.0a1 - built with ASAN

Updated

6 years ago
Component: Security → Graphics
Product: Firefox → Core
QA Contact: firefox → thebes

Updated

6 years ago
Attachment #599700 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 2

6 years ago
Doesn't repro on Windows 7 x64 with Nightly build. Repro-file has something weird going with encoding. Linux gedit shows the content in unicode. If I edit the file and save, the crash doesn't reproduce.
Confirmed. And this can also be seen in Valgrind (Debug build, m-c rev 307a032f2007):


==10012== Invalid read of size 4
==10012==    at 0x7920876: set_cluster (hb-ot-shape-complex-indic-machine.rl:90)
==10012==    by 0x79210F6: initial_reordering(hb_ot_map_t const*, _hb_face_t*, _hb_buffer_t*, void*) (hb-ot-shape-complex-indic-machine.rl:67)
==10012==    by 0x791FB90: hb_ot_map_t::apply(unsigned int, int (*)(void*, _hb_buffer_t*, unsigned int, unsigned int), void*, _hb_buffer_t*) const (hb-ot-map.cc:91)
==10012==    by 0x7921D40: hb_ot_shape (hb-ot-map-private.hh:68)
==10012==    by 0x7922743: hb_shape_full (hb-shape.cc:128)
==10012==    by 0x7819B6C: gfxHarfBuzzShaper::ShapeWord(gfxContext*, gfxShapedWord*, unsigned short const*) (gfxHarfBuzzShaper.cpp:889)
==10012==    by 0x7821130: gfxFcFont::ShapeWord(gfxContext*, gfxShapedWord*, unsigned short const*, bool) (gfxPangoFonts.cpp:2246)
==10012==    by 0x780B6EA: gfxShapedWord* gfxFont::GetShapedWord<unsigned short>(gfxContext*, unsigned short const*, unsigned int, unsigned int, int, int, unsigned int) (gfxFont.cpp:1948)
==10012==    by 0x780B901: bool gfxFont::SplitAndInitTextRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int, unsigned int, int) (gfxFont.cpp:2141)
==10012==    by 0x780C665: void gfxFontGroup::InitScriptRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int, unsigned int, int) (gfxFont.cpp:3241)
==10012==    by 0x780CC6C: void gfxFontGroup::InitTextRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int) (gfxFont.cpp:3188)
==10012==    by 0x780CDA8: gfxFontGroup::MakeTextRun(unsigned short const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) (gfxFont.cpp:3093)
==10012==  Address 0x1a999898 is 8 bytes after a block of size 640 alloc'd
==10012==    at 0x4C28F9F: malloc (vg_replace_malloc.c:236)
==10012==    by 0x4C29019: realloc (vg_replace_malloc.c:525)
==10012==    by 0x7915C0D: _hb_buffer_t::enlarge(unsigned int) (hb-buffer.cc:100)
==10012==    by 0x7916661: hb_buffer_add_utf16 (hb-buffer.cc:843)
==10012==    by 0x7819B54: gfxHarfBuzzShaper::ShapeWord(gfxContext*, gfxShapedWord*, unsigned short const*) (gfxHarfBuzzShaper.cpp:887)
==10012==    by 0x7821130: gfxFcFont::ShapeWord(gfxContext*, gfxShapedWord*, unsigned short const*, bool) (gfxPangoFonts.cpp:2246)
==10012==    by 0x780B6EA: gfxShapedWord* gfxFont::GetShapedWord<unsigned short>(gfxContext*, unsigned short const*, unsigned int, unsigned int, int, int, unsigned int) (gfxFont.cpp:1948)
==10012==    by 0x780B901: bool gfxFont::SplitAndInitTextRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int, unsigned int, int) (gfxFont.cpp:2141)
==10012==    by 0x780C665: void gfxFontGroup::InitScriptRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int, unsigned int, int) (gfxFont.cpp:3241)
==10012==    by 0x780CC6C: void gfxFontGroup::InitTextRun<unsigned short>(gfxContext*, gfxTextRun*, unsigned short const*, unsigned int) (gfxFont.cpp:3188)
==10012==    by 0x780CDA8: gfxFontGroup::MakeTextRun(unsigned short const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) (gfxFont.cpp:3093)
==10012==    by 0x6C7CBD9: BuildTextRunsScanner::BuildTextRunForFrames(void*) (nsTextFrameThebes.cpp:560)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase, valgrind
Whiteboard: [asan]
Created attachment 599754 [details]
Minimized testcase

Here's a minimized test. I wasn't able to reduce the number of "&gt;", that seems to be required to trigger the failure.
(Assignee)

Comment 5

6 years ago
The first thing that's weird here is that we hit code from hb-ot-shape-complex-indic-machine.rl at all, as we're not supposed to be using harfbuzz for Indic scripts at this point (unless you've changed the gfx.font_rendering.harfbuzz.scripts preference). So maybe we're not dispatching properly to the various shapers.

The invalid read is of course also a problem that we need to get fixed - but we shouldn't have been ending up on that codepath at all for now!
(Assignee)

Comment 6

6 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> The first thing that's weird here is that we hit code from
> hb-ot-shape-complex-indic-machine.rl at all, as we're not supposed to be
> using harfbuzz for Indic scripts at this point (unless you've changed the
> gfx.font_rendering.harfbuzz.scripts preference). So maybe we're not
> dispatching properly to the various shapers.

Ah.... it's the characters from the Balinese and Buhid scripts, which we don't currently recognize how to categorize, but harfbuzz treats as Indic (although I don't think they're really supported yet). So that's ok - they may not shape 100% properly, I expect, but at least it's legitimate for us to be in this code.
(Assignee)

Comment 7

6 years ago
Behdad, it looks like this is a genuine harfbuzz bug, if you have a chance to look into it....
Who else uses harfbuzz that we'd need to coordinate an advisory with (if this turns out to be exploitable)?

jfkthame: what are we reading? Is it just shape data (mostly harmless) or an object that we'd call functions on?
(Assignee)

Comment 10

6 years ago
Created attachment 600455 [details] [diff] [review]
patch, avoid potentially reading from an invalid glyph info in set_cluster()

If I'm understanding correctly, the bug is that the set_cluster function can get called with start >= end, and in that situation, it tries to read cluster information from the buffer position [start]. It then returns harmlessly, because the two following loops don't execute any iterations, but the initial read itself may have been invalid.

So I don't believe this is exploitable, beyond the possibility of causing a crash if the invalid read happens to cross into unmapped address space or something like that.

*Untested* patch attached that I think will prevent the problem; if someone with the appropriate tools could verify this (as valgrind isn't currently co-operating on my system), that would be great.
Attachment #600455 - Flags: review?(mozilla)
(Assignee)

Comment 11

6 years ago
Regarding exploitability, note that the code in question only recently landed in our tree (bug 695857, 2012-02-15), so it is not present in any channels except Nightly builds.
Assignee: nobody → jfkthame
Blocks: 695857
Group: core-security
status-firefox12: --- → unaffected
status-firefox13: --- → affected
Keywords: regression
Whiteboard: [asan] → [asan][sg:nse]

Comment 12

6 years ago
Somehow I have had missed this bug.  Let me get on it.

Comment 13

6 years ago
Jonathan, I think the fix is actually this:


-action next_syllable { set_cluster (buffer, p, last); last = p; }
+action next_syllable { set_cluster (buffer, last, p); last = p; }

Committing after some testing.

Comment 14

6 years ago
Committed in upstream HarfBuzz btw.

Updated

6 years ago
Attachment #600455 - Flags: review?(mozilla) → review-
(Assignee)

Comment 15

6 years ago
Created attachment 603208 [details] [diff] [review]
patch, fix the indic cluster machine, cherrypicked from upstream

Thanks, Behdad; looks like this is what we need.
Attachment #600455 - Attachment is obsolete: true
Attachment #603208 - Flags: review?(mozilla)

Comment 16

6 years ago
Comment on attachment 603208 [details] [diff] [review]
patch, fix the indic cluster machine, cherrypicked from upstream

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

LGTM.
Attachment #603208 - Flags: review?(mozilla) → review+
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/074a6a85dab6
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/074a6a85dab6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox13: affected → ---

Updated

5 years ago
Summary: ASAN: heap-buffer-overflow HTML → ASAN: heap-buffer-overflow in harfbuzz indic cluster machine
You need to log in before you can comment on or make changes to this bug.