Bug 15528 - jpeg decoder allocation size overflows
Summary: jpeg decoder allocation size overflows
Status: RESOLVED FIXED
Alias: None
Product: swfdec
Classification: Unclassified
Component: library (show other bugs)
Version: git
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: swfdec ml
QA Contact: swfdec ml
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-16 01:20 UTC by M Joonas Pihlaja
Modified: 2008-09-15 09:10 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Test jpegs (20.00 KB, application/octet-stream)
2008-04-16 01:20 UTC, M Joonas Pihlaja
Details
don't segfault with wookiemon.jpeg (521 bytes, patch)
2008-09-06 12:09 UTC, Riccardo Magliocchetti
Details | Splinter Review
tentative of a test (164 bytes, text/plain)
2008-09-06 12:11 UTC, Riccardo Magliocchetti
Details
validate jpeg image size in the right place (539 bytes, patch)
2008-09-07 02:17 UTC, Riccardo Magliocchetti
Details | Splinter Review
test for cookiemon (164 bytes, text/plain)
2008-09-07 02:19 UTC, Riccardo Magliocchetti
Details
avoid size validation (23 bytes, image/jpeg)
2008-09-15 08:36 UTC, M Joonas Pihlaja
Details
bypass check to hit another malloc (23 bytes, image/jpeg)
2008-09-15 08:41 UTC, M Joonas Pihlaja
Details

Note You need to log in before you can comment on or make changes to this bug.
Description M Joonas Pihlaja 2008-04-16 01:20:42 UTC
Created attachment 15947 [details]
Test jpegs

The two files cookiemon.jpg and wookiemon.jpg in the attached tar file trigger allocation overflows on x86 and amd64.  Valgrind says:

[for wookiemon.jpg]
==4516== Invalid write of size 1
==4516==    at 0x445D8F8: (within /usr/lib/liboil-0.3.so.0.1.0)
==4516==    by 0x80497FF: jpeg_decoder_decode_entropy_segment (jpeg.c:503)
==4516==    by 0x8049DEB: jpeg_decoder_decode (jpeg.c:683)
==4516==    by 0x804B1E1: jpeg_decode_argb (jpeg_rgb_decoder.c:58)
==4516==    by 0x8048A51: main (load.c:46)
==4516==  Address 0x632C490 is 0 bytes after a block of size 0 alloc'd
==4516==    at 0x442438B: malloc (vg_replace_malloc.c:149)
==4516==    by 0x8049084: jpeg_decoder_init_decoder (jpeg.c:192)
==4516==    by 0x8049CD3: jpeg_decoder_decode (jpeg.c:654)
==4516==    by 0x804B1E1: jpeg_decode_argb (jpeg_rgb_decoder.c:58)
==4516==    by 0x8048A51: main (load.c:46)

[for cookiemon.jpg]
==4520== Invalid write of size 4
==4520==    at 0x804B470: yuv_mux (jpeg_rgb_decoder.c:103)
==4520==    by 0x804BDDF: get_argb_420 (jpeg_rgb_decoder.c:278)
==4520==    by 0x804B329: jpeg_decoder_get_argb_image (jpeg_rgb_decoder.c:89)
==4520==    by 0x804B217: jpeg_decode_argb (jpeg_rgb_decoder.c:63)
==4520==    by 0x8048A51: main (load.c:46)
==4520==  Address 0x78C57D80 is 0 bytes after a block of size 40 alloc'd
==4520==    at 0x442438B: malloc (vg_replace_malloc.c:149)
==4520==    by 0x804BB54: get_argb_420 (jpeg_rgb_decoder.c:253)
==4520==    by 0x804B329: jpeg_decoder_get_argb_image (jpeg_rgb_decoder.c:89)
==4520==    by 0x804B217: jpeg_decode_argb (jpeg_rgb_decoder.c:63)
==4520==    by 0x8048A51: main (load.c:46)
Comment 1 Riccardo Magliocchetti 2008-09-06 12:09:51 UTC
Created attachment 18705 [details] [review]
don't segfault with wookiemon.jpeg

I think that when handling input using g_try_malloc is way better than segfaulting.
Comment 2 Riccardo Magliocchetti 2008-09-06 12:11:14 UTC
Created attachment 18706 [details]
tentative of a test

This is the test i'm trying against.
Comment 3 Riccardo Magliocchetti 2008-09-07 02:17:05 UTC
Created attachment 18720 [details] [review]
validate jpeg image size in the right place

This one fix both image loading for me.
Comment 4 Riccardo Magliocchetti 2008-09-07 02:19:03 UTC
Created attachment 18721 [details]
test for cookiemon
Comment 5 Benjamin Otte 2008-09-07 09:26:13 UTC
patch applied in git.
Comment 6 M Joonas Pihlaja 2008-09-15 08:36:38 UTC
Created attachment 18881 [details]
avoid size validation

Hi,

I had some fun this weekend while looking at dsjpeg more closely.  The proposed patch doesn't work 100% correctly, but does make crashing the decoder more difficult.  Checking for multiplication overflow when computing a*b should either take the form of a test like (a*b)/b = a or explicitly checking the sizes of a and b to avoid overflow.  The attached test case will crash due to a NULL pointer dereference (on 32 bit machines), or eventually due to an out of bounds write (on 64 bit machines.)

On a 64 bit machine, the result in gdb is reproduced below. (Valgrind takes way too long as the case needs to trawl through a lot of memory before hitting the segfaulting overwrite.)

Program received signal SIGSEGV, Segmentation fault.
0x00002b2c733b6000 in oil_test_new () from /usr/lib/liboil-0.3.so.0
(gdb) up
#1  0x0000000000404962 in get_argb_420 (dec=0x507030) at jpeg_rgb_decoder.c:279
279         oil_colorspace_argb(argbp, tmp, jfif_matrix, dec->width);
(gdb) p argb
No symbol "argb" in current context.
(gdb) p argbp
$1 = (uint32_t *) 0x2b2d9e48a010
(gdb) p tmp
$2 = (uint32_t *) 0x2b2cf39bf010
Comment 7 M Joonas Pihlaja 2008-09-15 08:41:03 UTC
Created attachment 18883 [details]
bypass check to hit another malloc

This test case causes a malloc argument overflow later in the code causing malloc to return NULL and a subsequent NULL ptr deref on both x86 and x86_64.
Comment 8 Riccardo Magliocchetti 2008-09-15 09:10:16 UTC
Benjamin committed a different fix that is indeed more correct than mine. I've tested both your new attachments with latest git and both are handled correctly. If you have other images please try them with 0.8.0 or git.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.