From: Tim Kientzle <kientzle@acm.org>
Date: Fri, 5 Jul 2024 10:08:38 +0000 (-0700)
Subject: Ignore out-of-range gid/uid/size/ino and harden AFIO parsing (#2258)
X-Git-Tag: v3.7.5~24
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1382d671a4bc32155cbb52a60cf9937120f56028;p=thirdparty%2Flibarchive.git

Ignore out-of-range gid/uid/size/ino and harden AFIO parsing (#2258)

The fuzzer constructed an AFIO (CPIO variant) archive that had a
rediculously large ino value, which caused an overflow of a signed
64-bit intermediate.

There are really three issues here:
* The CPIO parser was using a signed int64 as an intermediate type for
parsing numbers in all cases. I've addressed the overflow here by using
a uint64_t in the parser core, but left the resulting values as int64_t.
* The AFIO header parsing had no guards against rediculously large
values; it now rejects an archive when the ino or size fields (which are
allowed to be up to 16 hex digits long) overflow int64_t to produce a
negative value.
* The archive_entry would accept negative values for gid/uid/size/ino.
I've altered those so that these fields treat any negative value as zero
for these fields.

There was one test that actually verified that we could read a field
with size = -1. I've updated that to verify that the resulting size is
zero instead.

OSS-Fuzz Issue: 70019
---

diff --git a/libarchive/archive_entry.c b/libarchive/archive_entry.c
index ef322341a..f68fee65d 100644
--- a/libarchive/archive_entry.c
+++ b/libarchive/archive_entry.c
@@ -930,6 +930,9 @@ archive_entry_copy_fflags_text_w(struct archive_entry *entry,
 void
 archive_entry_set_gid(struct archive_entry *entry, la_int64_t g)
 {
+	if (g < 0) {
+		g = 0;
+	}
 	entry->stat_valid = 0;
 	entry->ae_stat.aest_gid = g;
 	entry->ae_set |= AE_SET_GID;
@@ -980,6 +983,9 @@ _archive_entry_copy_gname_l(struct archive_entry *entry,
 void
 archive_entry_set_ino(struct archive_entry *entry, la_int64_t ino)
 {
+	if (ino < 0) {
+		ino = 0;
+	}
 	entry->stat_valid = 0;
 	entry->ae_set |= AE_SET_INO;
 	entry->ae_stat.aest_ino = ino;
@@ -988,6 +994,9 @@ archive_entry_set_ino(struct archive_entry *entry, la_int64_t ino)
 void
 archive_entry_set_ino64(struct archive_entry *entry, la_int64_t ino)
 {
+	if (ino < 0) {
+		ino = 0;
+	}
 	entry->stat_valid = 0;
 	entry->ae_set |= AE_SET_INO;
 	entry->ae_stat.aest_ino = ino;
@@ -1343,6 +1352,9 @@ archive_entry_set_rdevminor(struct archive_entry *entry, dev_t m)
 void
 archive_entry_set_size(struct archive_entry *entry, la_int64_t s)
 {
+	if (s < 0) {
+		s = 0;
+	}
 	entry->stat_valid = 0;
 	entry->ae_stat.aest_size = s;
 	entry->ae_set |= AE_SET_SIZE;
@@ -1464,6 +1476,9 @@ _archive_entry_copy_symlink_l(struct archive_entry *entry,
 void
 archive_entry_set_uid(struct archive_entry *entry, la_int64_t u)
 {
+	if (u < 0) {
+		u = 0;
+	}
 	entry->stat_valid = 0;
 	entry->ae_stat.aest_uid = u;
 	entry->ae_set |= AE_SET_UID;
diff --git a/libarchive/archive_read_support_format_cpio.c b/libarchive/archive_read_support_format_cpio.c
index dcff23f69..69752cbb0 100644
--- a/libarchive/archive_read_support_format_cpio.c
+++ b/libarchive/archive_read_support_format_cpio.c
@@ -834,6 +834,7 @@ static int
 header_afiol(struct archive_read *a, struct cpio *cpio,
     struct archive_entry *entry, size_t *namelength, size_t *name_pad)
 {
+	int64_t t;
 	const void *h;
 	const char *header;
 
@@ -850,7 +851,12 @@ header_afiol(struct archive_read *a, struct cpio *cpio,
 
 	archive_entry_set_dev(entry, 
 		(dev_t)atol16(header + afiol_dev_offset, afiol_dev_size));
-	archive_entry_set_ino(entry, atol16(header + afiol_ino_offset, afiol_ino_size));
+	t = atol16(header + afiol_ino_offset, afiol_ino_size);
+	if (t < 0) {
+		archive_set_error(&a->archive, 0, "Nonsensical ino value");
+		return (ARCHIVE_FATAL);
+	}
+	archive_entry_set_ino(entry, t);
 	archive_entry_set_mode(entry,
 		(mode_t)atol8(header + afiol_mode_offset, afiol_mode_size));
 	archive_entry_set_uid(entry, atol16(header + afiol_uid_offset, afiol_uid_size));
@@ -863,8 +869,12 @@ header_afiol(struct archive_read *a, struct cpio *cpio,
 	*namelength = (size_t)atol16(header + afiol_namesize_offset, afiol_namesize_size);
 	*name_pad = 0; /* No padding of filename. */
 
-	cpio->entry_bytes_remaining =
-	    atol16(header + afiol_filesize_offset, afiol_filesize_size);
+	t = atol16(header + afiol_filesize_offset, afiol_filesize_size);
+	if (t < 0) {
+		archive_set_error(&a->archive, 0, "Nonsensical file size");
+		return (ARCHIVE_FATAL);
+	}
+	cpio->entry_bytes_remaining = t;
 	archive_entry_set_size(entry, cpio->entry_bytes_remaining);
 	cpio->entry_padding = 0;
 	__archive_read_consume(a, afiol_header_size);
@@ -1002,7 +1012,7 @@ be4(const unsigned char *p)
 static int64_t
 atol8(const char *p, unsigned char_cnt)
 {
-	int64_t l;
+	uint64_t l;
 	int digit;
 
 	l = 0;
@@ -1010,18 +1020,18 @@ atol8(const char *p, unsigned char_cnt)
 		if (*p >= '0' && *p <= '7')
 			digit = *p - '0';
 		else
-			return (l);
+			return ((int64_t)l);
 		p++;
 		l <<= 3;
 		l |= digit;
 	}
-	return (l);
+	return ((int64_t)l);
 }
 
 static int64_t
 atol16(const char *p, unsigned char_cnt)
 {
-	int64_t l;
+	uint64_t l;
 	int digit;
 
 	l = 0;
@@ -1033,12 +1043,12 @@ atol16(const char *p, unsigned char_cnt)
 		else if (*p >= '0' && *p <= '9')
 			digit = *p - '0';
 		else
-			return (l);
+			return ((int64_t)l);
 		p++;
 		l <<= 4;
 		l |= digit;
 	}
-	return (l);
+	return ((int64_t)l);
 }
 
 static int
diff --git a/libarchive/test/test_read_format_cpio_afio.c b/libarchive/test/test_read_format_cpio_afio.c
index 0eff8cfee..5d2b4b4c8 100644
--- a/libarchive/test/test_read_format_cpio_afio.c
+++ b/libarchive/test/test_read_format_cpio_afio.c
@@ -65,15 +65,6 @@ static unsigned char archive[] = {
 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
 };
 
-/*
- * XXX This must be removed when we use int64_t for uid.
- */
-static int
-uid_size(void)
-{
-	return (sizeof(uid_t));
-}
-
 DEFINE_TEST(test_read_format_cpio_afio)
 {
 	unsigned char *p;
@@ -106,8 +97,7 @@ DEFINE_TEST(test_read_format_cpio_afio)
 	 */
 	assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
 	assertEqualInt(17, archive_entry_size(ae));
-	if (uid_size() > 4)
-		assertEqualInt(65536, archive_entry_uid(ae));
+	assertEqualInt(65536, archive_entry_uid(ae));
 	assertEqualInt(archive_entry_is_encrypted(ae), 0);
 	assertEqualIntA(a, archive_read_has_encrypted_entries(a), ARCHIVE_READ_FORMAT_ENCRYPTION_UNSUPPORTED);
 	assertA(archive_filter_code(a, 0) == ARCHIVE_FILTER_NONE);
@@ -117,3 +107,21 @@ DEFINE_TEST(test_read_format_cpio_afio)
 
 	free(p);
 }
+
+// From OSS Fuzz Issue 70019:
+static unsigned char archive2[] = "070727bbbBbbbBabbbbbbcbcbbbbbbm726f777f777ffffffff518402ffffbbbabDDDDDDDDD7c7Ddd7DDDDnDDDdDDDB7777s77777777777C7727:";
+
+DEFINE_TEST(test_read_format_cpio_afio_broken)
+{
+	struct archive *a;
+	struct archive_entry *ae;
+
+	assert((a = archive_read_new()) != NULL);
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_support_filter_all(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_open_memory(a, archive2, sizeof(archive2)));
+	assertEqualIntA(a, ARCHIVE_FATAL, archive_read_next_header(a, &ae));
+	assertEqualInt(archive_filter_code(a, 0), ARCHIVE_FILTER_NONE);
+	assertEqualInt(archive_format(a), ARCHIVE_FORMAT_CPIO_AFIO_LARGE);
+	archive_read_free(a);
+}
diff --git a/libarchive/test/test_read_format_mtree.c b/libarchive/test/test_read_format_mtree.c
index 707308393..b4463298d 100644
--- a/libarchive/test/test_read_format_mtree.c
+++ b/libarchive/test/test_read_format_mtree.c
@@ -158,7 +158,7 @@ test_read_format_mtree1(void)
 	/* TODO: Mtree reader should probably return ARCHIVE_WARN for this. */
 	assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
 	assertEqualString(archive_entry_pathname(ae), "dir2/toosmallfile");
-	assertEqualInt(archive_entry_size(ae), -1);
+	assertEqualInt(archive_entry_size(ae), 0);
 	assertEqualInt(archive_entry_is_encrypted(ae), 0);
 	assertEqualIntA(a, archive_read_has_encrypted_entries(a), ARCHIVE_READ_FORMAT_ENCRYPTION_UNSUPPORTED);