Improve handling of malformed ZIP files by Fi418 · Pull Request #1539 · Cisco-Talos/clamav

Fi418 · 2025-07-08T19:11:31Z

This addresses #1534 and makes some minor improvements for maintainability when handling NULL terminated strings.

Solves loops when unzipping ZIP64 as well but only due to an overlapping cause.

This avoids error-prone assigning of NULL terminators and simplifies control flow. I kept the limit to 255, but I'm not sure it's part of the standard. Signed-off-by: Sophie0x2E <219585213+Sophie0x2E@users.noreply.github.com>

This may not be the correct solution but I assumed we want to move over the record and set the parse error flag. Signed-off-by: Sophie0x2E <219585213+Sophie0x2E@users.noreply.github.com>

val-ms · 2025-07-29T16:02:42Z

    }

+    if (size_of_fileheader_and_data == 0) {
+        size_of_fileheader_and_data = zip - local_header;


Unfortunately we can't use zip after done: because it isn't set until after the first goto done;.

But I think it's fine if it returns 0 if we set *ret = CL_EPARSE as you suggested 👍. We just need to check for either 0 or, more importantly, CL_EPARSE in the calling functions.

I think this may be a better solution. What do you think?

diff --git a/libclamav/unzip.c b/libclamav/unzip.c index b7b0ba6a1..35a88979a 100644 --- a/libclamav/unzip.c +++ b/libclamav/unzip.c @@ -776,6 +776,10 @@ done: free(original_filename); } + if (size_of_fileheader_and_data == 0) { + *ret = CL_EPARSE; + } + return size_of_fileheader_and_data; } @@ -1245,8 +1249,7 @@ cl_error_t index_local_file_headers_within_bounds( if (!(ptr = fmap_need_off_once(map, coff, 4))) continue; if (cli_readint32(ptr) == ZIP_MAGIC_LOCAL_FILE_HEADER) { - // increment coff by the size of the found local file header + file data - coff += parse_local_file_header( + size_t file_record_size = parse_local_file_header( map, coff, fsize - coff, @@ -1259,13 +1262,16 @@ cl_error_t index_local_file_headers_within_bounds( 1, NULL, &(zip_catalogue[index])); - // decrement coff by 1 to account for the increment at the end of the loop - coff -= 1; - if (CL_EPARSE != ret) { + if (file_record_size != 0 && CL_EPARSE != ret) { // Found a record. + cli_dbgmsg("cli_unzip: Found a record\n"); index++; total_file_count++; + + // increment coff by the size of the found local file header + file data + // but decrement by 1 to account for the increment at the end of the loop + coff += file_record_size - 1; } if (ret == CL_VIRUS) {

Nice catch. This was missing a else clause for CL_EPARSE at the caller but it ended up being simpler to set CL_EPARSE inside parse_local_file_header. Let me know if you prefer a different way.

I think we still need my suggested changes here in index_local_file_headers_within_bounds() to check ret before using the return value.

On a tangent, pretty much every other libclamav functions returns a cl_error_t and passes back everything else through parameters. It's more work, but it would be nice if we did it that way for parse_local_file_header(). That's a pretty opinionated change though. I can pick this up if you want.

This reverts commit de15a41.

…into CLAM-1534

val-ms

Oh Good call setting *ret in other error cases. It seems like we should set it to CL_SUCCESS immediately before the done: label as well.

Edit: Never mind. Setting to CL_SUCCESS right before done isn't enough. It might override a prior error with unzipping or decrypting. It would fit much better with other libclamav error handling patterns though. I'm going to play with this some more.

val-ms · 2025-07-30T16:58:42Z

        zsize -= 12;
        if (fmap_need_ptr_once(map, zip, 4)) {
-            if (cli_readint32(zip) == ZIP_MAGIC_FILE_BEGIN_SPLIT_OR_SPANNED) {
+            if (zip && cli_readint32(zip) == ZIP_MAGIC_FILE_BEGIN_SPLIT_OR_SPANNED) {


It would be a bug in fmap_need_ptr_once() if it returned a non-NULL value when zip is NULL. Prior logic also already used and incremented the zip pointer multiple times. It is harmless, but I don't think we should have this NULL-check.

I only did it to silence a warning, it can be removed.

You could probably do like this to silence the warning as well :

zip = fmap_need_ptr_once(map, zip, 4); if (NULL != zip) { ...

The fmap_need_ptr_once() function returns zip after verifying that the data pointed to by zip of length 4 is within the file and is paged into memory (for a file backed memory map).

val-ms · 2025-07-30T17:11:05Z

    }

+    if (size_of_fileheader_and_data == 0) {
+        size_of_fileheader_and_data = zip - local_header;


I think we still need my suggested changes here in index_local_file_headers_within_bounds() to check ret before using the return value.

On a tangent, pretty much every other libclamav functions returns a cl_error_t and passes back everything else through parameters. It's more work, but it would be nice if we did it that way for parse_local_file_header(). That's a pretty opinionated change though. I can pick this up if you want.

val-ms · 2025-07-30T17:11:50Z

+    if (*ret == CL_EPARSE) {
+        cli_dbgmsg("cli_unzip: Skipping local header for CL_EPARSE\n");
+        if (zip && local_header) {
+            size_of_fileheader_and_data = zip - local_header;
+        } else {
+            // Shouldn't happen, but best to log if it does due to changes
+            cli_dbgmsg("cli_unzip: zip %p local_header %p\n", zip, local_header);
+        }
+    }


I don't think this is necessary at all. The calling function should not use the return value if *ret is not CL_SUCCESS. So there is no obligation to try to make it be correct.

At least I think that's a decent plan. Can you think of a reason why it should return size_of_fileheader_and_data if there was a parse error?

It's not necessary. I only added it in case a mistake is done on new additions to the function since not all features of ZIP archives are present.

It can be removed.

val-ms · 2025-07-30T17:13:29Z

    if (zsize <= LOCAL_HEADER_flen) {
        cli_dbgmsg("cli_unzip: local header - fname out of file\n");
        fmap_unneed_off(map, loff, SIZEOF_LOCAL_HEADER);
        goto done;


This probably needs *ret = CL_VIRUS; as well

An infinite loop may occur when scanning some malformed ZIP files. I introduced this issue in 96c00b6 with this line: ```c // decrement coff by 1 to account for the increment at the end of the loop coff -= 1; ``` The problem is that the function may return 0, which should indicate that there are no more files. The result was that `coff` would stay the same and the loop would repeat. This issue is in 1.5 development and affects the 1.5.0 beta but does not affect any production versions. Fixes: Cisco-Talos#1534 Special thanks to Sophie0x2E for an initial fix, proposed in Cisco-Talos#1539 In review, I was uncomfortable with other existing code and decided to to a more significant overhaul of the error handling in the ZIP module.

An infinite loop may occur when scanning some malformed ZIP files. I introduced this issue in 96c00b6 with this line: ```c // decrement coff by 1 to account for the increment at the end of the loop coff -= 1; ``` The problem is that the function may return 0, which should indicate that there are no more files. The result was that `coff` would stay the same and the loop would repeat. This issue is in 1.5 development and affects the 1.5.0 beta but does not affect any production versions. Fixes: Cisco-Talos#1534 Special thanks to Sophie0x2E for an initial fix, proposed in Cisco-Talos#1539 In review, I was uncomfortable with other existing code and decided to to a more significant overhaul of the error handling in the ZIP module. In addition to cleanup, this commit has some functional changes: - When parsing a central directory file header inside of `parse_central_directory_file_header()`, it will now fail out if the "extra length" or "comment length" fields would exceced the length of the archive. That doesn't mean the associated local file header won't be parsed later, but it won't use the central directory file header to find it. Instead, the ZIP module will have to find the local file header by searching for extra records not listed in the central directory. This change was mostly to tidy up complex error handling. - Add two FTM new signatures to identify split ZIP archives. This signature identifies the first segment (first file) in a split or spanned ZIP archive. It may also be found on a single-segment "split" archive, depending on the ZIP archiver. ``` 0:0:504b0708504b0304:ZIP (First segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP ``` Practically speaking, this new signature makes it so ClamAV identifies the file as a ZIP right away without having to rely on SFX_ZIP detection. Extraction is then handled by the ZIP `cli_unzip` function rather than extracting each with `cli_unzip_single` which handles SFX_ZIP entries. Note: ClamAV isn't capable of finding additional files on disk to support handling the additional segments. So it doesn't make any difference with handling those other files. This signature is for single-segment split/spanned archives, depending on the ZIP archiver. ``` 0:0:504b0303504b0304:ZIP (Single-segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP ``` Like the first one, this also means we won't rely on SFX_ZIP detection and will treat this files as regular ZIPs. - Added a test file to verify that ClamAV can extract a single-file "split" ZIP. - Added a clamscan test with test files to verify that scanning a split archive across two segments correctly extracts the properly formed zip file entries. Sadly, we can't join the segments to extract everything.

val-ms · 2025-08-06T00:11:44Z

@Sophie0x2E The more I looked at the ZIP module the more I wanted to refactor. It didn't make sense to ask you to make so many changes, so I started a new branch and went to town on it: #1548

Thank you so much for finding the initial issue and for your other proposed improvements in this PR. I included them in my new PR and tried to give you credit in the commit message.

An infinite loop may occur when scanning some malformed ZIP files. I introduced this issue in 96c00b6 with this line: ```c // decrement coff by 1 to account for the increment at the end of the loop coff -= 1; ``` The problem is that the function may return 0, which should indicate that there are no more files. The result was that `coff` would stay the same and the loop would repeat. This issue is in 1.5 development and affects the 1.5.0 beta but does not affect any production versions. Fixes: Cisco-Talos#1534 Special thanks to Sophie0x2E for an initial fix, proposed in Cisco-Talos#1539 In review, I was uncomfortable with other existing code and decided to to a more significant overhaul of the error handling in the ZIP module. In addition to cleanup, this commit has some functional changes: - When parsing a central directory file header inside of `parse_central_directory_file_header()`, it will now fail out if the "extra length" or "comment length" fields would exceced the length of the archive. That doesn't mean the associated local file header won't be parsed later, but it won't use the central directory file header to find it. Instead, the ZIP module will have to find the local file header by searching for extra records not listed in the central directory. This change was mostly to tidy up complex error handling. - Add two FTM new signatures to identify split ZIP archives. This signature identifies the first segment (first file) in a split or spanned ZIP archive. It may also be found on a single-segment "split" archive, depending on the ZIP archiver. ``` 0:0:504b0708504b0304:ZIP (First segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP ``` Practically speaking, this new signature makes it so ClamAV identifies the file as a ZIP right away without having to rely on SFX_ZIP detection. Extraction is then handled by the ZIP `cli_unzip` function rather than extracting each with `cli_unzip_single` which handles SFX_ZIP entries. Note: ClamAV isn't capable of finding additional files on disk to support handling the additional segments. So it doesn't make any difference with handling those other files. This signature is for single-segment split/spanned archives, depending on the ZIP archiver. ``` 0:0:504b0303504b0304:ZIP (Single-segment split/spanned):CL_TYPE_ANY:CL_TYPE_ZIP ``` Like the first one, this also means we won't rely on SFX_ZIP detection and will treat this files as regular ZIPs. - Added a test file to verify that ClamAV can extract a single-file "split" ZIP. - Added a clamscan test with test files to verify that scanning a split archive across two segments correctly extracts the properly formed zip file entries. Sadly, we can't join the segments to extract everything.

Fi418 added 2 commits July 8, 2025 20:08

Zero out memory on initialization

446be2b

This avoids error-prone assigning of NULL terminators and simplifies control flow. I kept the limit to 255, but I'm not sure it's part of the standard. Signed-off-by: Sophie0x2E <219585213+Sophie0x2E@users.noreply.github.com>

Handle loop when parsing invalid ZIP files

5efc868

This may not be the correct solution but I assumed we want to move over the record and set the parse error flag. Signed-off-by: Sophie0x2E <219585213+Sophie0x2E@users.noreply.github.com>

val-ms self-requested a review July 29, 2025 15:57

val-ms requested changes Jul 29, 2025

View reviewed changes

Fi418 force-pushed the CLAM-1534 branch from a1fac5d to 2156e84 Compare July 29, 2025 18:07

Fi418 added 2 commits July 29, 2025 20:22

Check for CL_EPARSE in caller when unzipping

de15a41

Revert "Check for CL_EPARSE in caller when unzipping"

4114cad

This reverts commit de15a41.

Fi418 force-pushed the CLAM-1534 branch from 2156e84 to 4114cad Compare July 29, 2025 19:25

Set CL_EPARSE on out of bounds when unzipping

4e7393a

github-advanced-security AI found potential problems Jul 29, 2025

View reviewed changes

Comment thread libclamav/unzip.c Fixed

Comment thread libclamav/unzip.c Fixed

Fi418 added 4 commits July 29, 2025 21:02

Set CL_EPARSE on out of bounds when unzipping

de2fee2

Fix log with local header parameters

46f4370

Merge branch 'CLAM-1534' of https://github.com/Sophie0x2E/clamav-fork …

fd3f7eb

…into CLAM-1534

CodeQL warnings

0267652

Fi418 requested a review from val-ms July 29, 2025 20:12

val-ms requested changes Jul 30, 2025

View reviewed changes

val-ms mentioned this pull request Aug 6, 2025

ZIP: Fix infinite loop + significant code cleanup #1548

Merged

val-ms closed this Aug 6, 2025

Fi418 deleted the CLAM-1534 branch August 6, 2025 23:12

Uh oh!

Conversation

Fi418 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

val-ms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

val-ms commented Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fi418 commented Jul 8, 2025 •

edited

Loading

val-ms left a comment •

edited

Loading