GNOME Bugzilla – Bug 739134
Out of bounds read / heap overflow in tga importer / function bgr2rgb.part.1
Last modified: 2017-12-21 00:17:24 UTC
The tga importer has an out of bounds read / heap overflow bug. The bug can be triggered with the attached sample when GIMP was compiled with address sanitizer. I'll attach the sample and the output of address sanitizer. This is a potential (low severity) security issue, however as it is only a read error it's unlikely there's a realistic exploit scenario. Still it should be fixed.
Created attachment 289280 [details] sample tga file exposing the bug
Created attachment 289281 [details] address sanitizer output
Created attachment 289282 [details] decoded / symbolized address sanitizer output
Created attachment 363975 [details] [review] patch to prevent trying to load rgba/32 bit image where bytes per pixel is not 4 I made an attempt at fixing this. The problem is that for an RGBA image the code assumes it's always 4 bytes / 32 bits per pixel, however you can define less bits per pixel in the header, which should be invalid. The attached patch will catch this case and prevent loading. This fixes this case, but there may very well be similar issues in the code.
Thanks for the patch! Next time, could you make it a git-formatted patch so that it is easier to apply, and has proper authorship and description comment? Are the name and email used in bugzilla the right authorship I can use if I push this change? Anyway I am going to review now. > This fixes this case, but there may very well be similar issues in the code. I am sure there are. We'll be happy to get more patches if you find more. :-)
yes, feel free to add me as the author in all patches I upload with Hanno Böck / hanno[at]hboeck[dot]de.
Review of attachment 363975 [details] [review]: So the patch has a few issues. First the validity check you do can be made earlier, inside `load_image()` when the header is parsed. We can already stop here, so let's do the error check as soon as possible (also that's where most similar checks on supported variants of TGA are already done). Second issue is that, when you say: > The problem is that for an RGBA image the code assumes it's always 4 bytes / 32 bits per pixel, however you can define less bits per pixel in the header, which should be invalid. This is actually not right. I could not find an official spec for TGA, but found a bunch of specs here and there. Basically the `bitsperpixel` (offset x10 in the file) allows variants, even within RGB/RGBA images. For instance, on this link: http://www.paulbourke.net/dataformats/tga/ > The bitsperpixel specifies the size of each colour value. When 24 or 32 the normal conventions apply. For 16 bits each colour component is stored as 5 bits and the remaining bit is a binary alpha value. The colour components are converted into single byte components by simply shifting each component up by 3 bits (multiply by 8). And it turns out we have the support in our TGA plug-in. Actually we have support for RGB 15 and 24 bits, and RGBA 16 and 32 bits. With 15/16, the function upsample() will be used, whereas that for 24/32, it uses bgr2rgb(). Your patch would break such support of valid TGA files (RGBA stored as 16-bits in particular would lose support although it is valid). Now you are right that the code (in particular these 2 functions) make assumptions without comparing with the values in the header. And that's where the issue happens. In your example TGA file, I can see in a hexadecimal editor that it is a 24bits RGBA image with 4 declared bits of alpha, which is wrong (I don't know if this could theoretically be supported, though it would be weird since RGB would be on 20 bits, which is not a multiple of 3! In any case, we clearly don't have support for such variant, whether possible or not valid theoretically, so we must stop there for sure). Simply we must be more thorough with our checks regarding what we support or not. While I was reviewing, I have written a more correct patch for this issue.
Ok here is the patch fixing the issue and not losing support for valid 16 bits RGBA TGA files. We are simply more thorough on our tests, and we do them earlier. Basically let's check that 15-bit and 24-bit RGB images have 0 alpha bits, that 16-bit RGBA has 1 alpha bit and finally 32-bit RGBA has 8 alpha bits. commit 674b62ad45b6579ec6d7923dc3cb1ef4e8b5498b (HEAD -> master, origin/master, origin/HEAD) Author: Jehan <jehan@girinstud.io> Date: Wed Dec 20 13:02:38 2017 +0100 Bug 739134 - (CVE-2017-17786) Out of bounds read / heap overflow in... ... TGA importer. Be more thorough on valid TGA RGB and RGBA images. In particular current TGA plug-in can import RGBA as 32 bits (8 bits per channel) and 16 bits (5 bits per color channel and 1 bit for alpha), and RGB as 15 and 24 bits. Maybe there exist more variants, but if they do exist, we simply don't support them yet. Thanks to Hanno Böck for the report and a first patch attempt.
Small update. According to: http://www.paulbourke.net/dataformats/tga/ > For the Targa 16, this would be 0 or 1 (about the alpha bits) So it is possible to have a 16-bit RGB with the last bit simply ignored. Code updated. commit 8ea316667c8a3296bce2832b3986b58d0fdfc077 (HEAD -> master, origin/master, origin/HEAD) Author: Jehan <jehan@girinstud.io> Date: Wed Dec 20 13:26:26 2017 +0100 plug-ins: TGA 16-bit RGB (without alpha bit) is also valid. According to some spec on the web, 16-bit RGB is also valid. In this case, the last bit is simply ignored (at least that's how it is implemented right now). plug-ins/common/file-tga.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Thanks. It would be ideal to have a test suite that checks the rendering of different variations of all supported input file formats against a raw test rendering. That would make it easier to check such patches for correctness. But I guess I'll bring that up on the dev mailing list. I must admit it's challenging for me to try to propose fixes for these issues while not being overly familiar with the code and the supported formats.
> It would be ideal to have a test suite Test suites are always welcome. Current code base has tests, but they are clearly not extensive and we suck a lot at increasing out test base. Patches welcome in this direction. ;-) > I must admit it's challenging for me to try to propose fixes for these issues while not being overly familiar with the code and the supported formats. No prob. It's also the first time I look at this plug-in code. :-)
P.S.: the commits have also been backported to gimp-2-8 branch. commit 22e2571c25425f225abdb11a566cc281fca6f366 (HEAD -> gimp-2-8, origin/gimp-2-8) Author: Jehan <jehan@girinstud.io> Date: Wed Dec 20 13:26:26 2017 +0100 plug-ins: TGA 16-bit RGB (without alpha bit) is also valid. According to some spec on the web, 16-bit RGB is also valid. In this case, the last bit is simply ignored (at least that's how it is implemented right now). (cherry picked from commit 8ea316667c8a3296bce2832b3986b58d0fdfc077) plug-ins/common/file-tga.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) commit ef9c821fff8b637a2178eab1c78cae6764c50e12 Author: Jehan <jehan@girinstud.io> Date: Wed Dec 20 13:02:38 2017 +0100 Bug 739134 - (CVE-2017-17786) Out of bounds read / heap overflow in... ... TGA importer. Be more thorough on valid TGA RGB and RGBA images. In particular current TGA plug-in can import RGBA as 32 bits (8 bits per channel) and 16 bits (5 bits per color channel and 1 bit for alpha), and RGB as 15 and 24 bits. Maybe there exist more variants, but if they do exist, we simply don't support them yet. Thanks to Hanno Böck for the report and a first patch attempt. (cherry picked from commit 674b62ad45b6579ec6d7923dc3cb1ef4e8b5498b) plug-ins/common/file-tga.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Created attachment 365807 [details] poc file smaller (tga) Just in case someone cares, a much smaller example file triggering the bug (18 bytes).
It fails to load with appropriate message "Unhandled sub-format in '/home/jehan/dev/tmp/poc.tga' (type = 2, bpp = 24, alpha = 8)". Looks all fine. :-)