EGifGCBToSavedExtension() in egif_lib.c calls EGifGCBToExtension() on
ep->Bytes without checking ep->ByteCount. EGifGCBToExtension() always
writes 4 bytes, so if the existing extension buffer is shorter than
that, the write overflows the heap allocation made by
GifAddExtensionBlock() in gifalloc.c.
This can be reached with a malformed GIF whose Graphics Control
Extension declares a sub-block size less than 4. The GIF89a spec
requires exactly 4, and DGifExtensionToGCB() enforces that on the
read side, but DGifSlurp() stores whatever size the file declared,
so the bogus length survives into the in-memory model. Any client
that round-trips the GIF through DGifSlurp() followed by
EGifGCBToSavedExtension() then corrupts the heap. giftool -d, -t,
-u and -x are all affected via the giftool path that updates the
GCB for selected images.
Reproducer (a 40-byte crafted GIF declaring a 1-byte GCB sub-block):
printf '\x47\x49\x46\x38\x39\x61\x01\x00\x01\x00\x80\x00\x00'\
'\xff\xff\xff\x00\x00\x00\x21\xf9\x01\x00\x00\x2c\x00\x00\x00'\
'\x00\x01\x00\x01\x00\x00\x02\x02\x44\x01\x00\x3b' > bad.gif
./giftool -d 100 < bad.gif > /dev/null
Built with -fsanitize=address, this prints:
ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 1 at 0x... thread T0
#0 EGifGCBToExtension egif_lib.c:668
#1 EGifGCBToSavedExtension egif_lib.c:693
#2 main giftool.c:272
0x... is located 0 bytes after 1-byte region
allocated by thread T0 here:
#0 malloc
#1 GifAddExtensionBlock gifalloc.c:251
#2 DGifSlurp dgif_lib.c:1282
The attached patch fixes this by resizing the existing extension
to exactly 4 bytes before writing, which matches the invariant
DGifExtensionToGCB() already enforces on the read side. It is a
13-line change confined to EGifGCBToSavedExtension(). The full
regression suite (make check) passes with the fix applied.
This bug report is a duplicate of bug #199
As for the patch, I'm not really sure that silently resizing the buffer is the best approach. It does not match DGifExtensionToGCB which refuses the image and returns error. I think that doing the same - just returning error for invalid data would be better solution.