GNOME Bugzilla – Bug 739133
CVE-2017-17785 Out of bounds write / heap overflow in the .fli importer / fli_read_brun
Last modified: 2017-12-22 15:08:37 UTC
I discovered an out-of-bounds write / heap overflow in the fli importer of GIMP. This can be triggered by the attached sample which has been generated by the zzuf fuzzing tool. It will crash the plugin if gimp has been compiled with -fsanitize=address. I'll attach the sample and the output of address sanitizer. This may be a security issue if a user opens files from untrusted sources.
Created attachment 289275 [details] sample fli exposing out of bounds write
Created attachment 289276 [details] address sanitizer output
Created attachment 289277 [details] address sanitizer output decoded / symbolized
Hi Hanno More than these specific bug reports, I'm very glad to see someone using fuzz testing on the file plug-ins. Please can you mail the gimp-developer list with a HOWTO to test a single file plug-in? I'm sure others will also pick this up. If you can, can you test the following most frequently used formats: * XCF * JPEG * PNG * TIFF If you have more time: * GIF * BMP * PSD * PDF * PS
Hanno, do you plan to write patches for these problems?
Not really, at least not any time soon. But I'll probably follow the request to post a quick how-to-fuzz-gimp-plugins to your dev list.
Created attachment 362488 [details] [review] gimp-fli.patch Fixes given test case and more RLE issues in the code.
I assume the patch was tested on some files? If yes it should simply be pushed to master.
FYI this security issue has been given the following CVE number: CVE-2017-17785
Review of attachment 362488 [details] [review]: I did a review, that looks good. There is only this line I have been wondering about: > + if (numline > fli_header->height || fli_header->height-numline > firstline) > + return; Are you sure about the test, in particular the second part? Maybe I don't understand what firstline and numline are meant to be (in this case, naming is not good), but if I do, I would think that firstline + numline should always be lesser than the height (therefore this test would always return TRUE for a valid file). Do I misunderstand something? Also same as Mitch, comment 8, has this been tested with the FLI files running the exploit? As soon as you clarify my question on the if test and confirm that it has been thoroughly tested, I see no reason why not to push immediately.
(In reply to Jehan from comment #10) > > + if (numline > fli_header->height || fli_header->height-numline > firstline) > > + return; > > Are you sure about the test, in particular the second part? You are correct, it's supposed to be larger or equal, therefore the test must check the "lesser than" case.
Created attachment 365798 [details] [review] gimp-fli.patch Updated patch
Review of attachment 365798 [details] [review]: Hmmmm… Sorry to be boring, but: > + if (numline > fli_header->height || fli_header->height-numline < firstline) Shouldn't there be an '=' now? > + if (numline > fli_header->height || fli_header->height-numline <= firstline) I mean if firstline starts at 0 (which I assume it does as it should!), then it should be '<=' because then being at height is already out of bounds! Also you didn't answer if this has been tested with actual files too. This is an important question. :-)
(In reply to Jehan from comment #13) > Shouldn't there be an '=' now? > > > + if (numline > fli_header->height || fli_header->height-numline <= firstline) No, being equal is a good condition. Imagine that height is 1 and firstline is 0. Then numline (the amount of iterations through the for-loop) can be 1, as that's a valid range. > Also you didn't answer if this has been tested with actual files too. This > is an important question. :-) I have no real fli files. I was unable to reproduce the crash/exploit with applied patch, but then again, the initial patch broke the LC implementation as well.
FLI files: https://samples.ffmpeg.org/fli-flc/ I'll do some tests later.
> No, being equal is a good condition. Indeed, you are right. > I have no real fli files. I found this link: http://netghost.narod.ru/gff/sample/images/fli/index.htm I was able to properly open the 2 FLI files available there before and after the patch. And this patch looks good to me after the last review, and it would obviously fix some cases of out-of-bounds memory reading. Pushed in master: commit edb251a7ef1602d20a5afcbf23f24afb163de63b (HEAD -> master, origin/master, origin/HEAD) Author: Tobias Stoeckmann <tobias@stoeckmann.org> Date: Sun Oct 29 15:19:41 2017 +0100 Bug 739133 - (CVE-2017-17785) Heap overflow while parsing FLI files. It is possible to trigger a heap overflow while parsing FLI files. The RLE decoder is vulnerable to out of boundary writes due to lack of boundary checks. The variable "framebuf" points to a memory area which was allocated with fli_header->width * fli_header->height bytes. The RLE decoder therefore must never write beyond that limit. If an illegal frame is detected, the parser won't stop, which means that the next valid sequence is properly parsed again. This should allow GIMP to parse FLI files as good as possible even if they are broken by an attacker or by accident. While at it, I changed the variable xc to be of type size_t, because the multiplication of width and height could overflow a 16 bit type. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> In 2.8: commit 1882bac996a20ab5c15c42b0c5e8f49033a1af54 (HEAD -> gimp-2-8, origin/gimp-2-8) Author: Tobias Stoeckmann <tobias@stoeckmann.org> Date: Sun Oct 29 15:19:41 2017 +0100 Bug 739133 - (CVE-2017-17785) Heap overflow while parsing FLI files. It is possible to trigger a heap overflow while parsing FLI files. The RLE decoder is vulnerable to out of boundary writes due to lack of boundary checks. The variable "framebuf" points to a memory area which was allocated with fli_header->width * fli_header->height bytes. The RLE decoder therefore must never write beyond that limit. If an illegal frame is detected, the parser won't stop, which means that the next valid sequence is properly parsed again. This should allow GIMP to parse FLI files as good as possible even if they are broken by an attacker or by accident. While at it, I changed the variable xc to be of type size_t, because the multiplication of width and height could overflow a 16 bit type. Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> (cherry picked from commit edb251a7ef1602d20a5afcbf23f24afb163de63b) plug-ins/file-fli/fli.c | 50 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 15 deletions(-) Thanks for the patches!
For information: after checking that we had no pending patch related to FLI support (and actually no bug report even, it would seem), I just cleaned up the whole FLI plug-in to follow our coding style. Previous style was a huge compressed mess which was very hard to read (and a bit suffocating IMO!). There should be absolutely no code difference before and after (and if there is, that is a mistake from me), only coding style fix (spaces and such). Yet that touches the whole file. So if you were planning to do more stuff on this file, don't forget to push. Working on previous code will likely not merge well. :-) Of course I tested afterwards with a bunch of FLI files (from the link I gave in comment 16 as well as from the link Hanno gave in comment 15) and it looks I didn't break anything by mistake. :-P