Loading malformed GIF wxImage can cause crash (Issue #23409)
PB
(Just passing the report from the forum)
Description
When loading a GIF, wxGIFDecoder::getcode() does not account for function obtaining the block size, stream.GetC(), returning wxEOF. wxEOF is defined as -1, while the variable for the block size m_restbyte is an unsigned int, resulting in the code attempting to write 4,294,967,295 bytes to a 256-byte m_buffer.
https://github.com/wxWidgets/wxWidgets/blob/9b0ebf2c3a81cfddaa4933f5a2ae718638d82b8c/src/common/gifdecod.cpp#L300-L316
Expected vs observed behaviour:
I would expect the malformed GIF refused to be loaded without the application crashing; however, a crash happens due to overwriting memory. I have tried a couple of programs to view the GIF, none crashed and they all displayed a partial image.
To Reproduce:
- Download the attached image.
- Run the image sample.
- Press <Ctrl+O> and select the downloaded image.
- Observe the sample to crash or just quietly close.
Proposed fix
I think the simplest fix would be the image processing being stopped when m_restbyte == (unsigned int)wxEOF. However, I have no idea how should wxGIFDecoder::getcode() tell wxGIFDecoder::dgif() that the image is invalid (i.e., what code to return).
Another/better solution could be to just somehow deal with it and have the partial image as other programs do.
Malformed image:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
FWIW ImageMagick identify reports:
bad.gif GIF 1200x800 1200x800+0+0 8-bit sRGB 256c 33KB 0.016u 0:00.015
identify: Corrupt image `bad.gif' @ error/gif.c/PingGIFImage/964.
so I agree that giving an error for it would be enough.
But we definitely need to fix this. The trouble is that I see several other occurrences of GetC() where we use it without checking if it returned EOF in this code. We could try fixing them manually but it would be really nice to fuzz this code by adding GIF fuzzer to tests/fuzz.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
FWIW, I have found only one other place where the value of stream.GetC() is used without checking for stream.Eof() first:
https://github.com/wxWidgets/wxWidgets/blob/13ebaee247781b7cde1c830b36145aec9ab91127/src/common/gifdecod.cpp#L862-L867
The same fix as I did above (i.e., checking bits both for 0 and wxEOF) could be used there. However, I assume that stream.GetC() can return wxEOF when there was any error reading the stream, not just actually reaching its end.
I am not arguing against fuzzying the code (I have no idea how to do it) and/or any other solution. Since no one ran into this issue before, in all those years the code existed, there is probably no need to fix it ASAP.
BTW, Safari on my iPad is the only software that refused to display this invalid image at all, showing a question mark icon instead.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #23409 as completed via c2e5749.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I hoped to write more tests for this code, but I didn't have time for it, so finally I'm just going to apply (a slightly modified form of) your patch and include it in 3.2.3, as it's much better than holding on for a better solution, even if fuzzing this code would surely find more problems...
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.