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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Atte Kettunen]

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

Comment 1

[Atte Kettunen]

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

Comment 2

[Atte Kettunen]

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.

Comment 3

[Christian Holler (:decoder)]

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)

Comment 4

[Christian Holler (:decoder)]

Attachment: Minimized testcase

Here's a minimized test. I wasn't able to reduce the number of ">", that seems to be required to trigger the failure.

Comment 5

[Jonathan Kew [:jfkthame]]

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!

Comment 6

[Jonathan Kew [:jfkthame]]

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

Comment 7

[Jonathan Kew [:jfkthame]]

Behdad, it looks like this is a genuine harfbuzz bug, if you have a chance to look into it....

Comment 9

[Daniel Veditz [:dveditz]]

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?

Comment 10

[Jonathan Kew [:jfkthame]]

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

Comment 11

[Jonathan Kew [:jfkthame]]

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.

Comment 12

[Behdad Esfahbod]

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

Comment 13

[Behdad Esfahbod]

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

[Behdad Esfahbod]

Committed in upstream HarfBuzz btw.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: patch, fix the indic cluster machine, cherrypicked from upstream

Thanks, Behdad; looks like this is what we need.

Comment 16

[Behdad Esfahbod]

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

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

LGTM.

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/074a6a85dab6

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/074a6a85dab6