From cb403667d721a10f401c145742e2f7e051867003 Mon Sep 17 00:00:00 2001 From: Grzegorz Antoniak Date: Mon, 4 Aug 2025 15:07:32 +0200 Subject: [PATCH] RAR5 reader: early fail when file declares data for a dir entry RAR5 reader had inconsistent sanity checks for directory entries that declare data. On one hand such declaration was accepted during the header parsing process, but at the same time this was disallowed during the data reading process. Disallow logic was returning the ARCHIVE_FAILED error code that allowed the client to retry, while in reality, the error was non-retryable. This commit adds another sanity check during the header parsing logic that disallows the directory entries to declare any data. This will make clients fail early when such entry is detected. Also, the commit changes the ARCHIVE_FAILED error code to ARCHIVE_FATAL when trying to read data for the directory entry that declares data. This makes sure that tools like bsdtar won't attempt to retry unpacking such data. Fixes issue #2714. --- Makefile.am | 1 + libarchive/archive_read_support_format_rar5.c | 41 ++++++++++++++++--- libarchive/test/test_read_format_rar5.c | 12 ++++++ .../test/test_read_format_rar5_dirdata.rar.uu | 6 +++ 4 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 libarchive/test/test_read_format_rar5_dirdata.rar.uu diff --git a/Makefile.am b/Makefile.am index 9dd37bb30..1d202cd8c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -952,6 +952,7 @@ libarchive_test_EXTRA_DIST=\ libarchive/test/test_read_format_rar5_multiple_files.rar.uu \ libarchive/test/test_read_format_rar5_multiple_files_solid.rar.uu \ libarchive/test/test_read_format_rar5_nonempty_dir_stream.rar.uu \ + libarchive/test/test_read_format_rar5_dirdata.rar.uu \ libarchive/test/test_read_format_rar5_owner.rar.uu \ libarchive/test/test_read_format_rar5_readtables_overflow.rar.uu \ libarchive/test/test_read_format_rar5_sfx.exe.uu \ diff --git a/libarchive/archive_read_support_format_rar5.c b/libarchive/archive_read_support_format_rar5.c index 48dde0c2e..fb184eb5c 100644 --- a/libarchive/archive_read_support_format_rar5.c +++ b/libarchive/archive_read_support_format_rar5.c @@ -1686,6 +1686,33 @@ static int process_head_file_extra(struct archive_read* a, return ARCHIVE_OK; } +static int file_entry_sanity_checks(struct archive_read* a, + size_t block_flags, uint8_t is_dir, uint64_t unpacked_size, + size_t packed_size) +{ + if (is_dir) { + const int declares_data_size = + (int) (unpacked_size != 0 || packed_size != 0); + + /* FILE entries for directories still declare HFL_DATA in block flags, + even though attaching data to such blocks doesn't make much sense. */ + if (declares_data_size) { + archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, + "directory entries cannot have any data"); + return ARCHIVE_FATAL; + } + } else { + const int declares_hfl_data = (int) ((block_flags & HFL_DATA) != 0); + if (!declares_hfl_data) { + archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, + "no data found in file/service block"); + return ARCHIVE_FATAL; + } + } + + return ARCHIVE_OK; +} + static int process_head_file(struct archive_read* a, struct rar5* rar, struct archive_entry* entry, size_t block_flags) { @@ -1701,6 +1728,7 @@ static int process_head_file(struct archive_read* a, struct rar5* rar, int c_method = 0, c_version = 0; char name_utf8_buf[MAX_NAME_IN_BYTES]; const uint8_t* p; + int sanity_ret; enum FILE_FLAGS { DIRECTORY = 0x0001, UTIME = 0x0002, CRC32 = 0x0004, @@ -1744,10 +1772,6 @@ static int process_head_file(struct archive_read* a, struct rar5* rar, rar->file.bytes_remaining = data_size; } else { rar->file.bytes_remaining = 0; - - archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, - "no data found in file/service block"); - return ARCHIVE_FATAL; } if(!read_var_sized(a, &file_flags, NULL)) @@ -1764,6 +1788,13 @@ static int process_head_file(struct archive_read* a, struct rar5* rar, rar->file.dir = (uint8_t) ((file_flags & DIRECTORY) > 0); + sanity_ret = file_entry_sanity_checks(a, block_flags, rar->file.dir, + unpacked_size, data_size); + + if (sanity_ret != ARCHIVE_OK) { + return sanity_ret; + } + if(!read_var_sized(a, &file_attr, NULL)) return ARCHIVE_EOF; @@ -4163,7 +4194,7 @@ static int rar5_read_data(struct archive_read *a, const void **buff, * it's impossible to perform any decompression. */ archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT, "Can't decompress an entry marked as a directory"); - return ARCHIVE_FAILED; + return ARCHIVE_FATAL; } if(!rar->skip_mode && (rar->cstate.last_write_ptr > rar->file.unpacked_size)) { diff --git a/libarchive/test/test_read_format_rar5.c b/libarchive/test/test_read_format_rar5.c index fd233277b..18862068c 100644 --- a/libarchive/test/test_read_format_rar5.c +++ b/libarchive/test/test_read_format_rar5.c @@ -1111,6 +1111,18 @@ DEFINE_TEST(test_read_format_rar5_nonempty_dir_stream) EPILOGUE(); } +DEFINE_TEST(test_read_format_rar5_nonempty_dir_data) +{ + PROLOGUE("test_read_format_rar5_dirdata.rar"); + + /* This archive is invalid. It declares a directory entry with nonzero + data size. */ + + assertA(archive_read_next_header(a, &ae) == ARCHIVE_FATAL); + + EPILOGUE(); +} + DEFINE_TEST(test_read_format_rar5_fileattr) { unsigned long set, clear, flag; diff --git a/libarchive/test/test_read_format_rar5_dirdata.rar.uu b/libarchive/test/test_read_format_rar5_dirdata.rar.uu new file mode 100644 index 000000000..c7928f344 --- /dev/null +++ b/libarchive/test/test_read_format_rar5_dirdata.rar.uu @@ -0,0 +1,6 @@ +begin 644 - +M4F%R(1H'`0`BD'[;,`$%,#8P`0&`@("``B?GD;$U`@(+@X``"_C5%:2#``(` +M`#"``S`P,#`P,#`P,#!);S#6KA',@]:N$?*IN;YV[8"1S>?4^`,#`R,#`P,# +-`P,#1)'C@XX*4`O.^P`` +` +end -- 2.47.2