bugThe FreeType Project - Bugs: bug #43597, FreeType 2.5.3 sbix PNG handling...

 
 

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

bug #43597: FreeType 2.5.3 sbix PNG handling heap-based buffer overflow due to integer overflow

Submitter:  Mateusz Jurczyk <j00ru>
Submitted:  Thu 13 Nov 2014 09:56:56 AM UTC
   
 
Severity:  3 - Normal Item Group:  Crash
Status:  Fixed Privacy:  Public
Assigned to:  wl Open/Closed:  Closed
Planned Release:  2.5.4

Jump to the original submission

Tue 02 Dec 2014 01:56:31 PM UTC, comment #9: 

This is not an issue. C permits signed-unsigned comparison, period.

Alexei Podtelezhnikov <podtelez>
Group Member
Tue 02 Dec 2014 09:38:50 AM UTC, comment #8: 

Uh, oh, I consider `-Werror' as evil if used for production, since it is too much dependent on the compiler – and it has zero value for the end user.

My opinion is to live with that incompatibility issue, because the improvement outweighs this nuisance.  What do others think?

Werner LEMBERG <wl>
Group administrator
Sat 29 Nov 2014 09:55:17 PM UTC, comment #7: 

Unfortunately b3500af717010137046ec4076d1e1c0641e33727 is source incompatible for anything using -Werror.

../gui/text/qfontengine_ft.cpp: In member function ‘QFontEngineFT::Glyph* QFontEngineFT::loadGlyph(QFontEngineFT::QGlyphSet*, uint, QFixed, QFontEngine::GlyphFormat, bool) const’:
../gui/text/qfontengine_ft.cpp:1126:39: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
                        for (int x = 0; x < slot->bitmap.width; x++) {

Sergio Martins <iamsergio>
Fri 21 Nov 2014 04:04:39 PM UTC, comment #6: 

Yes, this is the corresponding code.  Thanks for checking.

Werner LEMBERG <wl>
Group administrator
Fri 21 Nov 2014 03:00:13 PM UTC, comment #5: 
Mateusz Jurczyk <j00ru>
Group Member
Mon 17 Nov 2014 05:51:17 PM UTC, comment #4: 

With signed 32-bit width/height, things really get complicated, because it's a mismatch between the unsigned type used in the IHDR PNG structure.

For example if width = 4294967295 and height = 2, then performing the "if( (width height) / width != height ) error ...;" statement will pass just fine, because it will check that (-1 2) / -1 == 2, which is true for signed numbers. It will then allocate 4294967294 bytes and try to load 8589934590 bytes into the buffer, causing an overflow. Similarly, the "width + height > 65535" check can also be bypassed if width/height are negative. If you want to keep the signedness of those fields, it is necessary to test if they are not negative.

Also, please keep in mind that in the code we're discussing, the size of the destination buffer is in fact (width height 4), so it's important that the whole expression doesn't overflow. Incidentally 0x7fff 0x7fff 4 = 0xfffc0004, so we're safe here. However, it's worth keeping in mind if you decided to verify the dimensions in a different way.

Mateusz Jurczyk <j00ru>
Group Member
Mon 17 Nov 2014 05:31:32 PM UTC, comment #3: 

Of course, the 32767-by-32767 bitmap limit, or 1 gigapixel, is plenty and should by sufficient for years to come. For the record though, we could use some alternatives:

1) Slower but thorough post-hoc overflow check:
if( (width * height) / width != height ) error...;

2) Just as quick but more flexible for rectangular bitmaps:
if( width + height > 65535 ) error...;


Alexei Podtelezhnikov <podtelez>
Group Member
Mon 17 Nov 2014 05:13:57 PM UTC, comment #2: 

Dear Werner,

Thanks for the fix; however, I'm afraid it's not sufficient to cover all possible scenarios. Looking at "freetype/include/ftimage.h", it seems that the "rows" and "width" fields (in addition to others) of "FT_Bitmap" are declared as signed 32-bit integers:

319:  typedef struct  FT_Bitmap_
320:  {
321:    int             rows;
322:    int             width;

I'm not very familiar with the code base, but in my opinion the signedness is an error in itself, as width/height should never go negative in legitimate cases, but it can cause security related problems. Anyway, considering that the fields are of signed type, the following submitted check:

+      if ( map->rows > 0x7FFF || map->width > 0x7FFF )

will not account for negative values, while it is indeed possible to create a 2^31 pixel wide PNG (which overflows the integer range and becomes -2147483648). If the field types are converted to unsigned or the sanity check considers negative values as well, it will then be sufficient.

Thanks,
Mateusz

Mateusz Jurczyk <j00ru>
Group Member
Sat 15 Nov 2014 08:06:40 AM UTC, comment #1: 

Thanks for the report.  Fixed in git, please test.

Werner LEMBERG <wl>
Group administrator
Thu 13 Nov 2014 09:56:56 AM UTC, original submission:  

FreeType 2.5.3 supports embedded bitmaps in SFNT-based fonts, in one of the three standard graphics formats (PNG), while JPEG and TIFF are not yet supported. Such bitmaps can be found in the Apple-defined "sbix" tables (see https://developer.apple.com/fonts/TrueType-Reference-Manual/RM06/Chap6sbix.html), which in the PNG case is internally handled in FreeType by the "Load_SBit_Png" function implemented in freetype/src/sfnt/pngshim.c.

In addition to bitmap data itself, additional glyph information such as width, height, bit depth etc. is obtained from corresponding PNG headers (specifically, IHDR) and used to initialize internal FT structures:

194:    png_uint_32  imgWidth, imgHeight;
195:
196:    int         bitdepth, color_type, interlace;
...
247:    png_get_IHDR( png, info,
248:                  &imgWidth, &imgHeight,
249:                  &bitdepth, &color_type, &interlace,
250:                  NULL, NULL );
...
258:    if ( populate_map_and_metrics )
259:    {
260:      FT_Long  size;
261:
262:
263:      metrics->width  = (FT_Int)imgWidth;
264:      metrics->height = (FT_Int)imgHeight;
265:
266:      map->width      = metrics->width;
267:      map->rows       = metrics->height;
268:      map->pixel_mode = FT_PIXEL_MODE_BGRA;
269:      map->pitch      = map->width * 4;
270:      map->num_grays  = 256;

More importantly, these sizes are further used with no bounds checks, in order to calculate the overall buffer size for the PNG data:

272:      size = map->rows * map->pitch;
273:
274:      error = ft_glyphslot_alloc_bitmap( slot, size );

The bitmap pixels are subsequently loaded into the buffer:

349:    for ( i = 0; i < (FT_Int)imgHeight; i++ )
350:      rows[i] = map->buffer + ( y_offset + i ) map->pitch + x_offset 4;
351:
352:    png_read_image( png, rows );

Now, since PNG supports very large images (the width and height fields in IHDR are 32-bit wide) and the "Load_SBit_Png" function blindly trusts the loaded dimensions, it is possible to overflow the "map->rows * map->pitch" expression, thus under-allocating the resulting buffer and later causing a heap-based buffer overflow.

Attached please find a font file (poc.ttf) with a valid embedded PNG file of width=65535 and height=16385, which causes "map->pitch" to become 262140, and "size" to become (uint32)(262140 * 16385) = (uint32)4295163900 = 196604. Since over 4GB of data is attempted to be loaded into a very small buffer, the sample triggers a SIGSEGV signal, or the following AddressSanitizer report.

In order to reproduce, run: $ ftview 150 /path/to/poc.ttf

=================================================================
==21730== ERROR: AddressSanitizer: unknown-crash on address 0xf57cf800 at pc 0xf6121ce0 bp 0xffba2718 sp 0xffba22f8
WRITE of size 262140 at 0xf57cf800 thread T0
    #0 0xf6121cdf (/usr/lib32/libasan.so.0+0xdcdf)
    #1 0xf5b60ea1 (/lib/i386-linux-gnu/libpng12.so.0+0x9ea1)
    #2 0xf5b681f7 (/lib/i386-linux-gnu/libpng12.so.0+0x111f7)
    #3 0xf5b68669 (/lib/i386-linux-gnu/libpng12.so.0+0x11669)
    #4 0xf60460a3 in Load_SBit_Png freetype2/src/sfnt/pngshim.c:352
    #5 0xf604c768 in tt_face_load_sbix_image freetype2/src/sfnt/ttsbit.c:1320
    #6 0xf604cb8e in tt_face_load_sbit_image freetype2/src/sfnt/ttsbit.c:1395
    #7 0xf5f621b6 in load_sbit_image freetype2/src/truetype/ttgload.c:2036
    #8 0xf5f633a6 in TT_Load_Glyph freetype2/src/truetype/ttgload.c:2360
    #9 0xf5f539f9 in tt_glyph_load freetype2/src/truetype/ttdriver.c:404
    #10 0xf5f188f3 in FT_Load_Glyph freetype2/src/base/ftobjs.c:726
    #11 0xf60a315e in ftc_basic_family_load_glyph freetype2/src/cache/ftcbasic.c:173
    #12 0xf60a0937 in FTC_INode_New freetype2/src/cache/ftcimage.c:79
    #13 0xf60a09eb in ftc_inode_new freetype2/src/cache/ftcimage.c:102
    #14 0xf609e4ff in FTC_Cache_NewNode freetype2/src/cache/ftccache.c:461
    #15 0xf60a5115 in FTC_ImageCache_LookupScaler freetype2/src/cache/ftcbasic.c:389
    #16 0x8059ca0 in FTDemo_Index_To_Bitmap ft2demos-2.5.3/src/ftcommon.c:859
    #17 0x8059e42 in FTDemo_Draw_Index ft2demos-2.5.3/src/ftcommon.c:892
    #18 0x804bcce in Render_All ft2demos-2.5.3/src/ftview.c:493
    #19 0x80540c4 in main ft2demos-2.5.3/src/ftview.c:1974
0xf57ff7fc is located 0 bytes to the right of 196604-byte region [0xf57cf800,0xf57ff7fc)
allocated by thread T0 here:
    #0 0xf612a854 (/usr/lib32/libasan.so.0+0x16854)
    #1 0xf5f0f387 in ft_alloc freetype2/builds/unix/ftsystem.c:102
    #2 0xf5f37304 in ft_mem_qalloc freetype2/src/base/ftutil.c:76
    #3 0xf5f3716e in ft_mem_alloc freetype2/src/base/ftutil.c:55
    #4 0xf5f1637e in ft_glyphslot_alloc_bitmap freetype2/src/base/ftobjs.c:332
    #5 0xf6045d27 in Load_SBit_Png freetype2/src/sfnt/pngshim.c:274
    #6 0xf604c768 in tt_face_load_sbix_image freetype2/src/sfnt/ttsbit.c:1320
    #7 0xf604cb8e in tt_face_load_sbit_image freetype2/src/sfnt/ttsbit.c:1395
    #8 0xf5f621b6 in load_sbit_image freetype2/src/truetype/ttgload.c:2036
    #9 0xf5f633a6 in TT_Load_Glyph freetype2/src/truetype/ttgload.c:2360
    #10 0xf5f539f9 in tt_glyph_load freetype2/src/truetype/ttdriver.c:404
    #11 0xf5f188f3 in FT_Load_Glyph freetype2/src/base/ftobjs.c:726
    #12 0xf60a315e in ftc_basic_family_load_glyph freetype2/src/cache/ftcbasic.c:173
    #13 0xf60a0937 in FTC_INode_New freetype2/src/cache/ftcimage.c:79
    #14 0xf60a09eb in ftc_inode_new freetype2/src/cache/ftcimage.c:102
    #15 0xf609e4ff in FTC_Cache_NewNode freetype2/src/cache/ftccache.c:461
    #16 0xf60a5115 in FTC_ImageCache_LookupScaler freetype2/src/cache/ftcbasic.c:389
    #17 0x8059ca0 in FTDemo_Index_To_Bitmap ft2demos-2.5.3/src/ftcommon.c:859
    #18 0x8059e42 in FTDemo_Draw_Index ft2demos-2.5.3/src/ftcommon.c:892
    #19 0x804bcce in Render_All ft2demos-2.5.3/src/ftview.c:493
    #20 0x80540c4 in main ft2demos-2.5.3/src/ftview.c:1974
SUMMARY: AddressSanitizer: unknown-crash ??:0 ??
Shadow bytes around the buggy address:
  0x3eaf9eb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eaf9ec0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eaf9ed0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eaf9ee0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x3eaf9ef0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x3eaf9f00:[00]00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3eaf9f10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3eaf9f20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3eaf9f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3eaf9f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x3eaf9f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
==21730== ABORTING

Mateusz Jurczyk <j00ru>
Group Member

 

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

Attached Files
file #32427:  poc.ttf added by j00ru (1MiB - 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 iamsergio (Posted a comment)
  • -email is unavailable- added by podtelez (Posted a comment)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by j00ru (Submitted the item)
  • -email is unavailable- added by j00ru
  •  

    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.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-11-15 wl StatusNone Fixed
        PrivacyPrivate Public
        Assigned toNone wl
        Open/ClosedOpen Closed
        Planned ReleaseNone 2.5.4
    2014-11-13 j00ru Attached File- Added poc.ttf, #32427
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code