search

thorfdbg / libjpeg

A complete implementation of 10918-1 (JPEG) coming from jpeg.org (the ISO group) with extensions for HDR, lossless and alpha channel coding standardized as ISO/IEC 18477 (JPEG XT).
327 stars 81 forks source link

[SEGV; heap-buffer-overflow](ycbcrtrafo.cpp:220): index `*r` out of range (`m_plEncodingLUT[0][*r]`) #98

Closed benehalo closed 7 months ago

benehalo commented 7 months ago

Dear All,

This bug was found on Ubuntu 20.04 64-bit & libjpeg was checked out from the master branch of GitHub repository. Its commit is 3d391f06c80b6662d4524d8297e84528b009b774 (Mon Jan 15 07:47:50 2024 +0100).

libjpeg was built with ASAN using clang-14. The compile command was:

cd $PROJECT_SRC
export FLAGS='-DGLOBAL_NEW_IS_OK=1 -DFORTIFY_SOURCE -fstack-protector-all -g -fPIC -fsanitize=address'
make clean 
CC="clang $FLAGS" CXX="clang++ $FLAGS" ./configure
make -j

To reproduce:

Download and unzip the attached zip archive, and get PoCs

$PROJECT_SRC/jpeg -q 10 -R 4 -h [poc]  /dev/null

Bug Analysis

The cause of the bug is the lack of proper range checking for the index *r when it is used or assigned. As a result, the following code in ycbcrtrafo.cpp:220 triggers an index out-of-bounds error

m_plEncodingLUT[0][*r];

See the GDB analysis below for details

GDB Analysis

Breakpoint 1, YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr (this=0x6160000003a0, r=..., source=0x603000000260, target=0x603000000080) at ycbcrtrafo.cpp:220
220               *ydst = INT_TO_COLOR(m_plEncodingLUT[0][*r]);
(gdb) p *r
$4 = 13621

When *r is used as an index to access array m_plEncodingLUT[0], an out-of-bounds error occurs

By backtracking, I find where m_plEncodingLUT[0] is allocated heap memory

Breakpoint 6, ColorTransformerFactory::InstallIntegerParameters (this=0x607000000190, trafo=0x6160000003a0, specs=0x611000000060, count=1, encoding=true, residual=false, inbpp=12 '\f', outbpp=13 '\r', resbpp=12 '\f', rbits=4 '\004', ltrafo=MergingSpecBox::Identity, rtrafo=MergingSpecBox::Zero, ctrafo=MergingSpecBox::Identity) at colortransformerfactory.cpp:337
337             inv = box->InverseScaledTableOf(inbpp,outbpp,0,0);
(gdb) s
InverseToneMappingBox::InverseScaledTableOf (this=0x60d000000060, dctbits=12 '\f', spatialbits=13 '\r', dctfract=0 '\000', spatialfract=0 '\000') at inversetonemappingbox.cpp:224
224       if (spatialbits + spatialfract != 8 + m_ucResidualBits)
(gdb) u 246
InverseToneMappingBox::InverseScaledTableOf (this=0x60d000000060, dctbits=12 '\f', spatialbits=13 '\r', dctfract=0 '\000', spatialfract=0 '\000') at inversetonemappingbox.cpp:246
246         m_plInverseMapping = (LONG *)m_pEnviron->AllocMem((1 << (spatialbits + spatialfract)) * sizeof(LONG));
(gdb) s
Environ::AllocMem (this=0x61b0000000d8, bytesize=32768) at environment.cpp:815
815       return CoreAllocMem(ULONG(bytesize),0);
(gdb) s
Environ::CoreAllocMem (this=0x61b0000000d8, bytesize=32768, reqments=0) at environment.cpp:643
643       if (bytesize == 0) {
(gdb) n
653         bytesize += 2 * sizeof(Align);
(gdb) n
656         if (m_pAllocationHook) {
(gdb) n
664           mem = malloc(bytesize);
(gdb) p bytesize
$3 = 32784

As the size of LONG is 4, bytesize / sizeof(LONG) = 8196, which is smaller than the aforementioned index *r = 13621.

Therefore, a heap-buffer-overflow (even SEGV when *r is big enough) occurs.

ASAN says

While checked with ASAN, this problem might trigger 3 different crashes (heap-buffer-overflow / SEGV / unknown-crash), depending on the extent of the array index out-of-bounds

==3473103==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62d00000d8e4 at pc 0x0000006f4d69 bp 0x7ffdbc043790 sp 0x7ffdbc043788
READ of size 4 at 0x62d00000d8e4 thread T0
    #0 0x6f4d68 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**) /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19
    #1 0x8ea24f in BlockBitmapRequester::EncodeUnsampled(RectAngle<int> const&, ColorTrafo*) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:888:17
    #2 0x8ec241 in BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:1006:5
    #3 0x8ec29d in non-virtual thunk to BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp
    #4 0x56666d in Image::EncodeRegion(BitMapHook*, RectangleRequest const*) /data/symccgo/bug/libjpeg/libjpeg/codestream/image.cpp:1159:21
    #5 0x547099 in JPEG::InternalProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:813:15
    #6 0x5463ec in JPEG::ProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:732:5
    #7 0x51a3bb in EncodeC(char const*, char const*, char const*, char const*, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, unsigned char, bool, bool, unsigned int, double, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, int, int, int, bool, bool, bool, int, bool, char const*, char const*, char const*, int, int, int, int, bool, int, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, char const*, char const*, char const*, char const*) /data/symccgo/bug/libjpeg/libjpeg/cmd/encodec.cpp:693:28
    #8 0x5022e9 in main /data/symccgo/bug/libjpeg/libjpeg/cmd/main.cpp:760:7
    #9 0x7fac04ae9082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:308:16
    #10 0x41c43d in _start (/data/symccgo/bug/libjpeg/obj-asan-dbg/jpeg+0x41c43d)

Address 0x62d00000d8e4 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**)
Shadow bytes around the buggy address:
  0x0c5a7fff9ac0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9ad0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9ae0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9af0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9b00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c5a7fff9b10: fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa fa fa
  0x0c5a7fff9b20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9b30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9b40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9b50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a7fff9b60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==3473103==ABORTING
=====
==3527212==ERROR: AddressSanitizer: SEGV on unknown address 0x63100003480c (pc 0x0000006f4d70 bp 0x7ffef664dc90 sp 0x7ffef664dac0 T0)
==3527212==The signal is caused by a READ memory access.
    #0 0x6f4d70 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**) /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19
    #1 0x8ea24f in BlockBitmapRequester::EncodeUnsampled(RectAngle<int> const&, ColorTrafo*) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:888:17
    #2 0x8ec241 in BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:1006:5
    #3 0x8ec29d in non-virtual thunk to BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp
    #4 0x56666d in Image::EncodeRegion(BitMapHook*, RectangleRequest const*) /data/symccgo/bug/libjpeg/libjpeg/codestream/image.cpp:1159:21
    #5 0x547099 in JPEG::InternalProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:813:15
    #6 0x5463ec in JPEG::ProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:732:5
    #7 0x51a3bb in EncodeC(char const*, char const*, char const*, char const*, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, unsigned char, bool, bool, unsigned int, double, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, int, int, int, bool, bool, bool, int, bool, char const*, char const*, char const*, int, int, int, int, bool, int, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, char const*, char const*, char const*, char const*) /data/symccgo/bug/libjpeg/libjpeg/cmd/encodec.cpp:693:28
    #8 0x5022e9 in main /data/symccgo/bug/libjpeg/libjpeg/cmd/main.cpp:760:7
    #9 0x7fc93cc3f082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:308:16
    #10 0x41c43d in _start (/data/symccgo/bug/libjpeg/obj-asan-dbg/jpeg+0x41c43d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**)
==3527212==ABORTING
=====
==3589630==ERROR: AddressSanitizer: unknown-crash on address 0x7f3aa9b854e8 at pc 0x0000006f4d69 bp 0x7ffe851b36d0 sp 0x7ffe851b36c8
READ of size 4 at 0x7f3aa9b854e8 thread T0
    #0 0x6f4d68 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**) /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19
    #1 0x8ea24f in BlockBitmapRequester::EncodeUnsampled(RectAngle<int> const&, ColorTrafo*) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:888:17
    #2 0x8ec241 in BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp:1006:5
    #3 0x8ec29d in non-virtual thunk to BlockBitmapRequester::EncodeRegion(RectAngle<int> const&) /data/symccgo/bug/libjpeg/libjpeg/control/blockbitmaprequester.cpp
    #4 0x56666d in Image::EncodeRegion(BitMapHook*, RectangleRequest const*) /data/symccgo/bug/libjpeg/libjpeg/codestream/image.cpp:1159:21
    #5 0x547099 in JPEG::InternalProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:813:15
    #6 0x5463ec in JPEG::ProvideImage(JPG_TagItem*) /data/symccgo/bug/libjpeg/libjpeg/interface/jpeg.cpp:732:5
    #7 0x51a3bb in EncodeC(char const*, char const*, char const*, char const*, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, bool, unsigned char, bool, bool, unsigned int, double, int, bool, bool, bool, bool, bool, bool, bool, bool, bool, int, int, int, bool, bool, bool, int, bool, char const*, char const*, char const*, int, int, int, int, bool, int, int, int, int, int, int, int, bool, bool, bool, bool, bool, bool, char const*, char const*, char const*, char const*) /data/symccgo/bug/libjpeg/libjpeg/cmd/encodec.cpp:693:28
    #8 0x5022e9 in main /data/symccgo/bug/libjpeg/libjpeg/cmd/main.cpp:760:7
    #9 0x7f3aacbbf082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:308:16
    #10 0x41c43d in _start (/data/symccgo/bug/libjpeg/obj-asan-dbg/jpeg+0x41c43d)

Address 0x7f3aa9b854e8 is a wild pointer.
SUMMARY: AddressSanitizer: unknown-crash /data/symccgo/bug/libjpeg/libjpeg/colortrafo/ycbcrtrafo.cpp:220:19 in YCbCrTrafo<unsigned short, 1, (unsigned char)65, 1, 0>::RGB2YCbCr(RectAngle<int> const&, ImageBitMap const* const*, int**)
Shadow bytes around the buggy address:
  0x0fe7d5368a40: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368a50: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368a60: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368a70: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368a80: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
=>0x0fe7d5368a90: fe fe fe fe fe fe fe fe fe fe fe fe fe[fe]fe fe
  0x0fe7d5368aa0: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368ab0: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368ac0: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368ad0: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fe7d5368ae0: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==3589630==ABORTING
=====

POC

poc-array-out-of-bounds.zip

thorfdbg commented 7 months ago

This is a garbadge-in garbadge-out problem. You provide an invalid ppm for encoding which contains elements are are not within the promised precision of the ppm.

thorfdbg commented 7 months ago

Won't fix. Missing input sanitation, this has to be done in the bitmap hook which is outside of the library, specifically cmd/bitmaphook.cpp if needed - this would add a single comparison per pixel which is at this time considered too expensive and not worth the hassle. Note that the decoder shall be robust, but the encoder operates on a garbadge-in garbadge-out basis.