GNOME Bugzilla – Bug 776040
Integer Overflow leading to weird behavior in gdk-pixbuf ico parser
Last modified: 2017-08-12 01:36:14 UTC
Created attachment 341870 [details] crashing file, password "crash", found by afl from an email written by Hanno Boeck to security@: I have discovered an interesting bug in gdk-pixbuf that shows some of the weird behaviors that can happen with undefined C. The bug is this line: State->HeaderSize = entry->DIBoffset + 40; entry->DIBoffset is a signed integer. If it's an insane large number close to INT_MAX then the addition can overflow. Looking at the code one might assume this shouldn't matter, because right after that there's a check if State->HeaderSize is smaller than zero. However when compiling gdk-pixbuf with address sanitizer this check is bypassed and an invalid memory read happens later on. An Integer Overflow is undefined behavior in C, therefore what the compiler does here is legal. Whether this bug appears seems to depend on the compiler switches used, but I've seen it both with an address sanitizer compile with clang and with my local installation resulting in a crash (that is gcc based).
Created attachment 341871 [details] [review] potential patch patch written by Hanno (CCed)
FWIW: I couldn't figure out how to add it to the test suite :-/ I needed to comment the cve-2015-4491.test out because it uses too much memory for my machine. Then, make check fails for me already with 14 errors in pixbuf tests. I tried to drop the crashing file in tests/ or tests/test-images/fail, but that doesn't seem to create another test case.
This already throws "Invalid header in icon (header size)" without the address sanitizer. A reproducer would be nice.
Created attachment 341948 [details] [review] tests: Add test for bug 776040
Created attachment 341951 [details] [review] potential patch True. I cannot reproduce at the moment. I tried to compile with -O2 and -O3, but the check isn't optimised away. But there is a very similar issue which also covers this one en-passant: DIBoffset is set via data_offset which gets the raw bytes out of the stream. It left-shifts one byte by 24 digits. This seems to be undefined behaviour for a signed int. DIBoffset probably ought to be a size_t. Then we can easily make data_offset an unsigned type, too. With such an unsigned type, the check for < 0 mentioned here in this bug report does not make sense. So it can be gotten rid of. We want to add 40 to DIBoffset and assign it to HeaderSize. HeaderSize is currently a signed int (I think it can be unsigned, too). Because HeaderSize's upper limit is INT_MAX, we check whether we can add another 40 to DIBoffset by checking whether it is greater than INT_MAX - 40. FTR: bug 313818 has probably caused this faulty overflow check.
Created attachment 341952 [details] [review] potential patch
Created attachment 345695 [details] [review] patch I had this lying around still.
I downloaded attachment 341870 [details], decrypted it, and ran pixbuf-read against it: ➜ gdk-master git:(omaster) ✗>git log -n1 commit df7c32c15d63ce1570a737bb73d3d3e2cd31f2f2 Author: Matthias Clasen <mclasen@redhat.com> Date: Mon Feb 13 11:02:01 2017 -0500 2.36.5 ➜ gdk-master git:(omaster) ✗>./tests/pixbuf-read ~/vcs/gdk-pixbuf/776040.ico /home/muelli/vcs/gdk-pixbuf/776040.ico io-ico.c:334:40: runtime error: signed integer overflow: 2147483647 + 40 cannot be represented in type 'int' error: Invalid header in icon (header size) ➜ gdk-master git:(omaster) ✗> So it's reproducible on my end. Hence I am reopening. I hope this is enough to apply a fix for this issue. FTR: I used ./autogen.sh --prefix=/tmp/gdkpb-omaster/ --enable-installed-tests --enable-always-build-tests --enable-introspection=no AFL_HARDEN=1 ASAN_OPTIONS=detect_leaks=0 V=1 CFLAGS="-O0 -g3 -fno-common -fsanitize=address -fsanitize=undefined" ; env ASAN_OPTIONS=detect_leaks=0 make -j ; make install
Review of attachment 345695 [details] [review]: This should probably be 3 separate patches. > data_size is only used to assign it to ImageScore, so it, too, can be > made an unsigned int. data_size is only used to print it. ::: gdk-pixbuf/io-ico.c @@ +129,3 @@ /* Score the various parts of the icon */ struct ico_direntry_data { + guint ImageScore; You can make that a gint64 and avoid the type promotion in compare_direntry_scores(). @@ +132,3 @@ gint width; gint height; + size_t DIBoffset; I'd rather you used gint64 rather than intermediate types like size_t. @@ +167,3 @@ struct headerpair Header; /* Decoded (BE->CPU) header */ GList *entries; + size_t DIBoffset; Ditto. @@ +324,3 @@ + /* We check whether the HeaderSize (int) would overflow */ + if (entry->DIBoffset > INT_MAX - INFOHEADER_SIZE) We shouldn't actually get here. We should check earlier, before assigning: entry->DIBoffset = data_offset; and creating a ico_direntry_data structure, whether the data_offset is valid. That way, we can skip broken icon entries altogether rather than erroring out. But if you're going to make the changes minimal, please split this out into a separate patch.
Created attachment 355549 [details] [review] ico: Avoid possible ImageScore overflow Seeing as this is only a comparison, don't try to do arithmetics that could overflow. Detected by UBSan: io-ico.c:204:9: runtime error: signed integer overflow: 2134193699 - -555819298 cannot be represented in type 'int'
Created attachment 355550 [details] [review] ico: Don't use signed ints to do uints offset arithmetics Make sure that the calculations we're doing on unsigned bytes can fit in the target variable by making it a 32-bit unsigned int. As detected by UBSan: io-ico.c:288:26: runtime error: left shift of 146 by 24 places cannot be represented in type 'int' io-ico.c:287:38: runtime error: left shift of 222 by 24 places cannot be represented in type 'int'
Created attachment 355551 [details] [review] ico: Fix possible offset overflow In case the offset is MAXINT, don't try to get a header size past it.
Created attachment 355552 [details] [review] ico: Try to skip over broken icon entries If an icon entry is broken, skip over it, but report the breakage if we could not find a single valid entry.
cool. thanks. Patches LGTM, FWIW.
Attachment 341948 [details] pushed as 925eac5 - tests: Add test for bug 776040 Attachment 355549 [details] pushed as 99508c7 - ico: Avoid possible ImageScore overflow Attachment 355550 [details] pushed as e08c115 - ico: Don't use signed ints to do uints offset arithmetics Attachment 355551 [details] pushed as b92030b - ico: Fix possible offset overflow Attachment 355552 [details] pushed as be43c8f - ico: Try to skip over broken icon entries