From f7b1d38b31877f39ea979722ddf1ef072a6c3af4 Mon Sep 17 00:00:00 2001 From: Grzegorz Antoniak Date: Sun, 5 May 2019 08:16:03 +0200 Subject: [PATCH] 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. --- libarchive/archive_read_support_format_rar5.c | 28 ++++++--- libarchive/test/test_read_format_rar5.c | 58 +++++++++++++++---- ...ead_format_rar5_nonempty_dir_stream.rar.uu | 9 +++ 3 files changed, 76 insertions(+), 19 deletions(-) create mode 100644 libarchive/test/test_read_format_rar5_nonempty_dir_stream.rar.uu diff --git a/libarchive/archive_read_support_format_rar5.c b/libarchive/archive_read_support_format_rar5.c index fff934624..7bb98074d 100644 --- 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 ecbd44408..29af4ea61 100644 --- 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 index 000000000..c508c1f97 --- /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 -- 2.47.2