bugThe FreeType Project - Bugs: bug #46149, signed integer overflow in 29...

 
 

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

bug #46149: signed integer overflow in 29 places in 32-bit mode

Submitter:  Kostya Serebryany <kcc>
Submitted:  Thu 08 Oct 2015 01:30:47 AM UTC
   
 
Severity:  3 - Normal Item Group:  None
Status:  Fixed Privacy:  Public
Assigned to:  wl Open/Closed:  Closed
Planned Release:  2.8.1

Jump to the original submission

Thu 01 Jun 2017 06:19:12 PM UTC, comment #23: 
Werner LEMBERG <wl>
Group administrator
Thu 01 Jun 2017 06:01:37 PM UTC, comment #22: 

Do you mind if we move the conversation to https://github.com/google/oss-fuzz/issues ?
This way more oss-fuzz developers and users will see it.
It's an interesting case.

Please just open a new issue

Kostya Serebryany <kcc>
Group Member
Thu 01 Jun 2017 05:59:46 PM UTC, comment #21: 

Sounds interesting!  Since FreeType's docker file doesn't contain any apt-get command for libraries, I assume that some (all?) are available by default, i.e., libpng, libharfbuzz, etc.  However, I guess that 32bit libraries are not installed.

What GNU/Linux distribution is used for building FreeType?  Note that I don't have any experience with apt-get (and no experience with Docker, and no experience with Jenkins – there's definitely something to learn :-)

BTW, on my openSuSE box, after installing the necessary 32bit libraries, I'm simply adding the `-m32' option to CC.

Werner LEMBERG <wl>
Group administrator
Thu 01 Jun 2017 05:32:17 PM UTC, comment #20: 

When I fuzzed 32-bit FreeType I did it manually.
OSS-Fuzz is a self service model.
Whatever you build there we will fuzz.
The VMs are 64-bit, but you can run 32-bit binaries on them
(admittedly, no one has used 32-bit builds so far, so something may need to be fixed on our side).

If you'd like to add 32-bit FreeType build to oss-fuzz all it takes is to add more build commands to https://github.com/google/oss-fuzz/blob/master/projects/freetype2/build.sh and produce e.g. $OUT/ftfuzzer32 (and a copy of the corpus)

Kostya Serebryany <kcc>
Group Member
Thu 01 Jun 2017 05:13:36 PM UTC, comment #19: 

Thanks!  It's a pity that you (no longer?) do 32bit fuzzying – I guess this shows problems much earlier...

Werner LEMBERG <wl>
Group administrator
Thu 01 Jun 2017 04:31:03 PM UTC, comment #18: 

Done:
https://github.com/google/oss-fuzz/commit/5401205191e850ce8c3b3a2db2c4b24287382718

Note that oss-fuzz only builds the 64-bit binaries,
so we won't see 32-bit-specific bugs.

Kostya Serebryany <kcc>
Group Member
Thu 01 Jun 2017 04:28:42 PM UTC, comment #17: 

All those overflows should now be fixed in git.  Please activate the sanitizer again!

Werner LEMBERG <wl>
Group administrator
Tue 20 Oct 2015 10:57:26 PM UTC, comment #16: 

The libFuzzer bot is not using -fsanitize=unsigned-integer-overflow any more.
Ping this bug when you'd like to re-enable it.

Kostya Serebryany <kcc>
Group Member
Mon 19 Oct 2015 05:04:20 AM UTC, comment #15: 

Yes, please switch it off.  It will take some time until I have time to implement the ideas from comment #8.

Werner LEMBERG <wl>
Group administrator
Fri 16 Oct 2015 08:33:05 PM UTC, comment #14: 

the bot is finding more integer overflows in 64-bot too.
Examples:

src/base/ftcalc.c:182:5: runtime error: negation of -9223372036854775808 cannot be represented in type 'FT_Long' (aka 'long'); cast to an unsigned type to negate this value to itself
src/base/ftobjs.c:585:16: runtime error: signed integer overflow: -4611686018427387840 + -9223372036854775808 cannot be represented in type 'long'
src/truetype/ttgload.c:1978:39: runtime error: signed integer overflow: 4611686018427387968 - -4611686018427387840 cannot be represented in type 'long'
src/truetype/ttinterp.c:1710:26: runtime error: signed integer overflow: -9223372036854775808 + -15 cannot be represented in type 'long'


I can send the reproducers if needed.
Shall I disable the integer-overflow-detection for now?
It just finds too many things and slows down the bot :(

Kostya Serebryany <kcc>
Group Member
Thu 15 Oct 2015 06:10:41 AM UTC, comment #13: 

Can you provide patches and send them to the list?

Werner LEMBERG <wl>
Group administrator
Tue 13 Oct 2015 03:56:12 PM UTC, comment #12: 

The pcf overflows can also be fixed. Even though the PCF format specifies fottAscent and fontDescent as int32, the ultimate bitmap height is FT_Short. We might be able to truncate the input and, perhaps, change bitmap height to FT_UShort.

Alexei Podtelezhnikov <podtelez>
Group Member
Mon 12 Oct 2015 03:12:50 PM UTC, comment #11: 

The t1decode overflows can be fixed. t1_builder_add_point ultimately does this:

      point->x = FIXED_TO_INT( x );
      point->y = FIXED_TO_INT( y );

effectively limiting the coordinate space to 16 bits. Should we move this up in t1_decode_parse_charstrings, we could avoid overflows. No? Another macro used for this purpose is Fix2Int.

Alexei Podtelezhnikov <podtelez>
Group Member
Thu 08 Oct 2015 09:55:13 PM UTC, comment #10: 

You can declare it as inline, yes. I think there will be no penalty. This function will likely be inlined by the compiler, and the arithmetic ops in it will not be checked by UBSan.

Alexey Samsonov <vonosmas>
Thu 08 Oct 2015 09:26:36 PM UTC, comment #9: 

OK, thanks.  Is there a penalty for using such a function (which should probably defined as inline)?

Werner LEMBERG <wl>
Group administrator
Thu 08 Oct 2015 08:06:33 PM UTC, comment #8: 

As #7 suggests, you can move the arithmetic operations that can lead to signed-integer-overflow (but this overflow is believe to be "harmless") to a separate function and annotate it with

_attribute_((no_sanitize("signed-integer-overflow")))

In this way you'll at least make the assumption visible in the source code, and it's better than maintaining a list of suppressions externally.

Alexey Samsonov <vonosmas>
Thu 08 Oct 2015 05:58:06 PM UTC, comment #7: 


>> You might get a crazy looking image as a result of the overflow but nothing with security implications. No?


Strictly speaking -- no.
signed integer overflow is undefined behavior.
According to the standard, signed integer overflow is allowed
to make your cat pregnant or cause much more worrisome troubles.

Look e.e. here:
http://stackoverflow.com/questions/7682477/why-does-integer-overflow-on-x86-with-gcc-cause-an-infinite-loop

Now, if you implement "int add(int a, int b) {return a + b; }"
and make ABSOLUTELY sure that it never gets inlined or somehow else used by the optimizer you may be somewhat safe.
But fooling compilers is a non-trivial business -- they are clever.

Kostya Serebryany <kcc>
Group Member
Thu 08 Oct 2015 05:28:58 PM UTC, comment #6: 

Some of the overflows you've found are in linear algebra concerned with different transformations of glyph contour coordinates. You might get a crazy looking image as a result of the overflow but nothing with security implications. No?

Alexei Podtelezhnikov <podtelez>
Group Member
Thu 08 Oct 2015 05:19:22 PM UTC, comment #5: 

Please do so!  Maybe there are not many libraries that need such a thing, so I'm really interested to hear more opinions :-)

Werner LEMBERG <wl>
Group administrator
Thu 08 Oct 2015 05:08:55 PM UTC, comment #4: 

A reasonable way to tag an overflow as "don't worry" is to
change the type from signed to unsigned.
Unlike with signed, unsigned overflow is strictly defined
by C/C++ standards and so by default ubsan (either gcc's or clang's) will not complain.

Another story, is that sometimes unsigned overflows are also bugs. (We've seen some quite scary ones).
Clang has -fsanitize=unsigned-integer-overflow, which is
not enabled by default under -fsanitize=undefined
because unsigned-integer-overflow is defined.

I've run my test corpus with -fsanitize=unsigned-integer-overflow
and it reported several dozens of unsigned overflows,
most of which are not bugs, but I can't be 100% sure about all of them.

Another way is to wrap the potentially overflowing instructions
into a function and somehow make sure it does not get instrumented. There is no trivial way to do that (other than
to put the function into a separate file and force it to be compiled w/o -fsanitize=*).

Let me invite the current owner of ubsan to this discussion...

Kostya Serebryany <kcc>
Group Member
Thu 08 Oct 2015 02:38:23 PM UTC, comment #3: 

[CCing Behdad also]

Some input parameters can be limited, e.g., the number of glyphs, which can't exceed the font file or stream size, for example.

However, for values that can't be set into relation to the file size, I feel uncomfortable to artificially limit them just to ensure that there is no overflow.  Additionally, it's always possible even then to create an outline with overflowing coordinates since the added values are relative in all vector formats...

I want to have a means to tag overflows as `don't worry', which should then silence both the fuzzer and the sanitizers of gcc and llvm.  Ideas?

Werner LEMBERG <wl>
Group administrator
Thu 08 Oct 2015 01:49:25 PM UTC, comment #2: 

I agree that is paranoid. I wonder if overflows are triggered by crazy large char_size and char_width in FT_Set_Char_Size or by crazy units_per_EM.

Alexei Podtelezhnikov <podtelez>
Group Member
Thu 08 Oct 2015 05:06:30 AM UTC, comment #1: 

Thanks a lot!

Alexei, Dave, please advise how to solve those issues gracefully.  Most overflow problems are `harmless', this is, if we ever encounter values that large, the rastered output will be meaningless.  On the other hand, checking the validity of the input data is often not trivially done, and I fear that thorough checks make FreeType slower.

A nice reading is http://www.fefe.de/intof.html and http://www.gnu.org/software/autoconf/manual/autoconf.html#Integer-Overflow, BTW.

Werner LEMBERG <wl>
Group administrator
Thu 08 Oct 2015 01:30:47 AM UTC, original submission:  

found with libFuzzer+ubsan in 32-bit mode on fresh git.

Feed the attached inputs into
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/diff/src/tools/ftfuzzer/ftfuzzer.cc

ubsan will report 29 instances of signed integer overflow
(not counting those from ##46107 in cff) 
although the number of root causes is likely much less.
     1 src/base/ftcalc.c:725:41:
     2 src/base/ftcalc.c:726:41:
     3 src/base/ftcalc.c:727:41:
     4 src/base/ftcalc.c:728:41:
     5 src/base/ftcalc.c:750:50:
     6 src/base/ftcalc.c:753:50:
     7 src/base/ftcalc.c:89:16:
     8 src/base/ftcalc.c:920:26:
     9 src/base/ftobjs.c:2638:39:
    10 src/base/ftoutln.c:1082:14:
    11 src/pcf/pcfdrivr.c:595:62:
    12 src/pcf/pcfdrivr.c:596:65:
    13 src/pcf/pcfread.c:1285:25:
    14 src/psaux/psconv.c:303:16:
    15 src/psaux/t1decode.c:1231:13:
    16 src/psaux/t1decode.c:1236:13:
    17 src/psaux/t1decode.c:1247:13:
    18 src/psaux/t1decode.c:1278:13:
    19 src/psaux/t1decode.c:1279:13:
    20 src/psaux/t1decode.c:1282:13:
    21 src/psaux/t1decode.c:1283:13:
    22 src/psaux/t1decode.c:1286:13:
    23 src/psaux/t1decode.c:1287:13:
    24 src/psaux/t1decode.c:1303:13:
    25 src/psaux/t1decode.c:860:21:
    26 src/sfnt/ttsbit.c:312:55:
    27 src/sfnt/ttsbit.c:313:56:
    28 src/sfnt/ttsbit.c:316:59:
    29 src/sfnt/ttsbit.c:317:64:


Kostya Serebryany <kcc>
Group Member

 

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

Attached Files
file #35096:  OVERFLOW-32bit.tgz added by kcc (23KiB - application/x-compressed-tar)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by vonosmas (Posted a comment)
  • -email is unavailable- added by kcc (adding ubsan guru)
  • -email is unavailable- added by wl
  • -email is unavailable- added by podtelez (Posted a comment)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by wl
  • -email is unavailable- added by wl
  • -email is unavailable- added by kcc (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.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-01 wl StatusPostponed Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 2.8.1
    2015-10-21 wl StatusNone Postponed
        Assigned toNone wl
    2015-10-08 kcc Carbon-Copy- Added -email is unavailable-
    2015-10-08 wl Carbon-Copy- Added behdad
    2015-10-08 wl Carbon-Copy- Added apodtele
        Carbon-Copy- Added darnold
    2015-10-08 kcc Attached File- Added OVERFLOW-32bit.tgz, #35096

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code