Permalink
Browse files

7zip: fix crash when parsing certain archives

Fuzzing with CRCs disabled revealed that a call to get_uncompressed_data()
would sometimes fail to return at least 'minimum' bytes. This can cause
the crc32() invocation in header_bytes to read off into invalid memory.

A specially crafted archive can use this to cause a crash.

An ASAN trace is below, but ASAN is not required - an uninstrumented
binary will also crash.

==7719==ERROR: AddressSanitizer: SEGV on unknown address 0x631000040000 (pc 0x7fbdb3b3ec1d bp 0x7ffe77a51310 sp 0x7ffe77a51150 T0)
==7719==The signal is caused by a READ memory access.
    #0 0x7fbdb3b3ec1c in crc32_z (/lib/x86_64-linux-gnu/libz.so.1+0x2c1c)
    #1 0x84f5eb in header_bytes (/tmp/libarchive/bsdtar+0x84f5eb)
    #2 0x856156 in read_Header (/tmp/libarchive/bsdtar+0x856156)
    #3 0x84e134 in slurp_central_directory (/tmp/libarchive/bsdtar+0x84e134)
    #4 0x849690 in archive_read_format_7zip_read_header (/tmp/libarchive/bsdtar+0x849690)
    #5 0x5713b7 in _archive_read_next_header2 (/tmp/libarchive/bsdtar+0x5713b7)
    #6 0x570e63 in _archive_read_next_header (/tmp/libarchive/bsdtar+0x570e63)
    #7 0x6f08bd in archive_read_next_header (/tmp/libarchive/bsdtar+0x6f08bd)
    #8 0x52373f in read_archive (/tmp/libarchive/bsdtar+0x52373f)
    #9 0x5257be in tar_mode_x (/tmp/libarchive/bsdtar+0x5257be)
    #10 0x51daeb in main (/tmp/libarchive/bsdtar+0x51daeb)
    #11 0x7fbdb27cab96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #12 0x41dd09 in _start (/tmp/libarchive/bsdtar+0x41dd09)

This was primarly done with afl and FairFuzz. Some early corpus entries
may have been generated by qsym.
  • Loading branch information...
daxtens committed Jan 1, 2019
1 parent 8312eaa commit 65a23f5dbee4497064e9bb467f81138a62b0dae1
Showing with 1 addition and 7 deletions.
  1. +1 −7 libarchive/archive_read_support_format_7zip.c
@@ -2964,13 +2964,7 @@ get_uncompressed_data(struct archive_read *a, const void **buff, size_t size,
if (zip->codec == _7Z_COPY && zip->codec2 == (unsigned long)-1) {
/* Copy mode. */

/*
* Note: '1' here is a performance optimization.
* Recall that the decompression layer returns a count of
* available bytes; asking for more than that forces the
* decompressor to combine reads by copying data.
*/
*buff = __archive_read_ahead(a, 1, &bytes_avail);
*buff = __archive_read_ahead(a, minimum, &bytes_avail);
if (bytes_avail <= 0) {
archive_set_error(&a->archive,
ARCHIVE_ERRNO_FILE_FORMAT,

2 comments on commit 65a23f5

@lamby

This comment has been minimized.

Copy link

lamby replied Feb 7, 2019

There's an identical "performance optimization" later in the same file; also vulnerable?

@anarcat

This comment has been minimized.

Copy link

anarcat replied Feb 7, 2019

I'm not sure. I'm not deeply familiar with the libarchive code, but it seems to me the optmization sit in two very different code locations: the above is in a if block that gets called only once in get_uncompressed_data and therefore might not return the requested minimum bytes. the other optimization, however, is in extract_pack_stream which is structured differently: it lives in a for (;;) loop that repeatedly calls the optimized code. before the function returns, there's this extra check as well:

	if (zip->uncompressed_buffer_bytes_remaining < minimum) {
		archive_set_error(&(a->archive),
		    ARCHIVE_ERRNO_MISC, "Damaged 7-Zip archive");
		return (ARCHIVE_FATAL);
	}

I also note that the function starts with a potentially similar code block as the vulnerable one, but without the optimization:

	if (zip->codec == _7Z_COPY && zip->codec2 == (unsigned long)-1) {
		if (minimum == 0)
			minimum = 1;
		if (__archive_read_ahead(a, minimum, &bytes_avail) == NULL
		    || bytes_avail <= 0) {
			archive_set_error(&a->archive,
			    ARCHIVE_ERRNO_FILE_FORMAT,
			    "Truncated 7-Zip file body");
			return (ARCHIVE_FATAL);
		}
		if (bytes_avail > (ssize_t)zip->pack_stream_inbytes_remaining)
			bytes_avail = (ssize_t)zip->pack_stream_inbytes_remaining;
		zip->pack_stream_inbytes_remaining -= bytes_avail;
		if (bytes_avail > (ssize_t)zip->folder_outbytes_remaining)
			bytes_avail = (ssize_t)zip->folder_outbytes_remaining;
		zip->folder_outbytes_remaining -= bytes_avail;
		zip->uncompressed_buffer_bytes_remaining = bytes_avail;
		return (ARCHIVE_OK);
	}

so I don't believe the code is vulnerable after patch. I can also confirm the reproducer stops working after applying the patch:

vagrant@jessie:~$ bsdtar -Oxf crash.7z
bsdtar: Damaged 7-Zip archive
bsdtar: Error exit delayed from previous errors.

So I'll go under the assumption this is properly fixed. But really, only formal proof (in C? ;), time and fuzzing will tell so don't quote me on that...

Please sign in to comment.