GNOME Bugzilla – Bug 785973
Integer overflow leads to memory corruption
Last modified: 2018-01-11 23:08:49 UTC
When loading a large gif image, integer overflow may happen in function gif_get_lzw under source file gdk-pixbuf/io-gif.c. Line 998 (git master branch): temp = dest + context->draw_ypos * gdk_pixbuf_get_rowstride (context->frame->pixbuf) + context->draw_xpos * 4; (https://github.com/GNOME/gdk-pixbuf/blob/1d3cd14f0afabfdeed71706077ad08ed3ecede50/gdk-pixbuf/io-gif.c#L998) context->draw_ypos is an integer and gdk_pixbuf_get_rowstride returns an integer. When the two values are large, multiplication of them may overflow the largest positive value that can be held by an integer, and thus result in a negative value. Adding a negative offset to dest leads to a buffer underflow. As dest is allocated on heap, this will corrupt data on heap. We triggered this bug on gdk-pixbuf 2.0 on a x86_64 machine running Linux 4.4.0 (with the application eog). After checking the git master branch, we observe this bug remains in the updated version. Similar problems: gdk-pixbuf/io-gif.c: Line 729: pixel = dest + (context->draw_ypos + offset) * gdk_pixbuf_get_rowstride (context->frame->pixbuf) + context->draw_xpos * 4; gdk-pixbuf/io-gif.c: Line 735: pixel = dest + (context->draw_ypos + offset) * gdk_pixbuf_get_rowstride (context->frame->pixbuf) + context->draw_xpos * 3;
Do you have a file to trigger this problem?
(In reply to Bastien Nocera from comment #1) > Do you have a file to trigger this problem? The file we used is kind of large. You can get it via: https://www.dropbox.com/s/17a5nzq07xldl8d/poc.gif?dl=0 Opening this file via eog, you should be able to observe a fault. We ever succeeded with a much smaller file, but we do not have that now. -Jun
(In reply to Bastien Nocera from comment #1) > Do you have a file to trigger this problem? Have you tried the sample yet? We found Chrome in Linux also use gdk-pixbuf and the sample can crash chrome as well. It would be great if you can confirm sooner.
I was travelling, so not in a position to download nearly 5 gigs for a test file ;) It's downloading now, thanks.
(In reply to Jun from comment #3) > (In reply to Bastien Nocera from comment #1) > > Do you have a file to trigger this problem? > > Have you tried the sample yet? We found Chrome in Linux also use gdk-pixbuf > and the sample can crash chrome as well. I really doubt Chrome uses gdk-pixbuf's GIF loader, you might want to file a separate bug about that with Google.
(In reply to Jun from comment #0) > When loading a large gif image, integer overflow may happen in function > gif_get_lzw under source file gdk-pixbuf/io-gif.c. > > > Line 998 (git master branch): > temp = dest + context->draw_ypos * gdk_pixbuf_get_rowstride > (context->frame->pixbuf) + context->draw_xpos * 4; > (https://github.com/GNOME/gdk-pixbuf/blob/ > 1d3cd14f0afabfdeed71706077ad08ed3ecede50/gdk-pixbuf/io-gif.c#L998) > > > context->draw_ypos is an integer and gdk_pixbuf_get_rowstride returns an > integer. When the two values are large, multiplication of them may overflow > the largest positive value that can be held by an integer, and thus result > in a negative value. Adding a negative offset to dest leads to a buffer > underflow. As dest is allocated on heap, this will corrupt data on heap. > > We triggered this bug on gdk-pixbuf 2.0 on a x86_64 machine running Linux > 4.4.0 (with the application eog). After checking the git master branch, we > observe this bug remains in the updated version. > > Similar problems: > > gdk-pixbuf/io-gif.c: Line 729: > pixel = dest + (context->draw_ypos + offset) * gdk_pixbuf_get_rowstride > (context->frame->pixbuf) + context->draw_xpos * 4; > > gdk-pixbuf/io-gif.c: Line 735: > pixel = dest + (context->draw_ypos + offset) * gdk_pixbuf_get_rowstride > (context->frame->pixbuf) + context->draw_xpos * 3; Looking at those 3 pieces of code, it's only a problem if context->draw_ypos is so big that the draw_ypos * rowstride overflows an integer. But draw_ypos is 0, 1, 2, or 4. We already have a check for dimensions in gdk_pixbuf_new(), which will fail after attempting to allocate memory. We can implement a similar shortcut to bug 765094, to check for dimension values before attempting to allocate it. pixbuf-read now fails very quickly with "Not enough memory". It just used to fail slower for me before this change. Can you please check whether this fixes the problem for you, with the current git master?
Created attachment 358248 [details] [review] io-gif: Fail quickly when image dimensions are too big Fail quickly when the dimensions would create an image that's bigger than MAXINT bytes long. See https://bugzilla.gnome.org/show_bug.cgi?id=765094
Created attachment 358249 [details] [review] tests: Add truncated GIF file with huge dimensions
(In reply to Bastien Nocera from comment #8) > Created attachment 358249 [details] [review] [review] > tests: Add truncated GIF file with huge dimensions Can you specify the check code? My OS is too old to compile the git master.
(In reply to Jun from comment #9) > (In reply to Bastien Nocera from comment #8) > > Created attachment 358249 [details] [review] [review] [review] > > tests: Add truncated GIF file with huge dimensions > > Can you specify the check code? My OS is too old to compile the git master. Sorry, I see it now.
Does that fix it for you?
Attachment 358248 [details] pushed as 0012e06 - io-gif: Fail quickly when image dimensions are too big Attachment 358249 [details] pushed as 140595d - tests: Add truncated GIF file with huge dimensions
I did build using this commit fix for version 2.32.2 and it fails for two tests: ERROR: animation - too few tests run (expected 2, got 0) ERROR: animation - exited with status 134 (terminated by signal 6?) ERROR:animation.c:11:test_animation: assertion failed (error == NULL): Couldn't recognize the image file format for file '/<<PKGBUILDDIR>>/tests/test-animation.gif' (gdk-pixbuf-error-quark, 3) Aborted (core dumped) # random seed: R02S2c011b430e591e7e33c3fef23a7041f1 1..2 # Start of animation tests # ERROR:animation.c:11:test_animation: assertion failed (error == NULL): Couldn't recognize the image file format for file '/<<PKGBUILDDIR>>/tests/test-animation.gif' (gdk-pixbuf-error-quark, 3) ERROR: animation - too few tests run (expected 2, got 0) ERROR: animation - exited with status 134 (terminated by signal 6?) ERROR: pixbuf-short-gif-write ============================= ** ERROR:pixbuf-short-gif-write.c:43:test_short_gif_write: assertion failed: (loader != NULL) Aborted (core dumped) # random seed: R02Sf65f09c34959ec41b231ff2200bb30fc 1..1 # Start of animation tests # ERROR:pixbuf-short-gif-write.c:43:test_short_gif_write: assertion failed: (loader != NULL) ERROR: pixbuf-short-gif-write - too few tests run (expected 1, got 0) ERROR: pixbuf-short-gif-write - exited with status 134 (terminated by signal 6?) I did a comparison between version 2.36 and 2.32 gdk-pixbuf/io-gif.c code is not so different after this patch. Do you have any clue why it's failing?
(In reply to Leônidas S. Barbosa from comment #13) > I did build using this commit fix for version 2.32.2 and it fails for two > tests: <snip> > ERROR:animation.c:11:test_animation: assertion failed (error == NULL): > Couldn't recognize the image file format for file > '/<<PKGBUILDDIR>>/tests/test-animation.gif' (gdk-pixbuf-error-quark, 3) <snip> > ERROR:pixbuf-short-gif-write.c:43:test_short_gif_write: assertion failed: > (loader != NULL) <snip> Those are the 2 errors. > I did a comparison between version 2.36 and 2.32 gdk-pixbuf/io-gif.c code is > not so different after this patch. I'm guessing you blindly copied the patches, and ignored compile-time warnings. Your GIF loader doesn't load properly, probably because it relies on internal API that's only available in newer gdk-pixbuf versions.
I build the version without the patch that fix this CVE and it builds ok including the test passing. Don't think it is about old API. I`m building for Ubuntu-xenial. The only patch I applied to get the ERRORs was 0012e066ba37439d402ce46afbc1311530a4ec61. Regarding the warnings none different than those I got building with the version without the patch.
Sorry for the noise I think I found the issue, it passed by my blind eyes: +io-gif.c:856:37: warning: implicit declaration of function ‘gdk_pixbuf_calculate_rowstride’ [-Wimplicit-function-declaration] + rowstride = gdk_pixbuf_calculate_rowstride (GDK_COLORSPACE_RGB, So it fails in else and returns Null, probably is that the issue.
No, it fails because it can't load the GIF plugin because there's an unresolved function, because the patch uses functions that aren't in version 2.32 of gdk-pixbuf. Fix this compile-time warning, and the tests should work as expected.