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.

diff --git a/Makefile.am b/Makefile.am
index 9dd37bb30054e0243baa2ebb58b74cc7124c96f1..1d202cd8c18536b65fcb32b2af2de166b9f1f34a 100644 (file)
--- 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 48dde0c2e8140974bf021c35968096b056691a12..fb184eb5ca707ca48f4b7064562852f4747bef5d 100644 (file)
--- 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 fd233277bc1b3f3220871d0e36090088797883f8..18862068c2d93e5adba37b67028ce6f749d228b3 100644 (file)
--- 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 (file)
index 0000000..c7928f3
--- /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