GNOME Bugzilla – Bug 779012
Possible out-of-bounds read or undefined behavior in io-ico.c
Last modified: 2017-11-30 04:09:27 UTC
Created attachment 346315 [details] A bad ico file to reproduce the bug Before the patch to bug 313818 (https://bugzilla.gnome.org/show_bug.cgi?id=313818) it was clear it was possible to overflow State->HeaderSize in io-ico.c. See line 334: State->HeaderSize = entry->DIBoffset + INFOHEADER_SIZE; So a check of (State->HeaderSize < 0) was added after this operation. However, I noticed that with optimization compilation flags, this check never took place. Specifically when the project was compiled with gcc with the flags "-O1 -fstrict-overflow -ftree-vrp" (or anything including these, such as -O2 or -O3). You can read about these flags to understand why this happens. This is a problem because I know that many distributions do default to allowing optimizations when building packages and I believe the default of JHBuild is to compile with -O2. This leads to a possible out-of-bounds read of BIH later in line 362 From line 359: BIH = Data+entry->DIBoffset; /* A compressed icon, try the next one */ if ((BIH[16] != 0) || (BIH[17] != 0) || (BIH[18] != 0) || (BIH[19] != 0)) { ... For a fix perhaps check the size of State->DIBoffset before setting State->HeaderSize (should be less than (maximum of State->HeaderSize) - INFOHEADER_SIZE). Attached is a file that can be used to reproduce the issue. I tested it on Ubuntu 16.04.1 (both 32-bit and 64-bit versions). The bug was found thanks to afl.
This bug reminds me of https://bugzilla.gnome.org/show_bug.cgi?id=770986. I am attaching a patch to fix this problem. I have only compile-tested my patch as I am unfamiliar with gdk-pixbuf development and testing process.
Created attachment 347366 [details] [review] patch to avoid invoking undefined behavior
I can confirm that my patch avoids the crash triggered by this bug successfully.
Review of attachment 347366 [details] [review]: Patch looks good to me.
Created attachment 364647 [details] [review] ico: Fix potential integer overflow Which relies on undefined behaviour. Instead of checking for an overflowed integer after the fact, check whether the addition would be possible at all. Fixes: CVE-2017-6312
Created attachment 364648 [details] [review] tests: Add test for CVE-2017-6312 Provided by Ariel Zelivansky <ariel.zelivans@gmail.com>
Dhiru, could you please attach git-formatted patches in the future? It makes it easier to apply, and keep authorship. I needed to apply your patch by hand, write a commit message, copy/paste your email address from the bug... I couldn't reproduce the original bug with or without the patch, so I'll take your word on it fixing the problem. Please reopen if the problem is still present, or I somehow didn't apply the patch correctly.
Attachment 364647 [details] pushed as dec9ca2 - ico: Fix potential integer overflow Attachment 364648 [details] pushed as a6303ad - tests: Add test for CVE-2017-6312
> Dhiru, could you please attach git-formatted patches in the future? I will certainly do so. > I couldn't reproduce the original bug with or without the patch. It has been a while but I believe that I was able to reproduce this problem on CentOS 6 or 7. ---- Thanks for fixing this bug!