RAR5 reader: don't try to unpack entries marked as directories

RAR5 structure contains two places where a file can be marked as a
directory. First place is inside the file_flags field in FILE and
SERVICE base blocks, and the second place is inside file_attributes
bitfield also in the same base blocks.

The first directory flag was used to decide if the reader should
allocate any memory for the dictionary buffer needed to unpack the
files. Because if the file is actually a directory, then there should be
nothing to unpack, so if a file was marked as a directory here, the
reader did not allocate any dictionary buffer.

The second directory flag was used to indicate what file attributes
should be passed to the caller. So this second directory flag was used
as an actual indicator what the caller should do during archive
unpacking: should it treat it as a directory, or should it treat it as a
file.

Because of this situation, it was possible to declare a file as a
directory in the file_flags field, but do not declare it as a directory
in the second field, also adding a compressed stream to the FILE/SERVICE
base block. This situation was leading to a condition where the reader
was trying to use unallocated/already freed memory (because it did not
allocate a new dictionary buffer due to the directory flag set in
file_flags).

This commit fixes it so that the reader will check if it tries to
decompress a FILE/SERVICE block that has been declared as a directory in
the file_flags field. If the check will evaluate to true, it will return
an ARCHIVE_FAILED code, because it's not a valid action to take, and
shouldn't exist in valid archives at all.

Also added a unit test for this issue.

This should fix OSSFuzz issue #14574.

This commit also has influenced some of the other unit tests, because it
turned out the sample files used in other tests also did have
inconsistent directory flags in the file_flags and file_attributes
fields. So, some assertions in affected test cases have been changed to
be more relaxed, but still functional.

diff --git a/libarchive/archive_read_support_format_rar5.c b/libarchive/archive_read_support_format_rar5.c
index fff9346246986342fb98f1594fb0a9e7f83b1654..7bb98074d0756d0453c9271b41c2a5f9b0a80595 100644 (file)
--- a/libarchive/archive_read_support_format_rar5.c
+++ b/libarchive/archive_read_support_format_rar5.c
@@ -97,6 +97,7 @@ struct file_header {
        uint8_t solid : 1;           /* Is this a solid stream? */
        uint8_t service : 1;         /* Is this file a service data? */
        uint8_t eof : 1;             /* Did we finish unpacking the file? */
+       uint8_t dir : 1;             /* Is this file entry a directory? */
 
        /* Optional time fields. */
        uint64_t e_mtime;
@@ -1545,7 +1546,7 @@ static int process_head_file(struct archive_read* a, struct rar5* rar,
        size_t name_size = 0;
        uint64_t unpacked_size;
        uint32_t mtime = 0, crc = 0;
-       int c_method = 0, c_version = 0, is_dir;
+       int c_method = 0, c_version = 0;
        char name_utf8_buf[MAX_NAME_IN_BYTES];
        const uint8_t* p;
 
@@ -1604,7 +1605,7 @@ static int process_head_file(struct archive_read* a, struct rar5* rar,
                return ARCHIVE_FATAL;
        }
 
-       is_dir = (int) (file_flags & DIRECTORY);
+       rar->file.dir = (uint8_t) ((file_flags & DIRECTORY) > 0);
 
        if(!read_var_sized(a, &file_attr, NULL))
                return ARCHIVE_EOF;
@@ -1625,7 +1626,7 @@ static int process_head_file(struct archive_read* a, struct rar5* rar,
        c_method = (int) (compression_info >> 7) & 0x7;
        c_version = (int) (compression_info & 0x3f);
 
-       rar->cstate.window_size = is_dir ?
+       rar->cstate.window_size = (rar->file.dir > 0) ?
                0 :
                g_unpack_window_size << ((compression_info >> 10) & 15);
        rar->cstate.method = c_method;
@@ -2155,11 +2156,15 @@ static void init_unpack(struct rar5* rar) {
                rar->cstate.window_mask = 0;
 
        free(rar->cstate.window_buf);
-
        free(rar->cstate.filtered_buf);
 
-       rar->cstate.window_buf = calloc(1, rar->cstate.window_size);
-       rar->cstate.filtered_buf = calloc(1, rar->cstate.window_size);
+       if(rar->cstate.window_size > 0) {
+               rar->cstate.window_buf = calloc(1, rar->cstate.window_size);
+               rar->cstate.filtered_buf = calloc(1, rar->cstate.window_size);
+       } else {
+               rar->cstate.window_buf = NULL;
+               rar->cstate.filtered_buf = NULL;
+       }
 
        rar->cstate.write_ptr = 0;
        rar->cstate.last_write_ptr = 0;
@@ -3674,6 +3679,16 @@ static int rar5_read_data(struct archive_read *a, const void **buff,
        int ret;
        struct rar5* rar = get_context(a);
 
+       if(rar->file.dir > 0) {
+               /* Don't process any data if this file entry was declared
+                * as a directory. This is needed, because entries marked as
+                * directory doesn't have any dictionary buffer allocated, so
+                * 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;
+       }
+
        if(!rar->skip_mode && (rar->cstate.last_write_ptr > rar->file.unpacked_size)) {
                archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER,
                                "Unpacker has written too many bytes");
@@ -3776,7 +3791,6 @@ static int rar5_cleanup(struct archive_read *a) {
        struct rar5* rar = get_context(a);
 
        free(rar->cstate.window_buf);
-
        free(rar->cstate.filtered_buf);
 
        free(rar->vol.push_buf);

diff --git a/libarchive/test/test_read_format_rar5.c b/libarchive/test/test_read_format_rar5.c
index ecbd44408a2066d7884e9b45bd0e735c2875c9eb..29af4ea61f7ad963f78770f7828662245d521c96 100644 (file)
--- a/libarchive/test/test_read_format_rar5.c
+++ b/libarchive/test/test_read_format_rar5.c
@@ -972,8 +972,10 @@ DEFINE_TEST(test_read_format_rar5_readtables_overflow)
        assertA(0 == archive_read_next_header(a, &ae));
        /* This archive is invalid. However, processing it shouldn't cause any
         * buffer overflow errors during reading rar5 tables. */
-       assertA(0 == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }
@@ -987,8 +989,10 @@ DEFINE_TEST(test_read_format_rar5_leftshift1)
        assertA(0 == archive_read_next_header(a, &ae));
        /* This archive is invalid. However, processing it shouldn't cause any
         * errors related to undefined operations when using -fsanitize. */
-       assertA(ARCHIVE_FATAL == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }
@@ -1000,10 +1004,13 @@ DEFINE_TEST(test_read_format_rar5_leftshift2)
        PROLOGUE("test_read_format_rar5_leftshift2.rar");
 
        assertA(0 == archive_read_next_header(a, &ae));
+
        /* This archive is invalid. However, processing it shouldn't cause any
         * errors related to undefined operations when using -fsanitize. */
-       assertA(ARCHIVE_FATAL == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }
@@ -1015,10 +1022,13 @@ DEFINE_TEST(test_read_format_rar5_truncated_huff)
        PROLOGUE("test_read_format_rar5_truncated_huff.rar");
 
        assertA(0 == archive_read_next_header(a, &ae));
+
        /* This archive is invalid. However, processing it shouldn't cause any
         * errors related to undefined operations when using -fsanitize. */
-       assertA(ARCHIVE_FATAL == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_FATAL == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }
@@ -1030,10 +1040,13 @@ DEFINE_TEST(test_read_format_rar5_invalid_dict_reference)
        PROLOGUE("test_read_format_rar5_invalid_dict_reference.rar");
 
        assertA(0 == archive_read_next_header(a, &ae));
+
        /* This archive is invalid. However, processing it shouldn't cause any
         * errors related to buffer underflow when using -fsanitize. */
-       assertA(ARCHIVE_FATAL == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }
@@ -1045,10 +1058,31 @@ DEFINE_TEST(test_read_format_rar5_distance_overflow)
        PROLOGUE("test_read_format_rar5_distance_overflow.rar");
 
        assertA(0 == archive_read_next_header(a, &ae));
+
        /* This archive is invalid. However, processing it shouldn't cause any
         * errors related to variable overflows when using -fsanitize. */
-       assertA(ARCHIVE_FATAL == archive_read_data(a, buf, sizeof(buf)));
-       assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
+
+       EPILOGUE();
+}
+
+DEFINE_TEST(test_read_format_rar5_nonempty_dir_stream)
+{
+       uint8_t buf[16];
+
+       PROLOGUE("test_read_format_rar5_nonempty_dir_stream.rar");
+
+       assertA(0 == archive_read_next_header(a, &ae));
+
+       /* This archive is invalid. However, processing it shouldn't cause any
+        * errors related to buffer overflows when using -fsanitize. */
+       assertA(archive_read_data(a, buf, sizeof(buf)) <= 0);
+
+       /* This test only cares about not returning success here. */
+       assertA(ARCHIVE_OK != archive_read_next_header(a, &ae));
 
        EPILOGUE();
 }

diff --git a/libarchive/test/test_read_format_rar5_nonempty_dir_stream.rar.uu b/libarchive/test/test_read_format_rar5_nonempty_dir_stream.rar.uu
new file mode 100644 (file)
index 0000000..c508c1f
--- /dev/null
+++ b/libarchive/test/test_read_format_rar5_nonempty_dir_stream.rar.uu
@@ -0,0 +1,9 @@
+begin 644 test_read_format_rar5_nonempty_dir_stream.rar
+M4F%R(1H'`0"-[P+2``(''($'$7\`_R4``BP<`0(`(0#_Y@```"#2````____
+M_P`(`/__^P#_W0`"(8#_`(:&;;%DS+?,L8```!;(&P#>``#__^_P```4```&
+M`````````````+`!`@`A`/_F````(-(```#_____``@`___[`/_=``(A``++
+M``"`]/^`!P!#^_____\"(2$!`@`A````_R4``B$A`0(`@````0```"#&`/_=
+M``(A@/\`AH9ML63,M\R`_P```,@;`````!@`````_0`````````!87(A&@<`
+5`(WO`M(``O8'&X`'`#C[_X`E``(A
+`
+end