Merge pull request #3091 from yPin9/fix/rar5-consume-remaining

rar5: skip unconsumed block bytes before ARCHIVE_RETRY

Makefile.am

@@ -985,7 +985,9 @@ libarchive_test_EXTRA_DIST=\
 	libarchive/test/test_read_format_rar4_encrypted_filenames.rar.uu \
 	libarchive/test/test_read_format_rar4_solid_encrypted.rar.uu \
 	libarchive/test/test_read_format_rar4_solid_encrypted_filenames.rar.uu \
+	libarchive/test/test_read_format_rar5_main_block_extra_bytes.rar.uu \
 	libarchive/test/test_read_format_rar5_only_crypt_exfld.rar.uu \
+	libarchive/test/test_read_format_rar5_skip_block_extra_bytes.rar.uu \
 	libarchive/test/test_read_format_rar5_unsupported_exfld.rar.uu \
 	libarchive/test/test_read_format_rar5_invalid_hash_valid_htime_exfld.rar.uu \
 	libarchive/test/test_read_format_rar5_encrypted.rar.uu \

libarchive/archive_read_support_format_rar5.c

@@ -2274,6 +2274,33 @@ static int scan_for_signature(struct archive_read* a);
  * <FILE> block.
  */
 
+/*
+ * A header that carries no file data (HEAD_MAIN, or an unknown block
+ * flagged HFL_SKIP_IF_UNKNOWN) may leave bytes in its body that the
+ * sub-parser did not read. Skip them before returning ARCHIVE_RETRY,
+ * otherwise rar5_read_header() re-parses the same block region O(N)
+ * times instead of O(1), letting a crafted RAR5 file stall the reader
+ * (GHSA-9h2c-464f-j3hj).
+ *
+ * Safe because read_ahead(a, hdr_size, &p) pre-loaded the whole block
+ * into one contiguous buffer with no compaction until we return, so
+ * body_start stays valid and (cur - body_start) is the exact number of
+ * body bytes consumed so far.
+ */
+static void
+rar5_skip_remaining_block(struct archive_read* a,
+    const uint8_t* body_start, size_t raw_hdr_size)
+{
+	const uint8_t* cur;
+
+	if(read_ahead(a, 1, &cur)) {
+		size_t body_used = (size_t)(cur - body_start);
+
+		if(body_used < raw_hdr_size)
+			(void)consume(a, raw_hdr_size - body_used);
+	}
+}
+
 static int process_base_block(struct archive_read* a,
     struct archive_entry* entry)
 {
@@ -2285,6 +2312,7 @@ static int process_base_block(struct archive_read* a,
 	size_t header_id = 0;
 	size_t header_flags = 0;
 	const uint8_t* p;
+	const uint8_t* body_start;
 	int ret;
 
 	enum HEADER_TYPE {
@@ -2346,6 +2374,10 @@ static int process_base_block(struct archive_read* a,
 #endif
 	}
 
+	/* Remember the first byte of the block body so we can later skip
+	 * any bytes the sub-parser leaves unconsumed. */
+	body_start = p + hdr_size_len;
+
 	/* If the checksum is OK, we proceed with parsing. */
 	if(ARCHIVE_OK != consume(a, hdr_size_len)) {
 		return ARCHIVE_EOF;
@@ -2371,8 +2403,11 @@ static int process_base_block(struct archive_read* a,
 			/* Main header doesn't have any files in it, so it's
 			 * pointless to return to the caller. Retry to next
 			 * header, which should be HEAD_FILE/HEAD_SERVICE. */
-			if(ret == ARCHIVE_OK)
+			if(ret == ARCHIVE_OK) {
+				rar5_skip_remaining_block(a, body_start,
+				    raw_hdr_size);
 				return ARCHIVE_RETRY;
+			}
 
 			return ret;
 		case HEAD_SERVICE:
@@ -2432,6 +2467,8 @@ static int process_base_block(struct archive_read* a,
 				/* If the block is marked as 'skip if unknown',
 				 * do as the flag says: skip the block
 				 * instead on failing on it. */
+				rar5_skip_remaining_block(a, body_start,
+				    raw_hdr_size);
 				return ARCHIVE_RETRY;
 			}
 	}

libarchive/test/test_read_format_rar5.c

@@ -1494,3 +1494,52 @@ DEFINE_TEST(test_read_format_rar5_invalidhash_and_validhtime_exfld)
 
 	EPILOGUE();
 }
+
+/*
+ * Regression tests for the RAR5 base-block parser leaving unconsumed body
+ * bytes before returning ARCHIVE_RETRY (GHSA-9h2c-464f-j3hj). Each archive is
+ * the test_read_format_rar5_stored archive with extra, unread bytes appended
+ * to a no-data block's body. Before the fix the reader did not skip those
+ * bytes, so the stream misaligned and the following file entry was lost; with
+ * the fix the trailing bytes are skipped and helloworld.txt is read normally.
+ */
+DEFINE_TEST(test_read_format_rar5_main_block_extra_bytes)
+{
+	const char helloworld_txt[] = "hello libarchive test suite!\n";
+	la_ssize_t file_size = sizeof(helloworld_txt) - 1;
+	char buff[64];
+
+	/* HEAD_MAIN block padded with trailing bytes the parser does not read. */
+	PROLOGUE("test_read_format_rar5_main_block_extra_bytes.rar");
+
+	assertA(0 == archive_read_next_header(a, &ae));
+	assertEqualString("helloworld.txt", archive_entry_pathname(ae));
+	assertEqualInt(file_size, archive_entry_size(ae));
+	assertA(file_size == archive_read_data(a, buff, file_size));
+	assertEqualMem(buff, helloworld_txt, file_size);
+
+	assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+
+	EPILOGUE();
+}
+
+DEFINE_TEST(test_read_format_rar5_skip_block_extra_bytes)
+{
+	const char helloworld_txt[] = "hello libarchive test suite!\n";
+	la_ssize_t file_size = sizeof(helloworld_txt) - 1;
+	char buff[64];
+
+	/* An unknown HFL_SKIP_IF_UNKNOWN block carrying trailing bytes is inserted
+	 * before the file entry; the parser must skip the entire block. */
+	PROLOGUE("test_read_format_rar5_skip_block_extra_bytes.rar");
+
+	assertA(0 == archive_read_next_header(a, &ae));
+	assertEqualString("helloworld.txt", archive_entry_pathname(ae));
+	assertEqualInt(file_size, archive_entry_size(ae));
+	assertA(file_size == archive_read_data(a, buff, file_size));
+	assertEqualMem(buff, helloworld_txt, file_size);
+
+	assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+
+	EPILOGUE();
+}

libarchive/test/test_read_format_rar5_main_block_extra_bytes.rar.uu

@@ -0,0 +1,6 @@
+begin 644 test_read_format_rar5_main_block_extra_bytes.rar
+M4F%R(1H' 0 $ .>F$@$%!@ % 0& @            #@P!F,L @,+G0 $G0"D
+M@P*T0Z"5@  !#FAE;&QO=V]R;&0N='AT"@,3?@ZK6U;I#AIH96QL;R!L:6)A
+;<F-H:79E('1E<W0@<W5I=&4A"AUW5E$#!00 
+`
+end

libarchive/test/test_read_format_rar5_skip_block_extra_bytes.rar.uu

@@ -0,0 +1,6 @@
+begin 644 test_read_format_rar5_skip_block_extra_bytes.rar
+M4F%R(1H' 0 SDK7E"@$%!@ % 0& @ ",.6KS"A $           X, 9C+ (#
+M"YT !)T I(,"M$.@E8   0YH96QL;W=O<FQD+G1X= H#$WX.JUM6Z0X::&5L
+B;&\@;&EB87)C:&EV92!T97-T('-U:71E(0H==U91 P4$    
+`
+end