From ed84daf889902c472db860f44cdf506d956f1fb4 Mon Sep 17 00:00:00 2001 From: Tim Kientzle Date: Mon, 13 Oct 2025 10:58:26 -0700 Subject: [PATCH] Merge pull request #2737 from kientzle/kientzle-volume-header-overflow Fix an infinite loop when parsing `V` headers (cherry picked from commit de73860cda5a49f97289a9924a3c5590edafef66) --- Makefile.am | 2 + libarchive/archive_read_support_format_tar.c | 60 ++++++++++++++----- libarchive/test/CMakeLists.txt | 1 + .../test_read_format_tar_V_negative_size.c | 48 +++++++++++++++ ...est_read_format_tar_V_negative_size.tar.uu | 20 +++++++ 5 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 libarchive/test/test_read_format_tar_V_negative_size.c create mode 100644 libarchive/test/test_read_format_tar_V_negative_size.tar.uu diff --git a/Makefile.am b/Makefile.am index b99c4afbb..2827615ad 100644 --- a/Makefile.am +++ b/Makefile.am @@ -526,6 +526,7 @@ libarchive_test_SOURCES= \ libarchive/test/test_read_format_rar5.c \ libarchive/test/test_read_format_raw.c \ libarchive/test/test_read_format_tar.c \ + libarchive/test/test_read_format_tar_V_negative_size.c \ libarchive/test/test_read_format_tar_concatenated.c \ libarchive/test/test_read_format_tar_empty_pax.c \ libarchive/test/test_read_format_tar_empty_filename.c \ @@ -977,6 +978,7 @@ libarchive_test_EXTRA_DIST=\ libarchive/test/test_read_format_raw.data.gz.uu \ libarchive/test/test_read_format_raw.data.Z.uu \ libarchive/test/test_read_format_raw.data.uu \ + libarchive/test/test_read_format_tar_V_negative_size.tar.uu \ libarchive/test/test_read_format_tar_concatenated.tar.uu \ libarchive/test/test_read_format_tar_empty_filename.tar.uu \ libarchive/test/test_read_format_tar_empty_with_gnulabel.tar.uu \ diff --git a/libarchive/archive_read_support_format_tar.c b/libarchive/archive_read_support_format_tar.c index 37fdd2ce7..eeb2c725f 100644 --- a/libarchive/archive_read_support_format_tar.c +++ b/libarchive/archive_read_support_format_tar.c @@ -233,7 +233,7 @@ static int tar_read_header(struct archive_read *, struct tar *, struct archive_entry *, int64_t *); static int tohex(int c); static char *url_decode(const char *, size_t); -static void tar_flush_unconsumed(struct archive_read *, int64_t *); +static int tar_flush_unconsumed(struct archive_read *, int64_t *); /* Sanity limits: These numbers should be low enough to * prevent a maliciously-crafted archive from forcing us to @@ -477,7 +477,7 @@ archive_read_format_tar_options(struct archive_read *a, * how much unconsumed data we have floating around, and to consume * anything outstanding since we're going to do read_aheads */ -static void +static int tar_flush_unconsumed(struct archive_read *a, int64_t *unconsumed) { if (*unconsumed) { @@ -490,9 +490,13 @@ tar_flush_unconsumed(struct archive_read *a, int64_t *unconsumed) memset(data, 0xff, *unconsumed); } */ - __archive_read_consume(a, *unconsumed); + int64_t consumed = __archive_read_consume(a, *unconsumed); + if (consumed != *unconsumed) { + return (ARCHIVE_FATAL); + } *unconsumed = 0; } + return (ARCHIVE_OK); } /* @@ -750,7 +754,9 @@ tar_read_header(struct archive_read *a, struct tar *tar, /* Find the next valid header record. */ while (1) { - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } /* Read 512-byte header record */ h = __archive_read_ahead(a, 512, &bytes); @@ -796,7 +802,9 @@ tar_read_header(struct archive_read *a, struct tar *tar, /* This is NOT a null block, so it must be a valid header. */ if (!checksum(a, h)) { - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } archive_set_error(&a->archive, EINVAL, "Damaged tar archive (bad header checksum)"); /* If we've read some critical information (pax headers, etc) @@ -1236,7 +1244,7 @@ header_volume(struct archive_read *a, struct tar *tar, header = (const struct archive_entry_header_ustar *)h; size = tar_atol(header->size, sizeof(header->size)); - if (size > (int64_t)pathname_limit) { + if (size < 0 || size > (int64_t)pathname_limit) { return (ARCHIVE_FATAL); } to_consume = ((size + 511) & ~511); @@ -1261,7 +1269,9 @@ read_bytes_to_string(struct archive_read *a, return (ARCHIVE_FATAL); } - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } /* Read the body into the string. */ src = __archive_read_ahead(a, size, NULL); @@ -1715,7 +1725,9 @@ read_mac_metadata_blob(struct archive_read *a, * Q: Is the above idea really possible? Even * when there are GNU or pax extension entries? */ - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } data = __archive_read_ahead(a, msize, NULL); if (data == NULL) { archive_set_error(&a->archive, EINVAL, @@ -1900,7 +1912,9 @@ header_pax_extension(struct archive_read *a, struct tar *tar, (long long)ext_size, (long long)ext_size_limit); return (ARCHIVE_WARN); } - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } /* Parse the size/name of each pax attribute in the body */ archive_string_init(&attr_name); @@ -1994,7 +2008,9 @@ header_pax_extension(struct archive_read *a, struct tar *tar, /* Consume size, name, and `=` */ *unconsumed += p - attr_start; - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } if (value_length == 0) { archive_set_error(&a->archive, EINVAL, @@ -2017,7 +2033,9 @@ header_pax_extension(struct archive_read *a, struct tar *tar, err = err_combine(err, r); /* Consume the `\n` that follows the pax attribute value. */ - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } p = __archive_read_ahead(a, 1, &did_read); if (p == NULL) { archive_set_error(&a->archive, EINVAL, @@ -2033,7 +2051,9 @@ header_pax_extension(struct archive_read *a, struct tar *tar, } ext_size -= 1; *unconsumed += 1; - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } } *unconsumed += ext_size + ext_padding; @@ -2290,7 +2310,9 @@ pax_attribute_read_number(struct archive_read *a, size_t value_length, int64_t * archive_string_init(&as); r = read_bytes_to_string(a, &as, value_length, &unconsumed); - tar_flush_unconsumed(a, &unconsumed); + if (tar_flush_unconsumed(a, &unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } if (r < ARCHIVE_OK) { archive_string_free(&as); *result = 0; @@ -3095,7 +3117,9 @@ gnu_sparse_old_read(struct archive_read *a, struct tar *tar, return (ARCHIVE_OK); do { - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } data = __archive_read_ahead(a, 512, &bytes_read); if (data == NULL) { archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, @@ -3285,7 +3309,9 @@ gnu_sparse_10_read(struct archive_read *a, struct tar *tar, int64_t *unconsumed) return (ARCHIVE_FATAL); } /* Skip rest of block... */ - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } bytes_read = tar->entry_bytes_remaining - remaining; to_skip = 0x1ff & -bytes_read; /* Fail if tar->entry_bytes_remaing would get negative */ @@ -3511,7 +3537,9 @@ readline(struct archive_read *a, struct tar *tar, const char **start, const char *s; void *p; - tar_flush_unconsumed(a, unconsumed); + if (tar_flush_unconsumed(a, unconsumed) != ARCHIVE_OK) { + return (ARCHIVE_FATAL); + } t = __archive_read_ahead(a, 1, &bytes_read); if (bytes_read <= 0 || t == NULL) diff --git a/libarchive/test/CMakeLists.txt b/libarchive/test/CMakeLists.txt index b5acb468c..f62737d79 100644 --- a/libarchive/test/CMakeLists.txt +++ b/libarchive/test/CMakeLists.txt @@ -168,6 +168,7 @@ IF(ENABLE_TEST) test_read_format_rar5.c test_read_format_raw.c test_read_format_tar.c + test_read_format_tar_V_negative_size.c test_read_format_tar_concatenated.c test_read_format_tar_empty_filename.c test_read_format_tar_empty_with_gnulabel.c diff --git a/libarchive/test/test_read_format_tar_V_negative_size.c b/libarchive/test/test_read_format_tar_V_negative_size.c new file mode 100644 index 000000000..d110553ac --- /dev/null +++ b/libarchive/test/test_read_format_tar_V_negative_size.c @@ -0,0 +1,48 @@ +/*- + * Copyright (c) 2025 Tim Kientzle + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. + * IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +#include "test.h" + +DEFINE_TEST(test_read_format_tar_V_negative_size) +{ + /* + * An archive that contains a `V` volume header with a negative body size + * + * This used to lead to an infinite loop: the tar reader would "advance" + * by the size of the body to skip it, which would in this case end up + * reversing back to the beginning of the same header. + */ + struct archive_entry *ae; + struct archive *a; + const char *refname = "test_read_format_tar_V_negative_size.tar"; + + extract_reference_file(refname); + assert((a = archive_read_new()) != NULL); + assertEqualInt(ARCHIVE_OK, archive_read_support_filter_all(a)); + assertEqualInt(ARCHIVE_OK, archive_read_support_format_all(a)); + assertEqualIntA(a, ARCHIVE_OK, archive_read_open_filename(a, refname, 10240)); + assertEqualIntA(a, ARCHIVE_FATAL, archive_read_next_header(a, &ae)); + assertEqualIntA(a, ARCHIVE_OK, archive_read_close(a)); + assertEqualInt(ARCHIVE_OK, archive_read_free(a)); +} diff --git a/libarchive/test/test_read_format_tar_V_negative_size.tar.uu b/libarchive/test/test_read_format_tar_V_negative_size.tar.uu new file mode 100644 index 000000000..230d710c8 --- /dev/null +++ b/libarchive/test/test_read_format_tar_V_negative_size.tar.uu @@ -0,0 +1,20 @@ +Tar archive with a single `V` header that has a negative size. +This used to result in an infinite loop -- the tar reader would +"advance" by the size of the header, which in this case just backed +up to re-read the same header again. + +begin 644 test_read_format_tar_V_negative_size.tar +M`#(VXP```````````````````-Z;@"E&LOX^\@````````````!7````'``` +M```````````````````````````````````````````````````````````` +M``````````````````````````````````````````#___\````````@```` +M````````````````````````````5H%ZL#X]SH\-``":SN#[C4;Z5OOW-&'] +M?HHQ%WRG?Z$Q>^E#_1.OY96VEI*Z