GNOME Bugzilla – Bug 775218
Integer overflows in jpeg_parse_exif_app1
Last modified: 2016-12-14 12:50:30 UTC
security
In jpeg_parse_exif_app1 it reads an offset off a file: /* read out the offset pointer to IFD0 */ offset = de_get32(&marker->data[i] + 4, endian); i = i + offset; this "i" is then used to peek into the buffer and read bytes. tags = de_get16(&marker->data[i], endian); i = i + 2; The addition may very well overflow. ➜ gdk-pixbuf git:(master) ✗> env ASAN_OPTIONS=abort_on_error=1 gdb -ex "set pagination 0" -ex "r" -ex "t a a bt full" --args /tmp/gdkpb/libexec/installed-tests/gdk-pixbuf/pixbuf-read /tmp/gdk-findings/compute32_fuzz19/crashes/id:000022,sig:11,src:003440+003703,op:splice,rep:16 GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1 Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /tmp/gdkpb/libexec/installed-tests/gdk-pixbuf/pixbuf-read...done. Starting program: /tmp/gdkpb/libexec/installed-tests/gdk-pixbuf/pixbuf-read /tmp/gdk-findings/compute32_fuzz19/crashes/id:000022,sig:11,src:003440+003703,op:splice,rep:16 [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". /tmp/gdk-findings/compute32_fuzz19/crashes/id:000022,sig:11,src:003440+003703,op:splice,rep:16 Program received signal SIGSEGV, Segmentation fault. 0x00007ffff0ee3263 in de_get16 (ptr=0x60d10000b33e, endian=4321) at io-jpeg.c:294 294 memcpy(&val, ptr, sizeof(val));
+ Trace 236887
Thread 1 (Thread 0x7ffff7fc3880 (LWP 7536))
$4 = (jpeg_saved_marker_ptr) 0x60d00000b320 (gdb) p *marker $5 = {next = 0x60d00000b250, marker = 225 '\341', original_length = 55, data_length = 55, data = 0x60d00000b340 "Exif66P\221\366MM"} (gdb) p i $6 = 4294967294 (gdb) p i+2 $7 = 0 (gdb) l 449 ret = FALSE; 450 goto out; 451 } 452 453 /* find out how many tags we have in IFD0. As per the TIFF spec, the first 454 two bytes of the IFD contain a count of the number of tags. */ 455 tags = de_get16(&marker->data[i], endian); 456 i = i + 2; 457 458 /* check that we still have enough data for all tags to check. The tags (gdb) This crashes firefox, epiphany, nautilus, probably everything that uses gdk-pixbuf to render the pathologic JPEG file that afl produced.
Created attachment 340887 [details] [review] patch This is one way to check for overflows. The test case passes.
Created attachment 340888 [details] crashing file, password "crash"
I don't do gpg. Can you please attach a patch that adds the test file to the testsuite ?
Review of attachment 340887 [details] [review]: this looks good to me
Created attachment 341821 [details] [review] crashing file added to tests/ this is the attached gpg file after having done gpg --decrypt /tmp/crash.jpg.gpg > bug775218.jpg
Created attachment 341826 [details] [review] jpeg: Check for integer overflows in app1 EXIF tags In jpeg_parse_exif_app1(), we would usually read offsets this way: /* read out the offset pointer to IFD0 */ offset = de_get32(&marker->data[i] + 4, endian); i = i + offset; "i" is then used to peek into the buffer and read bytes. tags = de_get16(&marker->data[i], endian); i = i + 2; But as the addition may overflow, we need to check whether the result of the addition would overflow and wrap-around.
Comment on attachment 340887 [details] [review] patch I fixed coding style problems, and reworded the commit message in a new patch, marking this one as obsolete.
Review of attachment 341826 [details] [review]: It regresses it seems: ERROR:/home/hadess/Projects/jhbuild/gdk-pixbuf/tests/pixbuf-jpeg.c:74:test_type9_rotation_exif_tag: assertion failed (error == NULL): Image size is 640x428 but should be 428x640 (gdk-pixbuf-error-quark, 0)
Created attachment 341833 [details] [review] tests: Add test case for bug 775218
(In reply to Bastien Nocera from comment #9) > Review of attachment 341826 [details] [review] [review]: > > It regresses it seems: > ERROR:/home/hadess/Projects/jhbuild/gdk-pixbuf/tests/pixbuf-jpeg.c:74: > test_type9_rotation_exif_tag: assertion failed (error == NULL): Image size > is 640x428 but should be 428x640 (gdk-pixbuf-error-quark, 0) The original patch regresses as well. At least now we have a test case hooked up, which shows the impact of the patch.
Created attachment 341836 [details] [review] jpeg: Check for integer overflows in app1 EXIF tags In jpeg_parse_exif_app1(), we would usually read offsets this way: /* read out the offset pointer to IFD0 */ offset = de_get32(&marker->data[i] + 4, endian); i = i + offset; "i" is then used to peek into the buffer and read bytes. tags = de_get16(&marker->data[i], endian); i = i + 2; But as the addition may overflow, we need to check whether the result of the addition would overflow and wrap-around.
Created attachment 341837 [details] [review] tests: Add test case for bug 775218
This last patch to io-jpeg.c corrects the checks done for integer overflow. You were doing "i = i+12" when you should just have been checking for whether "i+12" overflowed (or was too big), and weren't doing the checks at the end of each while loop. This passes the test suite for me.
very nice work! Looks good to me.
Attachment 341836 [details] pushed as 5daadc0 - jpeg: Check for integer overflows in app1 EXIF tags Attachment 341837 [details] pushed as 7fc0cc2 - tests: Add test case for bug 775218