After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775693 - buffer overflow in pixdata loader
buffer overflow in pixdata loader
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: loaders
git master
Other Linux
: Normal normal
: ---
Assigned To: gdk-pixbuf-maint
gdk-pixbuf-maint
Depends on:
Blocks:
 
 
Reported: 2016-12-06 10:51 UTC by Tobias Mueller
Modified: 2017-02-17 10:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
crashing file, password "crash", found by afl (614 bytes, application/pgp-encrypted)
2016-12-06 10:51 UTC, Tobias Mueller
  Details
potential patch (1.93 KB, patch)
2016-12-06 10:52 UTC, Tobias Mueller
committed Details | Review
tests: Add test for bug 775693 (3.06 KB, patch)
2016-12-13 15:53 UTC, Bastien Nocera
committed Details | Review
tests: Additional test for bug 775693 (889 bytes, patch)
2016-12-16 15:36 UTC, Bastien Nocera
committed Details | Review
tests: Fix bug in pixdata test (1.32 KB, patch)
2016-12-16 15:36 UTC, Bastien Nocera
committed Details | Review
pixdata: Add debug when decoding pixdata (1.17 KB, patch)
2016-12-16 15:36 UTC, Bastien Nocera
committed Details | Review
Revert "pixdata: Prevent buffer overflow by checking for bounds before memcpy" (1.95 KB, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review
pixdata: Avoid copying more data than available (2.03 KB, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review
pixdata: Add "-r" as an alias for "--rle" (801 bytes, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review
pixdata: Add "--rle" to the --help output (1.00 KB, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review
tests: Add more pixdata tests (7.58 KB, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review
pixdata: Check for RLE pixdata length (2.88 KB, patch)
2016-12-16 15:37 UTC, Bastien Nocera
committed Details | Review

Description Tobias Mueller 2016-12-06 10:51:20 UTC
Created attachment 341455 [details]
crashing file, password "crash", found by afl

The attached file crashes the pixdata loader:

==8826==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000019880 at pc 0x7ffff6ef6935 bp 0x7fffffffb270 sp 0x7fffffffaa18
READ of size 1060864 at 0x61d000019880 thread T0
    #0 0x7ffff6ef6934 in __asan_memcpy (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x8c934)
    #1 0x7ffff6bcf3ed in gdk_pixbuf_from_pixdata /home/muelli/vcs/gdk-pixbuf/gdk-pixbuf/gdk-pixdata.c:570
    #2 0x7ffff6be581c in try_load /home/muelli/vcs/gdk-pixbuf/gdk-pixbuf/io-pixdata.c:85
    #3 0x7ffff6be61c5 in pixdata_image_load_increment /home/muelli/vcs/gdk-pixbuf/gdk-pixbuf/io-pixdata.c:156
    #4 0x7ffff6bbd146 in gdk_pixbuf_loader_load_module /home/muelli/vcs/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-loader.c:443
    #5 0x7ffff6bbeba6 in gdk_pixbuf_loader_close /home/muelli/vcs/gdk-pixbuf/gdk-pixbuf/gdk-pixbuf-loader.c:808
    #6 0x400e74 in test_loader /home/muelli/vcs/gdk-pixbuf/tests/pixbuf-read.c:35
    #7 0x40121c in main /home/muelli/vcs/gdk-pixbuf/tests/pixbuf-read.c:75
    #8 0x7ffff555e82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #9 0x400d08 in _start (/tmp/gdkpb/libexec/installed-tests/gdk-pixbuf/pixbuf-read+0x400d08)

0x61d000019880 is located 0 bytes to the right of 2048-byte region [0x61d000019080,0x61d000019880)
allocated by thread T0 here:
    #0 0x7ffff6f02961 in realloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98961)
    #1 0x7ffff68807e7 in g_realloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4f7e7)

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 __asan_memcpy
Shadow bytes around the buggy address:
  0x0c3a7fffb2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a7fffb2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a7fffb2e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a7fffb2f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3a7fffb300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3a7fffb310:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a7fffb320: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a7fffb330: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a7fffb340: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a7fffb350: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a7fffb360: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
==8826==ABORTING

Program received signal SIGABRT, Aborted.
0x00007ffff5573428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) up
  • #1 __GI_abort
    at abort.c line 89
  • #2 ??
    from /usr/lib/x86_64-linux-gnu/libasan.so.2
  • #3 ??
    from /usr/lib/x86_64-linux-gnu/libasan.so.2
  • #4 ??
    from /usr/lib/x86_64-linux-gnu/libasan.so.2
  • #5 __asan_report_error
    from /usr/lib/x86_64-linux-gnu/libasan.so.2
  • #6 __asan_memcpy
    from /usr/lib/x86_64-linux-gnu/libasan.so.2
  • #7 gdk_pixbuf_from_pixdata
    at gdk-pixdata.c line 570
565	                               _("Image pixel data corrupt"));
566		  return NULL;
567		}
568	    }
569	  else if (copy_pixels)
570	    memcpy (data, pixdata->pixel_data, pixdata->rowstride * pixdata->height);
571	  else
572	    data = pixdata->pixel_data;
573	
574	  return gdk_pixbuf_new_from_data (data, GDK_COLORSPACE_RGB,
(gdb) p data
$1 = (guint8 *) 0x7ffff7e67800 '\276' <repeats 200 times>...
(gdb) p *pixdata
$2 = {magic = 1197763408, length = 80, pixdata_type = 822149377, rowstride = 256, width = 48, height = 4144, pixel_data = 0x61d000019098 "\b\002"}
(gdb) 

This overflow is a bit different than the ones reported so far, because it overflows a buffer allocated by the function itself.
Comment 1 Tobias Mueller 2016-12-06 10:52:04 UTC
Created attachment 341456 [details] [review]
potential patch
Comment 2 Bastien Nocera 2016-12-13 15:53:43 UTC
Created attachment 341885 [details] [review]
tests: Add test for bug 775693
Comment 3 Bastien Nocera 2016-12-13 16:34:45 UTC
Fix committed with a few style fixes.

Attachment 341885 [details] pushed as 3724a73 - tests: Add test for bug 775693
Comment 4 Tobias Mueller 2016-12-13 16:38:56 UTC
amazing! Very good work.
FTR: https://git.gnome.org/browse/gdk-pixbuf/commit/?id=9ae4723ec3fa631354e3d201c5435a7385c33d45 is the fix.
Comment 5 Allin Cottrell 2016-12-15 17:17:58 UTC
The fix seems wrong for the case of copy_pixels true and
encoding != GDK_PIXDATA_ENCODING_RLE. In this case we have:

...
const size_t data_size = pixdata->height * pixdata->rowstride;
data_limit = data + data_size;
...
if (data + (pixdata->rowstride * pixdata->height) < data_limit)
  memcpy(<args>)
else
  <fail>

None of the variables are modifed in the second ellipsis, so
I don't see how the condition for proceeding with the memcpy
can be satisfied: we should have

data + (pixdata->rowstride * pixdata->height) == data_limit

not less than.
Comment 6 Bastien Nocera 2016-12-16 12:56:51 UTC
(In reply to Allin Cottrell from comment #5)
> The fix seems wrong for the case of copy_pixels true and
> encoding != GDK_PIXDATA_ENCODING_RLE. In this case we have:
> 
> ...
> const size_t data_size = pixdata->height * pixdata->rowstride;
> data_limit = data + data_size;
> ...
> if (data + (pixdata->rowstride * pixdata->height) < data_limit)
>   memcpy(<args>)
> else
>   <fail>
> 
> None of the variables are modifed in the second ellipsis, so
> I don't see how the condition for proceeding with the memcpy
> can be satisfied: we should have
> 
> data + (pixdata->rowstride * pixdata->height) == data_limit
> 
> not less than.

Yeah, the fix is incorrect. It needs to check the ->length, if set.
Comment 7 Bastien Nocera 2016-12-16 15:36:46 UTC
Created attachment 342067 [details] [review]
tests: Additional test for bug 775693
Comment 8 Bastien Nocera 2016-12-16 15:36:52 UTC
Created attachment 342068 [details] [review]
tests: Fix bug in pixdata test

In 3724a739 we added a test for the deserializing code, but we shouldn't
be free'ing the contents straight away, as gdk_pixdata_deserialize()
does not copy the data, but expects it to stay around until we've copied
the data to the pixbuf (with gdk_pixbuf_from_pixdata() and copy_pixels
== TRUE), or we're done with the pixbuf.
Comment 9 Bastien Nocera 2016-12-16 15:36:58 UTC
Created attachment 342069 [details] [review]
pixdata: Add debug when decoding pixdata
Comment 10 Bastien Nocera 2016-12-16 15:37:03 UTC
Created attachment 342070 [details] [review]
Revert "pixdata: Prevent buffer overflow by checking for bounds before memcpy"

The fix is incorrect.

This reverts commit 9ae4723ec3fa631354e3d201c5435a7385c33d45.
Comment 11 Bastien Nocera 2016-12-16 15:37:14 UTC
Created attachment 342071 [details] [review]
pixdata: Avoid copying more data than available

The problem we were encountering in bug 775693 wasn't that we were
running past the end of the memory we just allocated, but that the
length of the data we were given couldn't possibly match the dimensions
of the image.

This fixes running past the end of the given pixdata when the length is
provided inside the structure.
Comment 12 Bastien Nocera 2016-12-16 15:37:20 UTC
Created attachment 342072 [details] [review]
pixdata: Add "-r" as an alias for "--rle"
Comment 13 Bastien Nocera 2016-12-16 15:37:25 UTC
Created attachment 342073 [details] [review]
pixdata: Add "--rle" to the --help output
Comment 14 Bastien Nocera 2016-12-16 15:37:30 UTC
Created attachment 342074 [details] [review]
tests: Add more pixdata tests

Those ones should succeed, and we test both uncompressed and
RLE-compressed data.
Comment 15 Bastien Nocera 2016-12-16 15:37:36 UTC
Created attachment 342075 [details] [review]
pixdata: Check for RLE pixdata length

Avoid copying data from past the end of the pixdata when processing
RLE-encoded pixdata.
Comment 16 Bastien Nocera 2016-12-16 15:38:39 UTC
Attachment 342067 [details] pushed as daaae1b - tests: Additional test for bug 775693
Attachment 342068 [details] pushed as 28fbde8 - tests: Fix bug in pixdata test
Attachment 342069 [details] pushed as 7f075ba - pixdata: Add debug when decoding pixdata
Attachment 342070 [details] pushed as 6ddb160 - Revert "pixdata: Prevent buffer overflow by checking for bounds before memcpy"
Attachment 342071 [details] pushed as 73ce596 - pixdata: Avoid copying more data than available
Attachment 342072 [details] pushed as 2396b4d - pixdata: Add "-r" as an alias for "--rle"
Attachment 342073 [details] pushed as 4f41e7c - pixdata: Add "--rle" to the --help output
Attachment 342074 [details] pushed as a829106 - tests: Add more pixdata tests
Attachment 342075 [details] pushed as 10c8111 - pixdata: Check for RLE pixdata length
Comment 17 Tobias Mueller 2017-02-17 10:08:51 UTC
507    while (image_buffer < image_limit &&
508 	   (rle_buffer_limit != NULL || rle_buffer > rle_buffer_limit))


I'm being curious: Is the "rle_buffer > rle_buffer_limit" limit check correct?

rle_buffer_limit = pixdata->pixel_data + pixdata->length - GDK_PIXDATA_HEADER_LENGTH;

I think rle_buffer_limit describes the byte *after* the buffer, i.e. where we should not be able to read from or write into.  So a typical check is the one that existed before, e.g. "buffer_p < buffer_limit". When checking the "other way around", as I think is done with the patch, it may be that the check needs to test for 
"rle_buffer >= rle_buffer_limit", i.e. whether the current pointer has already hit the limit and not exceeded it.

But: I don't know the code at all and I might be missing some things that are obvious to people with more knowledge.
Comment 18 Bastien Nocera 2017-02-17 10:59:52 UTC
(In reply to Tobias Mueller from comment #17)
> 507    while (image_buffer < image_limit &&
> 508 	   (rle_buffer_limit != NULL || rle_buffer > rle_buffer_limit))
> 
> 
> I'm being curious: Is the "rle_buffer > rle_buffer_limit" limit check
> correct?
> 
> rle_buffer_limit = pixdata->pixel_data + pixdata->length -
> GDK_PIXDATA_HEADER_LENGTH;
> 
> I think rle_buffer_limit describes the byte *after* the buffer,

No, it describes the last byte of the buffer. data_address + length should be the offset for the last byte of data.

At this point, it would be best to file new bugs referencing those older ones if you want to have answers.