From afd1eed9de2f702c0862586ef9f1c2a9819a9751 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Fri, 22 Apr 2022 16:16:12 -0700 Subject: [PATCH 1/2] Zip parser: tolerate 2-byte overlap in file entries The heuristic to alert on overlapping file entries is detecting some non-malicious JAR files observed in critical enterprise software. The goal with overlap detection is to alert on non-recursive zip- bombs, so this tiny overlap isn't a concern. We'll allow a 2-byte overlap so we don't alert on such zips. --- libclamav/unzip.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libclamav/unzip.c b/libclamav/unzip.c index b8b4a15dcc..3380a4943a 100644 --- a/libclamav/unzip.c +++ b/libclamav/unzip.c @@ -62,6 +62,10 @@ #define ZIP_MAGIC_FILE_BEGIN_SPLIT_OR_SPANNED (0x08074b50) // clang-format on +// Non-malicious zips in enterprise critical JAR-ZIPs have been observed with a 1-byte overlap. +// The goal with overlap detection is to alert on non-recursive zip bombs, so this tiny overlap isn't a concern. +// We'll allow a 2-byte overlap so we don't alert on such zips. +#define ZIP_RECORD_OVERLAP_FUDGE_FACTOR 2 #define ZIP_MAX_NUM_OVERLAPPING_FILES 5 #define ZIP_CRC32(r, c, b, l) \ @@ -1108,8 +1112,8 @@ cl_error_t index_the_central_directory( goto done; } - if (((curr_record->local_header_offset >= prev_record->local_header_offset) && (curr_record->local_header_offset < prev_record->local_header_offset + prev_record->local_header_size + prev_record->compressed_size)) || - ((prev_record->local_header_offset >= curr_record->local_header_offset) && (prev_record->local_header_offset < curr_record->local_header_offset + curr_record->local_header_size + curr_record->compressed_size))) { + if (((curr_record->local_header_offset >= prev_record->local_header_offset) && (curr_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < prev_record->local_header_offset + prev_record->local_header_size + prev_record->compressed_size)) || + ((prev_record->local_header_offset >= curr_record->local_header_offset) && (prev_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < curr_record->local_header_offset + curr_record->local_header_size + curr_record->compressed_size))) { /* Overlapping file detected */ num_overlapping_files++; From 696fa6a2dd3977494cd3667ea3e7bd8f2e3abf19 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Fri, 1 Jul 2022 18:56:04 -0700 Subject: [PATCH 2/2] squashme: address code review concerns --- libclamav/unzip.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/libclamav/unzip.c b/libclamav/unzip.c index 3380a4943a..aafd0aa2d6 100644 --- a/libclamav/unzip.c +++ b/libclamav/unzip.c @@ -1104,16 +1104,24 @@ cl_error_t index_the_central_directory( prev_record = &(zip_catalogue[index - 1]); curr_record = &(zip_catalogue[index]); + uint32_t prev_record_size = prev_record->local_header_size + prev_record->compressed_size; + uint32_t curr_record_size = curr_record->local_header_size + curr_record->compressed_size; + uint32_t prev_record_end; + uint32_t curr_record_end; + /* Check for integer overflow in 32bit size & offset values */ - if ((UINT32_MAX - (prev_record->local_header_size + prev_record->compressed_size) < prev_record->local_header_offset) || - (UINT32_MAX - (curr_record->local_header_size + curr_record->compressed_size) < curr_record->local_header_offset)) { + if ((UINT32_MAX - prev_record_size < prev_record->local_header_offset) || + (UINT32_MAX - curr_record_size < curr_record->local_header_offset)) { cli_dbgmsg("cli_unzip: Integer overflow detected; invalid data sizes in zip file headers.\n"); status = CL_EFORMAT; goto done; } - if (((curr_record->local_header_offset >= prev_record->local_header_offset) && (curr_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < prev_record->local_header_offset + prev_record->local_header_size + prev_record->compressed_size)) || - ((prev_record->local_header_offset >= curr_record->local_header_offset) && (prev_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < curr_record->local_header_offset + curr_record->local_header_size + curr_record->compressed_size))) { + prev_record_end = prev_record->local_header_offset + prev_record_size; + curr_record_end = curr_record->local_header_offset + curr_record_size; + + if (((curr_record->local_header_offset >= prev_record->local_header_offset) && (curr_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < prev_record_end)) || + ((prev_record->local_header_offset >= curr_record->local_header_offset) && (prev_record->local_header_offset + ZIP_RECORD_OVERLAP_FUDGE_FACTOR < curr_record_end))) { /* Overlapping file detected */ num_overlapping_files++; @@ -1123,7 +1131,7 @@ cl_error_t index_the_central_directory( cli_dbgmsg("cli_unzip: Ignoring duplicate file entry @ 0x%x.\n", curr_record->local_header_offset); } else { cli_dbgmsg("cli_unzip: Overlapping files detected.\n"); - cli_dbgmsg(" previous file end: %u\n", prev_record->local_header_offset + prev_record->local_header_size + prev_record->compressed_size); + cli_dbgmsg(" previous file end: %u\n", prev_record_end); cli_dbgmsg(" current file start: %u\n", curr_record->local_header_offset); if (ZIP_MAX_NUM_OVERLAPPING_FILES < num_overlapping_files) {