bugThe FreeType Project - Bugs: bug #43538, FreeType 2.5.3 Mac font parsing...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #43538: FreeType 2.5.3 Mac font parsing heap-based buffer overflow due to multiple integer overflows

Submitter:  Mateusz Jurczyk <j00ru>
Submitted:  Wed 05 Nov 2014 01:46:51 PM UTC
 
Severity:  3 - Normal Item Group:  Crash
Status:  Fixed Privacy:  Public
Assigned to:  mpsuzuki Open/Closed:  Closed
Planned Release:  2.5.4
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

( Jump to the original submission )

Wed 26 Nov 2014 08:50:49 PM UTC, comment #16: 

Looks good, thanks!

Mateusz Jurczyk <j00ru>
Project Member
Wed 26 Nov 2014 03:46:43 PM UTC, comment #15: 

Thank you for reviewing and finding my slipping point.
I added additional check "pfb_len + temp + 6 < pfb_len"
to the existing check "temp > 0x7FFFFFFFUL", and committed
to GIT. Please check.

suzuki toshiya <mpsuzuki>
Project Administrator
Wed 26 Nov 2014 11:31:28 AM UTC, comment #14: 

Thanks for the patch! :)

Out of three possible integer overflows I described in my original report in this bug, your patch fixes (1) and (3). However, (2) is not resolved yet: the "pfb_len += temp + 6" expression can still overflow.

Imagine that resource_cnt = 4, and "temp" is read as 0x40000000 in all four cases. Then:

pfb_len += 0x40000000 + 6;
pfb_len += 0x40000000 + 6;
pfb_len += 0x40000000 + 6;
pfb_len += 0x40000000 + 6;

In the end pfb_len will end up as 24 (0x18) due to an overflow. An additional check equivalent to the following one would be sufficient to close this loophole:

if (pfb_len + temp + 6 < pfb_len) {
  // error
}

Mateusz Jurczyk <j00ru>
Project Member
Wed 26 Nov 2014 09:11:49 AM UTC, comment #13: 

Sorry, no need to close the discussion,

suzuki toshiya <mpsuzuki>
Project Administrator
Wed 26 Nov 2014 09:10:08 AM UTC, comment #12: 

Committed to GIT.

suzuki toshiya <mpsuzuki>
Project Administrator
Fri 21 Nov 2014 04:07:54 PM UTC, comment #11: 

Sorry, this week I could not have returned to my lab.
Please let me do in next week, I'm quite sorry.

suzuki toshiya <mpsuzuki>
Project Administrator
Fri 21 Nov 2014 03:31:34 PM UTC, comment #10: 

Ping? :-)

Mateusz Jurczyk <j00ru>
Project Member
Wed 12 Nov 2014 12:41:52 PM UTC, comment #9: 

Sorry for the late response.

Regarding comment #4: I don't have an opinion w.r.t. itemĀ 1, so please proceed as you think is best.

For itemĀ 2, I think that the introduction of FT_INT32_MAX makes sense only if there is more than a single place where we can use it.  Otherwise a simple comment is fully sufficient.

And thanks to both of you for working on those issues!

Werner LEMBERG <wl>
Project Administrator
Wed 12 Nov 2014 12:16:15 PM UTC, comment #8: 

Thanks for uploading the patch! Just to make sure: is it the only patch to fix bugs #43538 (this one) and #43539, or is it going to be applied on top of Comment #2 in this bug (https://savannah.nongnu.org/bugs/?43538#comment2)?

Mateusz Jurczyk <j00ru>
Project Member
Wed 12 Nov 2014 11:24:15 AM UTC, comment #7: 

Oh, sorry. The error you received is similar to one that
I've received when I've not logged in Savannah.
Anyway, I uploaded my patch at:
http://gyvern.ipc.hiroshima-u.ac.jp/~mpsuzuki/mps20141111e.diff
Please check, thanks in advance!

suzuki toshiya <mpsuzuki>
Project Administrator
Wed 12 Nov 2014 10:26:58 AM UTC, comment #6: 

Hi,

Thanks for the comments and patches. :) Unfortunately, similarly to the other ticket, I am getting the following message when trying to download the attached patch:

"Non-authorized access to file attached to private item."

Could you please paste it in verbatim in a comment, or attach it elsewhere? As soon as I have access to the patches, I'll let you know if I think they are sufficient.

Thanks,
Mateusz

Mateusz Jurczyk <j00ru>
Project Member
Tue 11 Nov 2014 04:26:21 AM UTC, comment #5: 

Here is a patch using FT_ULong types to handle
the lengths written in POST fragments. There is
no explicit limit check to fit 0x7FFFFFFF (and
no check for the overflow of unsigned long).
Before each writings to the allocated buffer,
the room of the buffer is checked.

(file #32411)

suzuki toshiya <mpsuzuki>
Project Administrator
Tue 11 Nov 2014 01:19:33 AM UTC, comment #4: 

Thank you for comment, oops, I uploaded a concatenated
patch at #43539, before checking your comment.

1)
Coincidentally, I thought using unsigned 32-bit type would
be safer solution while I was thinking about the easiest
fix. At present I could not remember why these values are
loaded as signed integer - I guess InsideMacintosh or
some Adobe Technical Note defined these values as the
signed types, although there is no reason to use signed
types (i.e. in FontManager Reference of InsideMacintosh:Text,
there were so many signed offset, signed number-of-tables,
signed number-of-items, etc etc). I've not tested what occurs
when these values are negative. Werner, how do you think
of? Should I use the unsigned types and add a configurable
limit checker to prevent 0x7FFF or 0x7FFFFFFF which could
be optimized-out?

2)
Umm, INT_MAX could be 0x7FFF on 16-bit systems, or,
0x7FFFFFFFFFFFFFFF on ILP64 systems. I want to keep
0x7FFFFFFF. Anyway, splashing such magic numbers in
various places is bad idea. If possible, I want to introduce
a macro something like FT_INT32_MAX. Werner, how do you
think of?

suzuki toshiya <mpsuzuki>
Project Administrator
Mon 10 Nov 2014 05:18:44 PM UTC, comment #3: 

Hi Suzuki Toshiya,

The patch looks alright, but I think it would also make sense to do the following:

1) Change the types of len, pfb_len, pfb_pos, pfb_lenpos, rlen, temp from FT_Long to FT_ULong. As far as I am concerned, there's no reason for the variables to be of a signed type, and it only makes it easier for integer handling vulnerabilities to slip in.

2) Change the 0x7FFFFFFFL constant to INT_MAX for better portability (on any 32/64-bit GNU/Linux platform, this will still translate to 0x7FFFFFFFL).

Please let me know if the above sounds good to you.

Mateusz Jurczyk <j00ru>
Project Member
Mon 10 Nov 2014 02:41:59 AM UTC, comment #2: 

Dear Mateusz,
Thank you for finding this issue, and giving a sample font to debuug.
Although this issue is caused by the fonts designed for MacOS,
it affects on most POSIX platforms (because the code in ftobjs.c is
not platform independent). I could reproduce this issue on GNU/Linux
i386 with valgrind. I added a small check to limit the total length
within 0x7FFFFFFFL, it fixes the overrunning writings in valgrind.
Please review - if no problem is found, I will apply this patch within
this week.

diff --git a/src/base/ftobjs.c b/src/base/ftobjs.c
index cc56105..ee6051d 100644
--- a/src/base/ftobjs.c
+++ b/src/base/ftobjs.c
@@ -1601,12 +1601,25 @@
       error = FT_Stream_Seek( stream, offsets[i] );
       if ( error )
         goto Exit;
-      if ( FT_READ_LONG( temp ) )
+      else if ( FT_READ_LONG( temp ) )
         goto Exit;
-      pfb_len += temp + 6;
+      else if ( 0 > temp )
+        error = FT_THROW( Invalid_Offset );
+      else if ( 0x7FFFFFFFL - pfb_len < temp + 6 )
+        error = FT_THROW( Array_Too_Large );
+
+      if ( error )
+        goto Exit;
+      else
+        pfb_len += temp + 6;
     }

-    if ( FT_ALLOC( pfb_data, (FT_Long)pfb_len + 2 ) )
+    if ( 0x7FFFFFFFL - 2 < pfb_len )
+      error = FT_THROW( Array_Too_Large );
+    else
+      error = FT_ALLOC( pfb_data, (FT_Long)pfb_len + 2 );
+
+    if ( error )
       goto Exit;

     pfb_data[0] = 0x80;

(file #32396)

suzuki toshiya <mpsuzuki>
Project Administrator
Wed 05 Nov 2014 02:04:35 PM UTC, comment #1: 

Toshiya-san, please have a look.

Werner LEMBERG <wl>
Project Administrator
Wed 05 Nov 2014 01:46:51 PM UTC, original submission:  

In the freetype/src/base/ftobjs.c file, we can find multiple auxiliary functions for handling uncommon or exotic font formats. One such function is "Mac_Read_POST_Resource", which heavily operates on user-supplied data:

1586:    FT_Long    len;
1587:    FT_Long    pfb_len, pfb_pos, pfb_lenpos;
1588:    FT_Long    rlen, temp;
...
1596:    /* Find the length of all the POST resources, concatenated.  Assume */
1597:    /* worst case (each resource in its own section).                   */
1598:    pfb_len = 0;
1599:    for ( i = 0; i < resource_cnt; ++i )
1600:    {
1601:      error = FT_Stream_Seek( stream, offsets[i] );
1602:      if ( error )
1603:        goto Exit;
1604:      if ( FT_READ_LONG( temp ) )
1605:        goto Exit;
1606:      pfb_len += temp + 6;
1607:    }
1608:
1609:    if ( FT_ALLOC( pfb_data, (FT_Long)pfb_len + 2 ) )
1610:      goto Exit;
1611:
1612:    pfb_data[0] = 0x80;
1613:    pfb_data[1] = 1;            /* Ascii section */
1614:    pfb_data[2] = 0;            /* 4-byte length, fill in later */
1615:    pfb_data[3] = 0;
1616:    pfb_data[4] = 0;
1617:    pfb_data[5] = 0;
1618:    pfb_pos     = 6;
1619:    pfb_lenpos  = 2;

In the above code, there are multiple instances of integer handling problems:

1. There is an integer overflow in the "temp + 6" expression in line 1606.
2. There is an integer overflow in the "pfb_len += temp + 6" expression, overflowing the "pfb_len" variable in line 1606.
3. There is an integer overflow in the "(FT_Long)pfb_len + 2" expression in line 1609.

All of the above problems can be used on 32-bit builds to allocate fewer bytes for the "pfb_data" buffer than is actually required and therefore trigger a heap-based buffer overflow. This is illustrated by the attached example proof-of-concept sample (1.ttf), which causes the allocation to be only 1-byte long, while more bytes are written to the buffer in lines 1613-1619 (and later on in the function), resulting in the following AddressSanitizer output:

=================================================================
==10832== ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf5c006d1 at pc 0xf5f97664 bp 0xfffa9cd8 sp 0xfffa9ccc
WRITE of size 1 at 0xf5c006d1 thread T0
    #0 0xf5f97663 in Mac_Read_POST_Resource freetype2/src/base/ftobjs.c:1613
    #1 0xf5f9843f in IsMacResource freetype2/src/base/ftobjs.c:1811
    #2 0xf5f98ad9 in IsMacBinary freetype2/src/base/ftobjs.c:1888
    #3 0xf5f990a8 in load_mac_face freetype2/src/base/ftobjs.c:2003
    #4 0xf5f999fe in FT_Open_Face freetype2/src/base/ftobjs.c:2165
    #5 0xf5f964ff in FT_New_Face freetype2/src/base/ftobjs.c:1254
    #6 0x804b5a8 in get_face ft2demos-2.5.3/src/ftbench.c:705
    #7 0x804bc64 in main ft2demos-2.5.3/src/ftbench.c:924
0xf5c006d1 is located 0 bytes to the right of 1-byte region [0xf5c006d0,0xf5c006d1)
allocated by thread T0 here:
    #0 0xf61a3854 (/usr/lib32/libasan.so.0+0x16854)
    #1 0xf5f8adf7 in ft_alloc freetype2/builds/unix/ftsystem.c:102
    #2 0xf5fb2d74 in ft_mem_qalloc freetype2/src/base/ftutil.c:76
    #3 0xf5fb2bde in ft_mem_alloc freetype2/src/base/ftutil.c:55
    #4 0xf5f975de in Mac_Read_POST_Resource freetype2/src/base/ftobjs.c:1609
    #5 0xf5f9843f in IsMacResource freetype2/src/base/ftobjs.c:1811
    #6 0xf5f98ad9 in IsMacBinary freetype2/src/base/ftobjs.c:1888
    #7 0xf5f990a8 in load_mac_face freetype2/src/base/ftobjs.c:2003
    #8 0xf5f999fe in FT_Open_Face freetype2/src/base/ftobjs.c:2165
    #9 0xf5f964ff in FT_New_Face freetype2/src/base/ftobjs.c:1254
    #10 0x804b5a8 in get_face ft2demos-2.5.3/src/ftbench.c:705
    #11 0x804bc64 in main ft2demos-2.5.3/src/ftbench.c:924
SUMMARY: AddressSanitizer: heap-buffer-overflow freetype2/src/base/ftobjs.c:1613 Mac_Read_POST_Resource
Shadow bytes around the buggy address:
  0x3eb80080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb80090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb800a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb800b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb800c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3eb800d0: fa fa fa fa fa fa fa fa fa fa[01]fa fa fa fd fd
  0x3eb800e0: fa fa 00 04 fa fa 00 04 fa fa 00 04 fa fa 00 04
  0x3eb800f0: fa fa 00 04 fa fa 00 04 fa fa 00 04 fa fa 00 00
  0x3eb80100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb80110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eb80120: 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
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==10832== ABORTING

Mateusz Jurczyk <j00ru>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #32411:  mps20141111e.diff added by mpsuzuki (5KiB - text/x-patch)
file #32396:  mps20141110b.diff added by mpsuzuki (904B - text/x-patch)
file #32373:  1.ttf added by j00ru (237KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mpsuzuki (Updated the item)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by j00ru (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-08-22 wl PrivacyPrivate Public
    2014-12-01 mpsuzuki Open/ClosedOpen Closed
    2014-11-26 mpsuzuki Open/ClosedClosed Open
    2014-11-26 mpsuzuki Open/ClosedOpen Closed
        Planned ReleaseNone 2.5.4
    2014-11-26 mpsuzuki Open/ClosedClosed Open
    2014-11-26 mpsuzuki StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2014-11-11 mpsuzuki Attached File- Added mps20141111e.diff, #32411
    2014-11-10 mpsuzuki Attached File- Added mps20141110b.diff, #32396
        StatusNone Ready For Test
    2014-11-05 wl Assigned toNone mpsuzuki
    2014-11-05 j00ru Attached File- Added 1.ttf, #32373

    Back to the top


    Powered by Savane 3.9