1098497 - (CVE-2015-0823) Heap-use-after-free in ots::ots_gasp_parse

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Atte Kettunen]

Attachment: repro-file.html

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1415892131/

ASAN-trace:

==7261==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000128cb0 at pc 0x7f369587823a bp 0x7fff0897db50 sp 0x7fff0897db48
READ of size 2 at 0x603000128cb0 thread T0 (Web Content)
    #0 0x7f3695878239 in ots::ots_gasp_parse(ots::OpenTypeFile*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/gasp.cc:36:0
    #1 0x7f36958d8955 in (anonymous namespace)::ProcessGeneric(ots::OpenTypeFile*, unsigned int, ots::OTSStream*, unsigned char const*, unsigned long, std::vector<(anonymous namespace)::OpenTypeTable, std::allocator<(anonymous namespace)::OpenTypeTable> > const&, ots::Buffer&) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:590:0
    #2 0x7f36958d0411 in (anonymous namespace)::ProcessTTF(ots::OpenTypeFile*, ots::OTSStream*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:237:0
    #3 0x7f36958cd7a5 in ots::OTSContext::Process(ots::OTSStream*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:824:0
    #4 0x7f36902d64f3 in gfxUserFontEntry::SanitizeOpenTypeData(unsigned char const*, unsigned int, unsigned int&, gfxUserFontType) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:253:0
    #5 0x7f36902daa19 in gfxUserFontEntry::LoadPlatformFont(unsigned char const*, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:600:0
    #6 0x7f36902d79d4 in gfxUserFontEntry::LoadNextSrc() /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:488:0
    #7 0x7f36902de94f in Load /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:686:0
    #8 0x7f36902de94f in gfxUserFontSet::FindUserFontEntryAndLoad(gfxFontFamily*, gfxFontStyle const&, bool&, bool&) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:891:0
.
.
.
0x603000128cb0 is located 0 bytes inside of 32-byte region [0x603000128cb0,0x603000128cd0)
freed by thread T0 (Web Content) here:
    #0 0x471721 in free _asan_rtl_:0
    #1 0x7f3695877913 in ~OpenTypeGASP /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/gfx/ots/src/../../../dist/include/mozilla/mozalloc.h:232:0
    #2 0x7f3695877913 in ots::ots_gasp_parse(ots::OpenTypeFile*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/gasp.cc:36:0
    #3 0x7f36958d8955 in (anonymous namespace)::ProcessGeneric(ots::OpenTypeFile*, unsigned int, ots::OTSStream*, unsigned char const*, unsigned long, std::vector<(anonymous namespace)::OpenTypeTable, std::allocator<(anonymous namespace)::OpenTypeTable> > const&, ots::Buffer&) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:590:0
    #4 0x7f36958d0411 in (anonymous namespace)::ProcessTTF(ots::OpenTypeFile*, ots::OTSStream*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:237:0
    #5 0x7f36958cd7a5 in ots::OTSContext::Process(ots::OTSStream*, unsigned char const*, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/ots/src/ots.cc:824:0
    #6 0x7f36902d64f3 in gfxUserFontEntry::SanitizeOpenTypeData(unsigned char const*, unsigned int, unsigned int&, gfxUserFontType) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxUserFontSet.cpp:253:0
.
.
.

Comment 1

[John Daggett (:jtd)]

This is a macro expansion bug.

The macro callsite here uses a struct member var 'gasp->version':

This is a macro expansion bug.

The macro callsite here uses a struct member var 'gasp->version':

  if (gasp->version > 1) {
    // Lots of Linux fonts have bad version numbers...
    uint16_t version = gasp->version;
    DROP_THIS_TABLE("bad version: %u", version);
    return true;
  }

But the macro expands to code that deletes the struct before it's used:

#define DROP_THIS_TABLE(...) \
  do { \
    delete file->gasp; \
    file->gasp = 0; \
    OTS_FAILURE_MSG_(file, TABLE_NAME ": " __VA_ARGS__); \
    OTS_FAILURE_MSG("Table discarded"); \
  } while (0)



  if (gasp->version > 1) {
    // Lots of Linux fonts have bad version numbers...
    uint16_t version = gasp->version;
    DROP_THIS_TABLE("bad version: %u", version);
    return true;
  }

But the macro expands to code that deletes the struct before it's used:

#define DROP_THIS_TABLE(...) \
  do { \
    delete file->gasp; \
    file->gasp = 0; \
    OTS_FAILURE_MSG_(file, TABLE_NAME ": " __VA_ARGS__); \
    OTS_FAILURE_MSG("Table discarded"); \
  } while (0)

Comment 2

[John Daggett (:jtd)]

Attachment: patch, eliminate use-after-free in OTS gasp handling code

Comment 3

[Jonathan Kew [:jfkthame]]

Checking for DROP_THIS_TABLE across the OTS code shows that the same issue is present in several other files as well; they all need to be fixed similarly.

In general, though, I think I'd prefer to fix this by rearranging the contents of the macro so that it outputs the messages before deleting the table. That way it's not such a footgun, and the callsites won't need to be audited/fixed.

Also, OTS issues should be reported upstream, please: https://github.com/khaledhosny/ots/.

Comment 4

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

John, can you suggest a security rating for this bug?

Comment 5

[Khaled Hosny]

I agree with comment #3, will just do so.

Comment 6

[Khaled Hosny]

https://github.com/khaledhosny/ots/commit/003c62d28ae438aa8943cb31535563397f838a2c

Comment 7

[John Daggett (:jtd)]

(In reply to Al Billings [:abillings] from comment #4)
> John, can you suggest a security rating for this bug?

Sorry, no idea what to suggest here. Jonathan?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #7)
> (In reply to Al Billings [:abillings] from comment #4)
> > John, can you suggest a security rating for this bug?
> 
> Sorry, no idea what to suggest here. Jonathan?

Marking this as sec-low. AFAICS, there's no actual security risk here. ASAN is right that we read a value from a block that's been freed, but the only thing we do with it is include the value in a dev console message through a "%u" format specifier. That's pretty harmless.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Update OTS to latest upstream (at commit 7091a232a10a741591ff25158c98ca3f0c319cf9) to pick up recent fixes.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47f88e6ae34c

Comment 11

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

As a sec-low, this isn't bounty eligible so I'm minusing it now.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47f88e6ae34c