Merge pull request #2737 from kientzle/kientzle-volume-header-overflow

Fix an infinite loop when parsing `V` headers

(cherry picked from commit de73860cda)

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 \

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)

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

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));
+}

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<U[)$TR502_;F$;9FU"/F'!`V:0`````
+M`````````````````````````````````````````````````````0``````
+M````````````````````````````````````````````````````````````
+M`````````````````````````````0``````````````````````````````
+M````````````````````````````````````````````````````````````
+M````````````````````````````````````````````````````````````
+M````````````````````````````````````````````````````````````
+1````````````````````````
+`
+end