Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Huge memory allocation due to implicit cast from signed int to size_t #1237

Closed
rohanpadhye opened this issue Aug 24, 2019 · 5 comments

Comments

@rohanpadhye
Copy link

@rohanpadhye rohanpadhye commented Aug 24, 2019

Symptom: Unnecessary huge memory allocation (4GB) for tiny malformed file (21 bytes).
Affects: v3.4.0 and master.

Cause:
When decoding a malformed LZ4 input in legacy format having value 0xFBFFFFFF in compressed data field, libarchive allocates 4GB in a single malloc of __archive_read_filter_ahead().

This smells like a bug, because (1) int compressed in lz4_filter_read_legacy_stream is read as negative value -5, which causes (2) a bounds check to be deemed safe, and then (3) there is an implicit conversion of signed to unsigned integer to argument size_t min of __archive_read_filter_ahead, making the value of min to be 0xFFFFFF, leading to (4) an allocation of about 4GB.

I can attach an input here to repro if required. Please confirm if this is indeed a bug.

Found by: Fuzzing with FuzzFactory

@kientzle

This comment has been minimized.

Copy link
Contributor

@kientzle kientzle commented Aug 25, 2019

That does sound like a real bug. Having the input would be helpful in developing a fix.

@rohanpadhye

This comment has been minimized.

Copy link
Author

@rohanpadhye rohanpadhye commented Aug 25, 2019

Repro: huge.lz4.zip.

Unzip the file "huge.lz4" (21 bytes) from the attachment. Detailed repro with valgrind:

$ valgrind ./bsdtar -xOf huge.lz4
==29202== Memcheck, a memory error detector
==29202== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29202== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==29202== Command: ./bsdtar -xOf huge.lz4
==29202== 
==29202== Argument 'size' of function malloc has a fishy (possibly negative) value: -1
==29202==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29202==    by 0x12045A: __archive_read_filter_ahead.part.6 (archive_read.c:1450)
==29202==    by 0x128564: lz4_filter_read_legacy_stream (archive_read_support_filter_lz4.c:707)
==29202==    by 0x1202AC: __archive_read_filter_ahead.part.6 (archive_read.c:1384)
==29202==    by 0x126733: bzip2_reader_bid (archive_read_support_filter_bzip2.c:134)
==29202==    by 0x121C74: choose_filters (archive_read.c:574)
==29202==    by 0x121C74: archive_read_open1 (archive_read.c:514)
==29202==    by 0x126078: archive_read_open_filename (archive_read_open_filename.c:109)
==29202==    by 0x1154E3: read_archive (read.c:222)
==29202==    by 0x115DA6: tar_mode_x (read.c:112)
==29202==    by 0x1143DC: main (bsdtar.c:919)
bsdtar: Error opening archive: truncated lz4 input
==29202== 
==29202== HEAP SUMMARY:
==29202==     in use at exit: 161,264 bytes in 26 blocks
==29202==   total heap usage: 61 allocs, 35 frees, 8,692,629 bytes allocated
@kientzle

This comment has been minimized.

Copy link
Contributor

@kientzle kientzle commented Aug 25, 2019

Looking more carefully at the code, I see two obvious issues:

  • The compressed variable should be unsigned, not signed
  • We need to ensure that compressed + 4 cannot overflow an unsigned 32-bit integer (we need to verify that compressed is less than 0xfffffffc)

Of course, the above changes would not prevent us trying to allocate ~4GB, since the test file here does actually specify that the following block is that big. I suspect that we should also verify that compressed is less than LZ4_MAX_INPUT_SIZE, but I'm not familiar enough with liblz4 to be sure that's correct.

@mmatuska

This comment has been minimized.

Copy link
Member

@mmatuska mmatuska commented Sep 1, 2019

As of the types we are calling archive_le32dec() so compressed should be a uint32_t. Then we should check that compressed + 4 is not larger than SIZE_MAX.

@mmatuska

This comment has been minimized.

Copy link
Member

@mmatuska mmatuska commented Sep 1, 2019

Oh the check against LZ4_MAX_INPUT_SIZE is actually done right here:

if (compressed > LZ4_COMPRESSBOUND(LEGACY_BLOCK_SIZE)) {               
                state->stage = SELECT_STREAM;                                  
                return 0;
}
mmatuska added a commit to mmatuska/libarchive that referenced this issue Sep 3, 2019
Partially fixes libarchive#1237
@mmatuska mmatuska closed this in 57c8ea9 Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.