Skip to content

Improve handling of malformed ZIP files #1539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Fi418 wants to merge 9 commits into from
Closed

Improve handling of malformed ZIP files #1539

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
446be2b
Zero out memory on initialization
Fi418 Jul 8, 2025
5efc868
Handle loop when parsing invalid ZIP files
Fi418 Jul 8, 2025
de15a41
Check for CL_EPARSE in caller when unzipping
Fi418 Jul 29, 2025
4114cad
Revert "Check for CL_EPARSE in caller when unzipping"
Fi418 Jul 29, 2025
4e7393a
Set CL_EPARSE on out of bounds when unzipping
Fi418 Jul 29, 2025
de2fee2
Set CL_EPARSE on out of bounds when unzipping
Fi418 Jul 29, 2025
46f4370
Fix log with local header parameters
Fi418 Jul 29, 2025
fd3f7eb
Merge branch 'CLAM-1534' of https://github.com/Sophie0x2E/clamav-fork…
Fi418 Jul 29, 2025
0267652
CodeQL warnings
Fi418 Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions libclamav/unzip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,10 +616,10 @@ static unsigned int parse_local_file_header(
zip_cb zcb,
struct zip_record *record)
{
const uint8_t *local_header, *zip;
char name[256];
const uint8_t *local_header = NULL, *zip = NULL;
char name[256] = {0};
char *original_filename = NULL;
uint32_t csize, usize;
uint32_t csize = 0, usize = 0;
unsigned int size_of_fileheader_and_data = 0;

uint32_t nsize = 0;
Expand All @@ -641,31 +641,27 @@ static unsigned int parse_local_file_header(
zip = local_header + SIZEOF_LOCAL_HEADER;
zsize -= SIZEOF_LOCAL_HEADER;

memset(name, '\0', 256);

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;

@val-ms val-ms Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs *ret = CL_VIRUS; as well

}

nsize = (LOCAL_HEADER_flen >= sizeof(name)) ? sizeof(name) - 1 : LOCAL_HEADER_flen;
nsize = LOCAL_HEADER_flen >= (sizeof(name) - 1) ? sizeof(name) - 1 : LOCAL_HEADER_flen;
cli_dbgmsg("cli_unzip: nsize %u\n", nsize);
src = fmap_need_ptr_once(map, zip, nsize);
if (nsize && (NULL != src)) {
memcpy(name, zip, nsize);
name[nsize] = '\0';
if (CL_SUCCESS != cli_basename(name, nsize, &original_filename)) {
original_filename = NULL;
}
} else {
name[0] = '\0';
}

Comment thread
Fi418 marked this conversation as resolved.
zip += LOCAL_HEADER_flen;
zsize -= LOCAL_HEADER_flen;

cli_dbgmsg("cli_unzip: local header - ZMDNAME:%d:%s:%u:%u:%x:%u:%u:%u\n",
((LOCAL_HEADER_flags & F_ENCR) != 0), name, LOCAL_HEADER_usize, LOCAL_HEADER_csize, LOCAL_HEADER_crc32, LOCAL_HEADER_method, file_count, ctx->recursion_level);
cli_dbgmsg("cli_unzip: local header - ZMDNAME:%d:%.255s:%u:%u:" /*%x:%u:*/ "%u:%u\n",
((LOCAL_HEADER_flags & F_ENCR) != 0), name, usize, csize, file_count, ctx->recursion_level);
/* ZMDfmt virname:encrypted(0-1):filename(exact|*):usize(exact|*):csize(exact|*):crc32(exact|*):method(exact|*):fileno(exact|*):maxdepth(exact|*) */

/* Scan file header metadata. */
Expand Down Expand Up @@ -709,6 +705,7 @@ static unsigned int parse_local_file_header(
if (zsize <= LOCAL_HEADER_elen) {
cli_dbgmsg("cli_unzip: local header - extra out of file\n");
fmap_unneed_off(map, loff, SIZEOF_LOCAL_HEADER);
*ret = CL_EPARSE;
goto done;
}
zip += LOCAL_HEADER_elen;
Expand All @@ -720,6 +717,7 @@ static unsigned int parse_local_file_header(
if (zsize < csize) {
cli_dbgmsg("cli_unzip: local header - stream out of file\n");
fmap_unneed_off(map, loff, SIZEOF_LOCAL_HEADER);
*ret = CL_EPARSE;
goto done;
}

Expand Down Expand Up @@ -757,13 +755,15 @@ static unsigned int parse_local_file_header(
if (LOCAL_HEADER_flags & F_USEDD) {
if (zsize < 12) {
cli_dbgmsg("cli_unzip: local header - data desc out of file\n");
*ret = CL_EPARSE;
goto done;
}
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) {

@val-ms val-ms Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Fi418 Fi418 Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@val-ms val-ms Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (zsize < 4) {
cli_dbgmsg("cli_unzip: local header - data desc out of file\n");
*ret = CL_EPARSE;
goto done;
}
zip += 4;
Expand All @@ -780,6 +780,16 @@ static unsigned int parse_local_file_header(
free(original_filename);
}

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);
}
}
Comment on lines +783 to +791

@val-ms val-ms Jul 30, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Fi418 Fi418 Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return size_of_fileheader_and_data;
}

Expand Down
Loading