From aadf25df6a738544eb8d99b2b66e6cb42f9db385 Mon Sep 17 00:00:00 2001 From: "Val S." Date: Thu, 2 Oct 2025 11:46:14 -0400 Subject: [PATCH] Fix static analysis code quality issues (#1582) `libclamav/libmspack.c`: Initialize variables before first `goto done;` to fix unitialized variable use in an error condition. `libclamav/others.c`: Explicitly ignore return values for calls to add JSON values when subsequent calls don't depend on them. If we were to add error handling here, the only thing we'd do is debug- log it. I don't think it's worth adding the extra lines of code. `libclamav/unarj.c`: Removed dead code. The `status` variable is immediately set afterwards based on whether or not any files may be extracted. `libclamav/unzip.c`: Removed dead code. The `ret` variable is checked immediately after being set, above. This check after the `do`-`while()` loop is dead code. `sigtool/sigtool.c`: Fix potential NULL deref in error handling. This is a fix for the same issue as was fixed in a previous commit. I somehow overlooked this one. Copy/paste bug. `libclamav/pdfdecode.c`: Fix leaked `stream` memory when `filter_lzwdecode()` fails. `clamdtop/clamdtop.c`: Fix possible NULL dereference if `strchr` returns NULL in `read_version()` and `check_stats_available()`. `libclamav/rtf.c`: Fix memory leak in `rtf_object_process()` if `cli_gentemp_with_prefix()` fails. Also change empty for-loop to resolve clang-format weirdness and make it more obvious the for-loop has no body. `libclamav/aspack.c`: Ensure that `endoff - old` is not negative in `build_decrypt_array()` before passing to `CLI_ISCONTAINED()` which expects unsigned values. `libclamav/upx.c`: Fix integer overflow checks in multiple functions. `libclamav/vba_extract.c`: Set `entries` pointer back to NULL after free in `word_read_macro_entry()` error condition. `libclamav/unzip.c`: Remove logic to return `CL_EMAXFILES` from `index_local_file_headers()`. It seems it only overwrote the status when not `CL_SUCCESS` in which case it could be overriding a more serious failure. Further, updates to the how the ZIP parser works has made it so this needs to return `CL_SUCCESS` in order for the caller to at least scan the files found so far. Finally, the calling function has checks of its own to make sure we don't exceeds the max-files limit. `libclamav/unzip.c`: Fix issue where `cli_append_potentially_unwanted()` in `index_local_file_headers()` might overwrite an error in `status` with `CL_CLEAN`. Instead, it now checks the return value and only overwrites the `CL_EFORMAT` status with a different value if not `CL_SUCCESS`. `libclamav/unzip.c`: Fix a potential leak with `combined_catalogue` and `temp_catalogue` in an error condition. We should always free them if not NULL, not just if the function failed. And to make this safe, we must set `combined_catalogue` to NULL when we give ownership to `*catalogue`. `libclamav/scanners.c`: Fix a potential leak in error handling for the `cli_ole2_tempdir_scan_vba()` function. CLAM-2768 --- clamdtop/clamdtop.c | 18 ++++++++++-- libclamav/aspack.c | 10 +++++-- libclamav/libmspack.c | 6 ++-- libclamav/others.c | 20 +++++++------- libclamav/pdfdecode.c | 8 ++++-- libclamav/rtf.c | 10 +++++-- libclamav/scanners.c | 8 ++++-- libclamav/unarj.c | 3 -- libclamav/unzip.c | 61 ++++++++++++++++++----------------------- libclamav/upx.c | 8 +++--- libclamav/vba_extract.c | 1 + sigtool/sigtool.c | 6 ++-- 12 files changed, 90 insertions(+), 69 deletions(-) diff --git a/clamdtop/clamdtop.c b/clamdtop/clamdtop.c index 827edf7b5..90fee6f00 100644 --- a/clamdtop/clamdtop.c +++ b/clamdtop/clamdtop.c @@ -1338,12 +1338,19 @@ static int read_version(conn_t *conn) { char buf[1024]; unsigned i; + if (!recv_line(conn, buf, sizeof(buf))) return -1; + if (!strcmp(buf, "UNKNOWN COMMAND\n")) return -2; + + char *p = strchr(buf, ':'); + if (NULL == p) + return -1; + // check if VERSION command is available - if (!strcmp(strchr(buf, ':'), ": COMMAND UNAVAILABLE\n")) + if (!strcmp(p, ": COMMAND UNAVAILABLE\n")) return -3; conn->version = strdup(buf); @@ -1358,10 +1365,17 @@ static int check_stats_available(conn_t *conn) { char buf[1024]; send_string(conn, "nSTATS\n"); + if (!recv_line(conn, buf, sizeof(buf))) return 0; - if (!strcmp(strchr(buf, ':'), ": COMMAND UNAVAILABLE\n")) + + char *p = strchr(buf, ':'); + if (NULL == p) return 0; + + if (!strcmp(p, ": COMMAND UNAVAILABLE\n")) + return 0; + return 1; } diff --git a/libclamav/aspack.c b/libclamav/aspack.c index 67e8f3893..5b6e14ff3 100644 --- a/libclamav/aspack.c +++ b/libclamav/aspack.c @@ -161,9 +161,13 @@ static uint8_t build_decrypt_array(struct ASPK *stream, uint8_t *array, uint8_t if (counter >= 0x10) { uint32_t old = endoff; endoff = d3[i + 1] >> 0x10; - if (endoff - old) { - if (!CLI_ISCONTAINED(stream->dict_helper[which].ends, 0x100, stream->dict_helper[which].ends + old, endoff - old)) return 0; - memset((stream->dict_helper[which].ends + old), i + 1, endoff - old); + + if (endoff < old) return 0; + uint32_t remaining = endoff - old; + + if (remaining > 0) { + if (!CLI_ISCONTAINED(stream->dict_helper[which].ends, 0x100, stream->dict_helper[which].ends + old, remaining)) return 0; + memset((stream->dict_helper[which].ends + old), i + 1, remaining); } } diff --git a/libclamav/libmspack.c b/libclamav/libmspack.c index a4c98ff4d..8d1afa3bc 100644 --- a/libclamav/libmspack.c +++ b/libclamav/libmspack.c @@ -408,6 +408,9 @@ cl_error_t cli_scanmscab(cli_ctx *ctx, size_t sfx_offset) struct mspack_name mspack_fmap = {0}; struct mspack_system_ex ops_ex = {0}; + char *tmp_fname = NULL; + bool tempfile_exists = false; + mspack_fmap.fmap = ctx->fmap; if (sfx_offset > INT32_MAX) { @@ -418,9 +421,6 @@ cl_error_t cli_scanmscab(cli_ctx *ctx, size_t sfx_offset) mspack_fmap.org = (off_t)sfx_offset; - char *tmp_fname = NULL; - bool tempfile_exists = false; - memset(&ops_ex, 0, sizeof(struct mspack_system_ex)); ops_ex.ops = mspack_sys_fmap_ops; diff --git a/libclamav/others.c b/libclamav/others.c index 253b8fed2..d14a6aecb 100644 --- a/libclamav/others.c +++ b/libclamav/others.c @@ -1593,21 +1593,21 @@ static cl_error_t append_virus(cli_ctx *ctx, const char *virname, IndicatorType if (NULL == indicator_obj) { cli_errmsg("append_virus: no memory for json indicator object\n"); } else { - json_object_object_add(indicator_obj, "Name", json_object_new_string(virname)); + (void)json_object_object_add(indicator_obj, "Name", json_object_new_string(virname)); switch (type) { case IndicatorType_Strong: { - json_object_object_add(indicator_obj, "Type", json_object_new_string("Strong")); + (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("Strong")); } break; case IndicatorType_PotentiallyUnwanted: { - json_object_object_add(indicator_obj, "Type", json_object_new_string("PotentiallyUnwanted")); + (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("PotentiallyUnwanted")); } break; case IndicatorType_Weak: { - json_object_object_add(indicator_obj, "Type", json_object_new_string("Weak")); + (void)json_object_object_add(indicator_obj, "Type", json_object_new_string("Weak")); } break; } - json_object_object_add(indicator_obj, "Depth", json_object_new_int(0)); // 0 for this layer - cli_jsonuint64(indicator_obj, "ObjectID", (uint64_t)ctx->recursion_stack[ctx->recursion_level].object_id); - json_object_array_add(indicators, indicator_obj); + (void)json_object_object_add(indicator_obj, "Depth", json_object_new_int(0)); // 0 for this layer + (void)cli_jsonuint64(indicator_obj, "ObjectID", (uint64_t)ctx->recursion_stack[ctx->recursion_level].object_id); + (void)json_object_array_add(indicators, indicator_obj); } // If this is a strong or potentially unwanted indicator, we add it to the "Alerts" array. @@ -1620,14 +1620,14 @@ static cl_error_t append_virus(cli_ctx *ctx, const char *virname, IndicatorType status = CL_EMEM; goto done; } - json_object_object_add(ctx->this_layer_metadata_json, "Alerts", arrobj); + (void)json_object_object_add(ctx->this_layer_metadata_json, "Alerts", arrobj); } // Increment the indicator_obj reference count, so that it can be added to the "Alerts" array. - json_object_get(indicator_obj); + (void)json_object_get(indicator_obj); // Add the same indicator object to the "Alerts" array. - json_object_array_add(arrobj, indicator_obj); + (void)json_object_array_add(arrobj, indicator_obj); } } diff --git a/libclamav/pdfdecode.c b/libclamav/pdfdecode.c index 139e4f129..49355d30f 100644 --- a/libclamav/pdfdecode.c +++ b/libclamav/pdfdecode.c @@ -893,6 +893,7 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj, uint8_t *content = (uint8_t *)token->content; uint32_t length = token->length; lzw_stream stream; + bool stream_initialized = false; int echg = 1, lzwstat, rc = CL_SUCCESS; if (pdf->ctx && !(pdf->ctx->dconf->other & OTHER_CONF_LZW)) { @@ -942,6 +943,7 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj, rc = CL_EMEM; goto done; } + stream_initialized = true; memset(&stream, 0, sizeof(stream)); stream.next_in = content; @@ -1066,9 +1068,11 @@ static cl_error_t filter_lzwdecode(struct pdf_struct *pdf, struct pdf_obj *obj, break; } - (void)lzwInflateEnd(&stream); - done: + if (stream_initialized) { + (void)lzwInflateEnd(&stream); + } + if (rc == CL_SUCCESS) { if (declen == 0) { cli_dbgmsg("cli_pdf: empty stream after inflation completed.\n"); diff --git a/libclamav/rtf.c b/libclamav/rtf.c index b263c5a4d..e5614889b 100644 --- a/libclamav/rtf.c +++ b/libclamav/rtf.c @@ -279,8 +279,10 @@ static int rtf_object_process(struct rtf_state* state, const unsigned char* inpu return 0; if (data->has_partial) { - for (i = 0; i < len && !isxdigit(input[i]); i++) - ; + for (i = 0; i < len && !isxdigit(input[i]); i++) { + continue; + }; + if (i < len) { outdata[out_cnt++] = data->partial | hextable[input[i++]]; data->has_partial = 0; @@ -532,8 +534,10 @@ int cli_scanrtf(cli_ctx* ctx) return CL_EMEM; } - if (!(tempname = cli_gentemp_with_prefix(ctx->this_layer_tmpdir, "rtf-tmp"))) + if (!(tempname = cli_gentemp_with_prefix(ctx->this_layer_tmpdir, "rtf-tmp"))) { + free(stack.states); return CL_EMEM; + } if (mkdir(tempname, 0700)) { cli_dbgmsg("ScanRTF -> Can't create temporary directory %s\n", tempname); diff --git a/libclamav/scanners.c b/libclamav/scanners.c index 7d44b1abd..21db3ba17 100644 --- a/libclamav/scanners.c +++ b/libclamav/scanners.c @@ -1826,8 +1826,8 @@ static cl_error_t cli_ole2_tempdir_scan_vba(const char *dir, cli_ctx *ctx, struc cl_error_t ret; int i, j; size_t data_len; - vba_project_t *vba_project; - char *fullname = NULL; + vba_project_t *vba_project = NULL; + char *fullname = NULL; char vbaname[1024]; unsigned char *data = NULL; char *hash; @@ -2024,6 +2024,10 @@ done: free(proj_contents_fname); } + if (NULL != vba_project) { + cli_free_vba_project(vba_project); + } + if (NULL != data) { free(data); } diff --git a/libclamav/unarj.c b/libclamav/unarj.c index c5ad3fa54..e83c2fe78 100644 --- a/libclamav/unarj.c +++ b/libclamav/unarj.c @@ -1209,11 +1209,8 @@ cl_error_t cli_unarj_header_check( } else if (ret == CL_BREAK) { cli_dbgmsg("cli_unarj_header_check: End of archive\n"); - status = CL_BREAK; - } else { cli_dbgmsg("cli_unarj_header_check: Error reading file header: %s\n", cl_strerror(ret)); - status = ret; } CLI_FREE_AND_SET_NULL(metadata.filename); diff --git a/libclamav/unzip.c b/libclamav/unzip.c index c1fb87055..90920ec5b 100644 --- a/libclamav/unzip.c +++ b/libclamav/unzip.c @@ -1147,11 +1147,6 @@ cl_error_t index_the_central_directory( } } while (1); - if (ret == CL_VIRUS) { - status = CL_VIRUS; - goto done; - } - if (records_count > 1) { /* * Sort the records by local file offset @@ -1463,7 +1458,6 @@ cl_error_t index_local_file_headers( struct zip_record *prev_record = NULL; size_t local_file_headers_count = 0; uint32_t num_overlapping_files = 0; - bool exceeded_max_files = false; if (NULL == catalogue || NULL == num_records || NULL == *catalogue) { cli_dbgmsg("index_local_file_headers: Invalid NULL arguments\n"); @@ -1505,8 +1499,6 @@ cl_error_t index_local_file_headers( if (ctx->engine->maxfiles && total_files_found >= ctx->engine->maxfiles) { cli_dbgmsg("cli_unzip: Files limit reached (max: %u)\n", ctx->engine->maxfiles); cli_append_potentially_unwanted_if_heur_exceedsmax(ctx, "Heuristics.Limits.Exceeded.MaxFiles"); - exceeded_max_files = true; // Set a bool so we can return the correct status code later. - // We still need to scan the files we found while reviewing the file records up to this limit. break; } @@ -1611,10 +1603,12 @@ cl_error_t index_local_file_headers( cli_dbgmsg(" current file start: %u\n", curr_record->local_header_offset); if (ZIP_MAX_NUM_OVERLAPPING_FILES < num_overlapping_files) { + status = CL_EFORMAT; if (SCAN_HEURISTICS) { - status = cli_append_potentially_unwanted(ctx, "Heuristics.Zip.OverlappingFiles"); - } else { - status = CL_EFORMAT; + ret = cli_append_potentially_unwanted(ctx, "Heuristics.Zip.OverlappingFiles"); + if (CL_SUCCESS != ret) { + status = ret; + } } goto done; } @@ -1631,8 +1625,11 @@ cl_error_t index_local_file_headers( free(temp_catalogue); temp_catalogue = NULL; + free(*catalogue); - *catalogue = combined_catalogue; + *catalogue = combined_catalogue; + combined_catalogue = NULL; + *num_records = total_files_found; } else { free(temp_catalogue); @@ -1654,34 +1651,30 @@ done: free(*catalogue); *catalogue = NULL; } + } - if (NULL != temp_catalogue) { - size_t i; - for (i = 0; i < local_file_headers_count; i++) { - if (NULL != temp_catalogue[i].original_filename) { - free(temp_catalogue[i].original_filename); - temp_catalogue[i].original_filename = NULL; - } + if (NULL != temp_catalogue) { + size_t i; + for (i = 0; i < local_file_headers_count; i++) { + if (NULL != temp_catalogue[i].original_filename) { + free(temp_catalogue[i].original_filename); + temp_catalogue[i].original_filename = NULL; } - free(temp_catalogue); - temp_catalogue = NULL; } + free(temp_catalogue); + temp_catalogue = NULL; + } - if (NULL != combined_catalogue) { - size_t i; - for (i = 0; i < total_files_found; i++) { - if (NULL != combined_catalogue[i].original_filename) { - free(combined_catalogue[i].original_filename); - combined_catalogue[i].original_filename = NULL; - } + if (NULL != combined_catalogue) { + size_t i; + for (i = 0; i < total_files_found; i++) { + if (NULL != combined_catalogue[i].original_filename) { + free(combined_catalogue[i].original_filename); + combined_catalogue[i].original_filename = NULL; } - free(combined_catalogue); - combined_catalogue = NULL; - } - - if (exceeded_max_files) { - status = CL_EMAXFILES; } + free(combined_catalogue); + combined_catalogue = NULL; } return status; diff --git a/libclamav/upx.c b/libclamav/upx.c index a01636023..ad5e57b23 100644 --- a/libclamav/upx.c +++ b/libclamav/upx.c @@ -360,7 +360,7 @@ int upx_inflate2b(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0); if (oob == -1) return -1; - if (backsize + 2 > UINT32_MAX) + if (backsize > UINT32_MAX - 2) return -1; backsize += 2; } @@ -455,7 +455,7 @@ int upx_inflate2d(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0); if (oob == -1) return -1; - if (backsize + 2 > UINT32_MAX) + if (backsize > UINT32_MAX - 2) return -1; backsize += 2; } @@ -554,7 +554,7 @@ int upx_inflate2e(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u } while ((oob = doubleebx(src, &myebx, &scur, ssize)) == 0); if (oob == -1) return -1; - if (backsize + 2 > UINT32_MAX) + if (backsize > UINT32_MAX - 2) return -1; backsize += 2; } @@ -563,7 +563,7 @@ int upx_inflate2e(const char *src, uint32_t ssize, char *dst, uint32_t *dsize, u if ((uint32_t)unp_offset < 0xfffffb00) backsize++; - if (backsize + 2 > UINT32_MAX) + if (backsize > UINT32_MAX - 2) return -1; backsize += 2; diff --git a/libclamav/vba_extract.c b/libclamav/vba_extract.c index f81378e8c..0c9625b8b 100644 --- a/libclamav/vba_extract.c +++ b/libclamav/vba_extract.c @@ -2075,6 +2075,7 @@ word_read_macro_info(int fd, macro_info_t *macro_info) } if (!word_read_macro_entry(fd, macro_info)) { free(macro_info->entries); + macro_info->entries = NULL; macro_info->count = 0; return NULL; } diff --git a/sigtool/sigtool.c b/sigtool/sigtool.c index 9d76e43f7..b1c7cb1b4 100644 --- a/sigtool/sigtool.c +++ b/sigtool/sigtool.c @@ -517,10 +517,10 @@ done: if (NULL != ctx->options) { free(ctx->options); } - if (ctx->recursion_stack[ctx->recursion_level].evidence) { - evidence_free(ctx->recursion_stack[ctx->recursion_level].evidence); - } if (NULL != ctx->recursion_stack) { + if (ctx->recursion_stack[ctx->recursion_level].evidence) { + evidence_free(ctx->recursion_stack[ctx->recursion_level].evidence); + } free(ctx->recursion_stack); } free(ctx);