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

Fix a 7zip crash and a ISO9660 infinite loop #1120

Merged
merged 2 commits into from Jan 20, 2019

Conversation

Projects
None yet
4 participants
@daxtens
Copy link
Contributor

daxtens commented Jan 16, 2019

Fuzzing found two further file-format specific issues - a read-only segfault in 7z, and an infinite loop in ISO9660. Full details are in the commit messages.

Test cases are (tested on Ubuntu bionic and git head):

In order to allow me to upload them to GitHub, I have converted them to text with xxd. To replicate:

xxd -r rrforever.iso.txt rrforever.iso
bsdtar -Oxf rrforever.iso

(and likewise for crash.7z)

Fuzzing was done with AFL, FairFuzz (afl-rb) and a little bit of qsym.

daxtens added some commits Jan 1, 2019

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.
iso9660: Fail when expected Rockridge extensions is missing
A corrupted or malicious ISO9660 image can cause read_CE() to loop
forever.

read_CE() calls parse_rockridge(), expecting a Rockridge extension
to be read. However, parse_rockridge() is structured as a while
loop starting with a sanity check, and if the sanity check fails
before the loop has run, the function returns ARCHIVE_OK without
advancing the position in the file. This causes read_CE() to retry
indefinitely.

Make parse_rockridge() return ARCHIVE_WARN if it didn't read an
extension. As someone with no real knowledge of the format, this
seems more apt than ARCHIVE_FATAL, but both the call-sites escalate
it to a fatal error immediately anyway.

Found with a combination of AFL, afl-rb (FairFuzz) and qsym.

@mmatuska mmatuska merged commit 45e6d54 into libarchive:master Jan 20, 2019

@daxtens

This comment has been minimized.

Copy link
Contributor Author

daxtens commented Jan 23, 2019

These have been assigned CVEs through the DWF project:

@lamby

This comment has been minimized.

Copy link

lamby commented on 65a23f5 Feb 7, 2019

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

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment