Looks good, thanks!

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
}

Committed to GIT.

Ping? :-)

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)?

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

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?

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)

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