GNOME Bugzilla – Bug 778204
gdk-pixbuf-thumbnailer.c fails to set error before returning NULL (causing NULL dereference)
Last modified: 2018-04-17 07:08:17 UTC
Created attachment 344973 [details] Garbage ico file that can be used to reproduce the issue I found a bug that results in a segmentation fault. gnome-thumbnailer-skeleton.c:272 calls file_to_pixbuf. file_to_pixbuf (gdk-pixbuf-thumbnailer.c:209) calls _gdk_pixbuf_new_from_uri_at_scale, which calls gdk_pixbuf_loader_get_pixbuf (see gdk-pixbuf-thumbnailer:195). gdk_pixbuf_loader_get_pixbuf may returns NULL, and it doesn't get a pointer to an error object to set an error. So when it returns NULL, back to, on file_to_pixbuf pixbuf will be NULL, and on line 312 printing error->message will result in a NULL deference (error->message will be 0x0 + 0x8). I am leaving the fix for the development team since I am not sure if gdk_pixbuf_loader_get_pixbuf returning NULL is by design or it is actually an error. Perhaps the problem is not having an error set on io-ico.c.
Created attachment 349903 [details] [review] Fix NULL pointer deference gdk_pixbuf_loader_get_pixbuf returning NULL probably is by design. Thumbnailer crashing because of that certainly isn't. Attached patch should fix that. But underlying problem should probably be fixed also: as you mentioned, io-ico.c should have failed before thumbnailer calls gdk_pixbuf_loader_get_pixbuf. I would suspect gdk_pixbuf__ico_image_load_increment to be failing without return FALSE...
Created attachment 356473 [details] [review] thumbnailer: Update skeleton to fix a possible crash If the loader returns a NULL pixbuf without returning an error, the skeleton would crash trying to print the error. Print that the thumbnailer is broken instead.
(In reply to Ariel Zelivansky from comment #0) > I am leaving the fix for the development team since I am not sure if > gdk_pixbuf_loader_get_pixbuf returning NULL is by design or it is actually > an error. Perhaps the problem is not having an error set on io-ico.c. Haha, totally: 614 /* FIXME this thing needs to report errors if• 615 * we have unused image data• 616 */•
Created attachment 356474 [details] [review] thumbnailer: Update skeleton to fix a possible crash If the loader returns a NULL pixbuf without returning an error, the skeleton would crash trying to print the error. Print that the thumbnailer is broken instead.
Created attachment 356475 [details] [review] ico: Return an error when the ICO didn't load If we don't even read enough data to fill the header, return an error. This doesn't cover everything that could go wrong with the ICO incremental loader, but this is a good first throw.
Created attachment 356476 [details] [review] tests: Add truncated (and broken) ICO file The loader should return an error in that case.
How did this ever get a CVE? The thumbnailers are made to crash, this never was a DoS... Attachment 356474 [details] pushed as 57362ed - thumbnailer: Update skeleton to fix a possible crash Attachment 356475 [details] pushed as 7586553 - ico: Return an error when the ICO didn't load Attachment 356476 [details] pushed as 67a02e1 - tests: Add truncated (and broken) ICO file
Good job. Thanks for fixing this. Jhon https://mrkortingscode.nl/