GNOME Bugzilla – Bug 50187
Robustness audit on pixbuf loader modules
Last modified: 2010-07-10 04:07:37 UTC
The pixbuf loader modules need to be audited for robustness against hostile or broken image files. This is a potential security issue so can't really be punted, has to be done before stable release. We can possibly punt some of the image loaders and only audit the most important ones if there's a time crunch.
Put all GTK 1.3.x bugs on 2.0.0 milestone
Created attachment 459 [details] my replacement for io-gif.c
for your viewing displeasure I rewrote the gif decoder, this one should be fast, robust, accurate and portable indeed.
Helmethead: Can you briefly mention what you changed between your version of io-gif.c and the one in cvs?
I can't say for sure since I rewrote it from scratch, though it's quite similar to gtk's one. Gtk's io-gif.c code is a bit of a mess, and also I decided to rewrite as a little project for myself. There are no great claims to fame for my version, apart from mostly having clean code. It's better in some small ways (bit faster and despite that, sometimes more accurate) and it should be robust against any dodgyness you can feed it.
Created attachment 569 [details] test-cases and fixes for pixbuf loaders
I have uploaded a file with test cases for some of the GdkPixbuf loaders. They are the result of feeding random data to the loaders. A particularly nasty test was taking a valid image and modifying the bytes in it at random. Only the JPEG loader survived this treatment. The tarball also contains a patch that fixes some, but far from all, of the crashes. The changes also implements 'incremental' loading for TIFF images without saving to a temporary file. It also stops libpng from spewing messages to stderr; however, it also introduces even more memory leaks than there already are. See the comments in test-loaders.c for more details.
You left out the file test-random.c - for now I'm just not putting it in CVS, send it along if it was supposed to be included.
I checked in the test-loaders patch with some tweaks, thanks, very useful. Unfortunately it reveals that we aren't even close to being able to close this bug... Soren, can you tell us how you created test-images.h so we can add images to it in the future?
Everytime a loader crashed, I reran the test with the same seed recording (using the verbose parameters) what bytes actually made the loader crash. Then I added these to test-images.h (by hand). The file test-random.c used to be a separate program to generate random data; I don't use it anymore, all of its functionality is included in test-loaders.c now.
Created attachment 737 [details] [review] Makes .ICO loader more robust, minor fix for JPEG, and test images for wbmp
I have committed that patch now. I'm leaving this bug open as holding pen for further robustness audit.
Created attachment 917 [details] [review] small fixes to the jpeg and tiff loaders
The attached patch makes it less likely that the jpeg loader crashes in low-memory situations and keeps the tiff loader from stumbling over its own feet in some cases (ie forgetting to free global_error).
Created attachment 949 [details] [review] test for TGA loader. always return FALSE for wbmp check
Created attachment 5040 [details] [review] more tests and improvements for several loaders
Created attachment 5046 [details] [review] more robustness fixes for the xpm loader
I have committed the outstanding patches, since they seemed uncontroversial enough to not provoke a single comment in a full month.
jsut trying to use bugzilla in a better way this bug depends on 56837 please correct me if i am wrong
the ico crash with 32bit icons is resolved
Hey Kristian, the patches have all been applied, therefore I removed the PATCH keyword. It is my understanding that PATCH should be added only if the bug report has *outstanding* patches for review. If we add PATCH to any bug report which ever had a patch attached, it will be essentially useless.
Here is a new round of fixes for the ico, tga and wbmp loaders. They are all fixing crashes found by TEST_RANDOMLY_MODIFIED in test-loaders.
Created attachment 6603 [details] [review] the patch
Hmm, mostly looks fine to commit. There looks like there is some random indentation changes in the patch. (We probably should add the appropriate emacs magic comments to the source files in gdk-pixbuf since it doesn't match the rest of GTK+) The error code set seems to be a little funny in some places: if (State->DIBoffset < 0) { + g_set_error (error, + GDK_PIXBUF_ERROR, + GDK_PIXBUF_ERROR_INSUFFICIENT_MEMORY, + _("Invalid header in icon")); + return; + } + Shouldn't that be CORRUPT_IMAGE instead? I sort of wonder if some of the < 0 checks shouldn't actually be unsigned variables instead, but that would require more checking for unsigned/signed comparison problems and probably makes no practical difference.
Yes, I very much agree about the emacs magic comments. I'll go over the patch and filter out irrelevant noise before committing. The error codes are just copy-and-paste leftovers. I'll correct them as well. You're probably right about the < 0 vs. unsigned stuff, but I aimed for minimally invasive fixes.
-Wsign-compare might be good in the gdk-pixbuf subdir, though in general it's really irritating because it warns about comparing an int to an enum.
I have committed the patch after fixing the error codes and reducing the whitespace noise a bit.
More random tests --> more patches... Apart from fixing tga, ico and wbmp for the random tests, the following patch also adds one disabled tiff test which provokes a segfault in TIFFReadDirectory(). Not quite as bad as a double-free in zlib, but still...
Created attachment 6687 [details] [review] the patch
Should we be saving all the bad random images in some kind of regression suite?
You mean as files, not in the form they're now in in test-images.h ?
It doesn't matter what format, I was just wondering if we should be saving the results each time the random mutation thing finds an image that breaks a loader.
Yes, I agree. Unfortunately, I didn't save the bad seeds, so I'll have to back out the patch and run test-loaders for some time to rediscover the crashers...
There's no need to do that, just something to keep in mind in the future...
New patch looks reasonable. (I might suggest a switch instead of a == foo1 || a == foo2 || a == foo3 || a == foo4....). [ Actlually, why don't you go ahead and make further fixes as you think are appropriate. While I might catch a thing or two, without spending a lot of time, I don't think I have significnatly more insight into this code than you have, probably significantly less. ]
The TIFFReadDirectory problem mentioned above was observed with libtiff 3.5.5, the current version 3.5.7 doesn't crash.
I applied the last patch.
Moving this back to the 2.0.0 milestone since the patch is in and I don't have any outstanding crashes which are not either a) crashes in an external image library b) GError trying to strdup in an OOM situation
Loading untrusted image data will just have to be a post-2.0.0 feature :-(. Sat Mar 2 21:28:03 2002 Owen Taylor <otaylor@redhat.com> * gdk-pixbuf.c (gdk_pixbuf_new): Bullet-proof against integer overflow. fixes one obvious problem.
Here is another round of patches, this time fixing up the pnm and ras loaders far enough to survive extended random testing. Tests included.
Created attachment 7134 [details] [review] ras / pnm patch
Turns out I was wrong about the TIFFReadDirectory crash, it still occurs with 3.5.7. Seems like a libtiff bug report is in order.
Reported as http://bugzilla.remotesensing.org/show_bug.cgi?id=109
The ras / pnm patch has been committed.
Move open bugs from milestones 2.0.[012] -- > 2.0.3, since 2.0.2 is already out.
Created attachment 10696 [details] JPEG file making gdk-pixbug crash badly
Works fine in head, probably a CMYK jpeg.
Putting on 2.4.0 since the remaining project here (developing a standard for a formal audit and doing it) is a rather large job.
Shall this be closed after more than three years of inactivity?
In reply to comment #49) > Shall this be closed after more than three years of inactivity? Yes. If people still run into issues with gtk 2.12 or later, they can reopen or file a new ticket.