#135 SEGV in gifbuild.c:877

[Manh-Dung NGUYEN]

Hi,
Our fuzzer found a crash in gifbuild (the latest commit on master 866ea06 - version 5.1.9) due to a null pointer dereference.
PoC: https://github.com/strongcourage/PoCs/blob/master/giflib_866ea06/PoC_npd_gifbuild
ASAN says:

gifbuild-asan -d PoC_npd_gifbuild
#
# GIF information from ./PoC_npd_gifbuild
screen width 0
screen height 0
screen colors 2
screen background 0
pixel aspect byte 232

image # 1
image left 0
image top 0
image interlaced
image bits 4 by 1
9927

# The following sets edit modes for GNU EMACS
# Local Variables:
# mode:picture
# truncate-lines:t
# End:
# End of ./PoC_npd_gifbuild dump
ASAN:DEADLYSIGNAL
=================================================================
==18392==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000403d84 bp 0x7fc122903708 sp 0x7ffcac6ff150 T0)
    #0 0x403d83 in Gif2Icon /home/dungnguyen/gueb-testing/giflib-asan/gifbuild.c:877
    #1 0x401c3c in main /home/dungnguyen/gueb-testing/giflib-asan/gifbuild.c:100
    #2 0x7fc12255e82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #3 0x4020b8 in _start (/home/dungnguyen/PoCs/giflib_866ea06/gifbuild-asan+0x4020b8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/dungnguyen/gueb-testing/giflib-asan/gifbuild.c:877 in Gif2Icon
==18392==ABORTING

Thanks,
Manh Dung

[Eric S. Raymond]

Can't reproduce. When I feed this file to gifbuild, it says

Stdin:13: Invalid ASCII pixel key.

Which looks like a good error message given the line of test load indicated. I guess I have a bounds check that isn't in the version you're using.

[Manh-Dung NGUYEN]

Sorry, I still can reproduce the bug in the version 5.2.0 (commit d5cd5d0). I use Ubuntu 16.04 (64 bit). I compiled giflib with ASAN using gcc version 4.9.4 (CFLAGS: -g -fsanitize=address -std=gnu99 -fPIC -Wall -Wno-format-truncation -O2). You can see the details in the attached image.

I think the bug comes from your sanity checks in gifbuild.c (lines 878-881): https://sourceforge.net/p/giflib/code/ci/master/tree/gifbuild.c#l878. When running gifbuild with my input, GifFile->SColorMap is NULL, consequently, there is a null pointer dereference bug. I propose a check as follows:

if (GifFile->SBackGroundColor < 0
        || (GifFile->SColorMap && GifFile->SBackGroundColor >= GifFile->SColorMap->ColorCount)) {
        fprintf(stderr, "gifbuild: background color invalid for screen colormap.\n");
    }

Thanks,
Manh Dung

[Eric S. Raymond]

Manh-Dung NGUYEN strongcourage@users.sourceforge.net:

Sorry, I still can reproduce the bug in the version 5.2.0 (commit d5cd5d0). I use Ubuntu 16.04 (64 bit). I compiled giflib with ASAN using gcc version 4.9.4 (CFLAGS: -g -fsanitize=address -std=gnu99 -fPIC -Wall -Wno-format-truncation -O2). You can see the details in the attached image.

I think the bug comes from your sanity checks in gifbuild.c (lines 878-881): https://sourceforge.net/p/giflib/code/ci/master/tree/gifbuild.c#l878. When running gifbuild with my input, GifFile->SColorMap is NULL, consequently, there is a null pointer dereference bug. I propose a check as follows:
~~~
if (GifFile->SBackGroundColor < 0
|| (GifFile->SColorMap && GifFile->SBackGroundColor >= GifFile->SColorMap->ColorCount)) {
fprintf(stderr, "gifbuild: background color invalid for screen colormap.\n");
}

I've pushed tht change. Sincem for whatever reason, I can't replicare
the crash here would you please verify that it works at your end?
--
Eric S. Raymond (www.catb.org)

[Manh-Dung NGUYEN]

Yes, the proposed patch fixes this bug. Thanks.