From: Grzegorz Antoniak Date: Wed, 1 May 2019 04:47:31 +0000 (+0200) Subject: RAR5 reader: fix bad shift-left operations. X-Git-Tag: v3.4.0~57^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=02f465f175f9925a1779e09e9f5f563e5a8fc5b4;p=thirdparty%2Flibarchive.git RAR5 reader: fix bad shift-left operations. This commit fixes some undefined shift-left operations on types that do not support such a big shift. Those invalid shift operations were triggering on invalid files produced by fuzzing. The commit also contains two unit tests that ensure such problems won't arise in the future. Fixes OSSFuzz cases #14490 and #14491. --- diff --git a/libarchive/archive_read_support_format_rar5.c b/libarchive/archive_read_support_format_rar5.c index cf897657c..aac74cd47 100644 --- a/libarchive/archive_read_support_format_rar5.c +++ b/libarchive/archive_read_support_format_rar5.c @@ -2524,7 +2524,9 @@ static int parse_filter_data(struct rar5* rar, const uint8_t* p, return ARCHIVE_EOF; } - data += (byte >> 8) << (i * 8); + /* Cast to uint32_t will ensure the shift operation will not produce + * undefined result. */ + data += ((uint32_t) byte >> 8) << (i * 8); skip_bits(rar, 8); } @@ -2748,7 +2750,11 @@ static int do_uncompress_block(struct archive_read* a, const uint8_t* p) { dist += dist_slot; } else { dbits = dist_slot / 2 - 1; - dist += (2 | (dist_slot & 1)) << dbits; + + /* Cast to uint32_t will make sure the shift left operation + * won't produce undefined result. Then, the uint32_t type will + * be implicitly casted to int. */ + dist += (uint32_t) (2 | (dist_slot & 1)) << dbits; } if(dbits > 0) { diff --git a/libarchive/test/test_read_format_rar5.c b/libarchive/test/test_read_format_rar5.c index d52b6002c..9b03af13b 100644 --- a/libarchive/test/test_read_format_rar5.c +++ b/libarchive/test/test_read_format_rar5.c @@ -965,3 +965,33 @@ DEFINE_TEST(test_read_format_rar5_readtables_overflow) EPILOGUE(); } + +DEFINE_TEST(test_read_format_rar5_leftshift1) +{ + uint8_t buf[16]; + + PROLOGUE("test_read_format_rar5_leftshift1.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)); + + EPILOGUE(); +} + +DEFINE_TEST(test_read_format_rar5_leftshift2) +{ + uint8_t buf[16]; + + 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)); + + EPILOGUE(); +} diff --git a/libarchive/test/test_read_format_rar5_leftshift1.rar.uu b/libarchive/test/test_read_format_rar5_leftshift1.rar.uu new file mode 100644 index 000000000..694a27f6f --- /dev/null +++ b/libarchive/test/test_read_format_rar5_leftshift1.rar.uu @@ -0,0 +1,9 @@ +begin 644 test_read_format_rar5_leftshift1.rar +M4F%R(1H'`0"-[P+2``(''(`'`"``_R4``B$<`0(`#@```0```"#2````____ +M_P`(`/__^P#_W0`"(8#_`(:&;;%DS+?,L=:NL0#(3`$````````````````` +M``"``````````+!DS+*RL[*RL@```-P``````````````````(`````````` +ML&3,LK*RLK*R````W`````#X____````````````````````````%5H>;&@T +M+3HW"2!SB^)_@`````` +` +end diff --git a/libarchive/test/test_read_format_rar5_leftshift2.rar.uu b/libarchive/test/test_read_format_rar5_leftshift2.rar.uu new file mode 100644 index 000000000..57ffad725 --- /dev/null +++ b/libarchive/test/test_read_format_rar5_leftshift2.rar.uu @@ -0,0 +1,6 @@ +begin 644 test_read_format_rar5_leftshift2.rar +M4F%R(1H'`0"-[P+2``(''(`'`2``_RL``B'+`0(`,O__````-WJ\KR<<)0`" +M(;<*`BY*`!```&;%T#%24%"`_R4`[@K+(2Y*`&$``'__`/\E``(N2@`0`0(` +0(?__`%N&?Q2UH.CHZ.CHZ``` +` +end