From c9adeff907de11ceb9af84d4207b5a1f617e27af Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Mon, 17 Apr 2023 11:39:18 -0700 Subject: [PATCH 01/14] Coverity: fix assorted static analysis issues RTF: - Coverity-344490: Use cli_realloc instead of cli_realloc2. cli_realloc2 will free the memory if the allocation fails, though we also free the memory later in SCAN_CLEANUP. - Fix warning about unused variable. AutoIt: - Fix possible memory leaks of input and output buffers. - Set pointer to NULL after handing off memory to new pointer. --- libclamav/autoit.c | 14 +++++++++++++- libclamav/rtf.c | 6 ++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/libclamav/autoit.c b/libclamav/autoit.c index d563f01b20..10b1a96d50 100644 --- a/libclamav/autoit.c +++ b/libclamav/autoit.c @@ -761,6 +761,10 @@ static cl_error_t ea05(cli_ctx *ctx, const uint8_t *base, char *tmpd) cli_dbgmsg("autoit: file is compressed\n"); if (cli_readint32(UNP.inputbuf) != 0x35304145) { cli_dbgmsg("autoit: bad magic or unsupported version\n"); + // Free this inputbuf and set back to NULL. + free(UNP.inputbuf); + UNP.inputbuf = NULL; + continue; } @@ -769,6 +773,10 @@ static cl_error_t ea05(cli_ctx *ctx, const uint8_t *base, char *tmpd) } if (cli_checklimits("autoit", ctx, UNP.usize, 0, 0) != CL_CLEAN) { + // Free this inputbuf and set back to NULL. + free(UNP.inputbuf); + UNP.inputbuf = NULL; + continue; } @@ -848,12 +856,16 @@ static cl_error_t ea05(cli_ctx *ctx, const uint8_t *base, char *tmpd) */ cli_dbgmsg("autoit: file is not compressed\n"); UNP.outputbuf = UNP.inputbuf; - UNP.usize = UNP.csize; + UNP.inputbuf = NULL; + + UNP.usize = UNP.csize; } if (UNP.usize < 4) { cli_dbgmsg("autoit: file is too short\n"); free(UNP.outputbuf); + UNP.outputbuf = NULL; + continue; } diff --git a/libclamav/rtf.c b/libclamav/rtf.c index 041df7a031..c3ca1c9c43 100644 --- a/libclamav/rtf.c +++ b/libclamav/rtf.c @@ -167,9 +167,11 @@ static int push_state(struct stack* stack, struct rtf_state* state) /* grow stack */ struct rtf_state* states; stack->stack_size += 128; - states = cli_realloc2(stack->states, stack->stack_size * sizeof(*stack->states)); - if (!states) + states = cli_realloc(stack->states, stack->stack_size * sizeof(*stack->states)); + if (!states) { + // Realloc failed. Note that stack->states has not been freed and must still be cleaned up by the caller. return CL_EMEM; + } stack->states = states; } stack->states[stack->stack_cnt++] = *state; From a69f803b1b29d60243bd83d5f255f2ff87d9e3da Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 11:57:40 -0700 Subject: [PATCH 02/14] Assorted unit test Coverity fixes Coverity-344508: Fix out-of-bound read in check_str test. The len argument cannot be longer than the size of the source buffer. The original test was attempting to test an append failure. The updated test checks for correct behavior with two consecutive appends. Also added function comments to document correct use of textbuffer functions. Coverity-344493: Fix out-of-bounds read in check_jsnorm test. The buffers passed to tokenizer_test must be NULL-terminated. --- libclamav/jsparse/textbuf.h | 36 ++++++++++++++++++++++++++++++++++++ unit_tests/check_jsnorm.c | 8 ++++++-- unit_tests/check_str.c | 4 ++-- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/libclamav/jsparse/textbuf.h b/libclamav/jsparse/textbuf.h index b2d0e7bdd5..951c11bd87 100644 --- a/libclamav/jsparse/textbuf.h +++ b/libclamav/jsparse/textbuf.h @@ -27,6 +27,18 @@ struct text_buffer { size_t capacity; }; +/** + * @brief If the provided text_buffer capacity is smaller than the requested len, + * then resize the text_buffer to be at least `len` bytes in size. + * + * Note: If a resize is required, it will allocate an additional 4096 bytes, minimum. + * + * Safety: Will NOT free the text_buffer data if the realloc fails! + * + * @param txtbuf + * @param len + * @return int + */ static inline int textbuffer_ensure_capacity(struct text_buffer *txtbuf, size_t len) { if (txtbuf->pos + len > txtbuf->capacity) { @@ -41,6 +53,16 @@ static inline int textbuffer_ensure_capacity(struct text_buffer *txtbuf, size_t return 0; } +/** + * @brief Append bytes from source `s` to the data in text_buffer `txtbuf`. Reallocate to a larger buf as needed. + * + * Safety: `s` must be at least `len` bytes in length. + * + * @param txtbuf The destination text_buffer. + * @param s Pointer to the source data. + * @param len The number of bytes to copy from `s` to append to `txtbuf` + * @return int 0 on success. -1 on failure + */ static inline int textbuffer_append_len(struct text_buffer *txtbuf, const char *s, size_t len) { if (textbuffer_ensure_capacity(txtbuf, len) == -1) @@ -50,12 +72,26 @@ static inline int textbuffer_append_len(struct text_buffer *txtbuf, const char * return 0; } +/** + * @brief A wrapper around textbuffer_append_len() for source buffers that are NULL-terminated strings. + * + * @param txtbuf The destination text_buffer. + * @param s Pointer to the source data. + * @return int 0 on success. -1 on failure + */ static inline int textbuffer_append(struct text_buffer *txtbuf, const char *s) { size_t len = strlen(s); return textbuffer_append_len(txtbuf, s, len); } +/** + * @brief Append a single cahracter from source `c` to the data in text_buffer `txtbuf`. Reallocate to a larger buf as needed. + * + * @param txtbuf The destination text_buffer. + * @param c Pointer to the source data. + * @return int 0 on success. -1 on failure + */ static inline int textbuffer_putc(struct text_buffer *txtbuf, const char c) { if (textbuffer_ensure_capacity(txtbuf, 1) == -1) diff --git a/unit_tests/check_jsnorm.c b/unit_tests/check_jsnorm.c index 6a16a8d1fa..64934023dc 100644 --- a/unit_tests/check_jsnorm.c +++ b/unit_tests/check_jsnorm.c @@ -398,18 +398,22 @@ START_TEST(js_buffer) const char s_exp[] = ""; char *tst = malloc(len); - char *exp = malloc(len + sizeof(s_exp) + sizeof(e_exp) - 2); + + const size_t explen = len + sizeof(s_exp) + sizeof(e_exp) - 2; + char *exp = malloc(len + sizeof(s_exp) + sizeof(e_exp) - 2); ck_assert_msg(!!tst, "malloc"); ck_assert_msg(!!exp, "malloc"); - memset(tst, 'a', len); + memset(tst, 'a', len - 1); strncpy(tst, s, strlen(s)); strncpy(tst + len - sizeof(e), e, sizeof(e)); + tst[len - 1] = '\0'; strncpy(exp, s_exp, len); strncpy(exp + sizeof(s_exp) - 1, tst, len - 1); strncpy(exp + sizeof(s_exp) + len - 2, e_exp, sizeof(e_exp)); + exp[explen - 1] = '\0'; tokenizer_test(tst, exp, 1); free(exp); diff --git a/unit_tests/check_str.c b/unit_tests/check_str.c index 54722370f0..d3bb93d5cc 100644 --- a/unit_tests/check_str.c +++ b/unit_tests/check_str.c @@ -117,8 +117,8 @@ START_TEST(test_append_len) ck_assert_msg(textbuffer_append_len(&buf, "test", 3) != -1, "tbuf append"); ck_assert_msg(buf.data && !strncmp(buf.data, "tes", 3), "textbuffer_append_len"); errmsg_expected(); - ck_assert_msg(textbuffer_append_len(&buf, "test", CLI_MAX_ALLOCATION) == -1, "tbuf append"); - ck_assert_msg(buf.data && !strncmp(buf.data, "tes", 3), "textbuffer_append_len"); + ck_assert_msg(textbuffer_append_len(&buf, "TEST", 4) != -1, "tbuf append"); + ck_assert_msg(buf.data && !strncmp(buf.data, "tesTEST", 4), "textbuffer_append_len"); } END_TEST From b222fc214ab4d91a0142b348b6c831f835502d1c Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 12:34:16 -0700 Subject: [PATCH 03/14] Coverity-396088: Fix possible use of uninitialized variable in msxml parser. --- libclamav/msxml_parser.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libclamav/msxml_parser.c b/libclamav/msxml_parser.c index 39e69cb0bc..199c4287de 100644 --- a/libclamav/msxml_parser.c +++ b/libclamav/msxml_parser.c @@ -579,6 +579,8 @@ cl_error_t cli_msxml_parse_document(cli_ctx *ctx, xmlTextReaderPtr reader, const if (!ictx.root) ictx.flags &= ~MSXML_FLAG_JSON; ictx.toval = 0; + } else { + ictx.root = NULL; } #else ictx.flags &= ~MSXML_FLAG_JSON; From 8c3917476dab9d581ff25a5bd0258d3820e589a6 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 12:41:04 -0700 Subject: [PATCH 04/14] Coverity-401432: Correctly initialize file descriptor in HFS+ parser If not initialized, it could try to close some random file descriptor. --- libclamav/hfsplus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libclamav/hfsplus.c b/libclamav/hfsplus.c index 3ab142c6df..6b5476e844 100644 --- a/libclamav/hfsplus.c +++ b/libclamav/hfsplus.c @@ -322,7 +322,7 @@ static cl_error_t hfsplus_scanfile(cli_ctx *ctx, hfsPlusVolumeHeader *volHeader, hfsPlusExtentDescriptor *currExt; const uint8_t *mPtr = NULL; char *tmpname = NULL; - int ofd; + int ofd = -1; uint64_t targetSize; uint32_t outputBlocks = 0; uint8_t ext; From 736fe01f23b434907397dd8b59e9f7b65f604d4c Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 13:37:54 -0700 Subject: [PATCH 05/14] Coverity-396111: Fix possibly unitialized binop variable in bytecode module Fix possibly unitialized binop variable in bytecode module for STORE and COPY instructions in bytecode module. Refactored slightly to include additional opcode login in the switch statement. --- libclamav/bytecode.c | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/libclamav/bytecode.c b/libclamav/bytecode.c index 89df87280e..a94ee5320f 100644 --- a/libclamav/bytecode.c +++ b/libclamav/bytecode.c @@ -1258,6 +1258,10 @@ static cl_error_t parseBB(struct cli_bc *bc, unsigned func, unsigned bb, unsigne BB->insts = &bcfunc->allinsts[bcfunc->insn_idx]; while (!last) { unsigned numOp; + + // Initialize instruction to zero + memset(&inst, 0, sizeof(inst)); + if (buffer[offset] == 'T') { last = 1; offset++; @@ -1355,6 +1359,33 @@ static cl_error_t parseBB(struct cli_bc *bc, unsigned func, unsigned bb, unsigne inst.u.ops.ops[i] = readOperand(bcfunc, buffer, &offset, len, &ok); } break; + case OP_BC_STORE: + numOp = operand_counts[inst.opcode]; + if (2 != numOp) { + // invalid number of operands + cli_errmsg("Invalid number of operands (%u) for OP_BC_STORE opcode\n", numOp); + return CL_EMALFDB; + } + inst.u.binop[0] = readOperand(bcfunc, buffer, &offset, len, &ok); + inst.u.binop[1] = readOperand(bcfunc, buffer, &offset, len, &ok); + + int16_t t = get_optype(bcfunc, inst.u.binop[0]); + if (t) { + inst.type = t; + } + break; + case OP_BC_COPY: + numOp = operand_counts[inst.opcode]; + if (2 != numOp) { + // invalid number of operands + cli_errmsg("Invalid number of operands (%u) for OP_BC_COPY opcode\n", numOp); + return CL_EMALFDB; + } + inst.u.binop[0] = readOperand(bcfunc, buffer, &offset, len, &ok); + inst.u.binop[1] = readOperand(bcfunc, buffer, &offset, len, &ok); + + inst.type = get_optype(bcfunc, inst.u.binop[1]); + break; case OP_BC_ICMP_EQ: case OP_BC_ICMP_NE: case OP_BC_ICMP_UGT: @@ -1391,22 +1422,18 @@ static cl_error_t parseBB(struct cli_bc *bc, unsigned func, unsigned bb, unsigne break; } } - if (inst.opcode == OP_BC_STORE) { - int16_t t = get_optype(bcfunc, inst.u.binop[0]); - if (t) - inst.type = t; - } - if (inst.opcode == OP_BC_COPY) - inst.type = get_optype(bcfunc, inst.u.binop[1]); + if (!ok) { cli_errmsg("Invalid instructions or operands\n"); return CL_EMALFDB; } + if (bcfunc->insn_idx + BB->numInsts >= bcfunc->numInsts) { cli_errmsg("More instructions than declared in total: %u > %u!\n", bcfunc->insn_idx + BB->numInsts, bcfunc->numInsts); return CL_EMALFDB; } + inst.interp_op = inst.opcode * 5; if (inst.type > 1) { if (inst.type <= 8) From e5bef6d7b8e5820189a34db8b830c1de71372ab8 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 14:26:04 -0700 Subject: [PATCH 06/14] Coverity-396100: Initialize url flags variable in phishcheck module. --- libclamav/phishcheck.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libclamav/phishcheck.c b/libclamav/phishcheck.c index 77d768fde2..5162f8b7f1 100644 --- a/libclamav/phishcheck.c +++ b/libclamav/phishcheck.c @@ -211,6 +211,7 @@ static void url_check_init(struct url_check* urls) string_init_c(&urls->realLink, NULL); string_init_c(&urls->displayLink, NULL); string_init_c(&urls->pre_fixup.pre_displayLink, NULL); + urls->flags = 0; } /* string reference counting implementation, From dcb56adb2e4f2341ac3ef6b8fd7536af0996303d Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 14:38:32 -0700 Subject: [PATCH 07/14] Coverity-396093: Correctly initialize zlib stream structure in vba_extract module. --- libclamav/vba_extract.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libclamav/vba_extract.c b/libclamav/vba_extract.c index 5ccf19367c..31016b9bc5 100644 --- a/libclamav/vba_extract.c +++ b/libclamav/vba_extract.c @@ -1805,6 +1805,8 @@ ppt_unlzw(const char *dir, int fd, uint32_t length) return FALSE; } + memset(&stream, 0, sizeof(stream)); + stream.zalloc = Z_NULL; stream.zfree = Z_NULL; stream.opaque = (void *)NULL; From 9694cad26781516ab831f4f72a57bb3b5b809fef Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 14:43:06 -0700 Subject: [PATCH 08/14] Coverity-396091: Initialize host array in clamav-milter netcode. --- clamav-milter/netcode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clamav-milter/netcode.c b/clamav-milter/netcode.c index a5b94df50c..cd450be433 100644 --- a/clamav-milter/netcode.c +++ b/clamav-milter/netcode.c @@ -466,7 +466,8 @@ int islocalnet_name(char *name) int islocalnet_sock(struct sockaddr *sa) { - uint32_t host[4], family; + uint32_t host[4] = {0}; + uint32_t family; if (!lnet) return 0; From 8745c10ccac12a08b5893ac9b2e9bade91a58914 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Tue, 18 Apr 2023 14:53:57 -0700 Subject: [PATCH 09/14] Coverity-344513: Fix use-after-free in unit test error condition. --- unit_tests/check_clamav.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unit_tests/check_clamav.c b/unit_tests/check_clamav.c index 045591ed66..61ecd8ad37 100644 --- a/unit_tests/check_clamav.c +++ b/unit_tests/check_clamav.c @@ -2055,7 +2055,9 @@ int main(void) srunner_set_log(sr, OBJDIR PATHSEP "test.log"); if (freopen(OBJDIR PATHSEP "test-stderr.log", "w+", stderr) == NULL) { - fputs("Unable to redirect stderr!\n", stderr); + // The stderr FILE pointer may be closed by `freopen()` even if redirecting to the log file files. + // So we will output the error message to stdout instead. + fputs("Unable to redirect stderr!\n", stdout); } cl_debug(); From 84bf2c7d42c4ce24aa916ce6e3f2c20b070b3d0e Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 19 Apr 2023 16:10:16 -0700 Subject: [PATCH 10/14] Coverity and OSS-Fuzz fixes in PDF module Prevent double-extraction of same PDF object Two indirect references to the same PDF object may cause it to try to extract that object twice. This also may cause it to set the extraction path twice, which leaks the memory from the first time. This commit records when object extraction is attempted and prevents doing it again. It also adds a couple extra checks to make sure that the object path string is not leaked. Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58072 Also: - Coverity-317959: Fix complaint about logically dead code. No need to check if UE variable is NULL because we would've returned earlier if it was NULL. - A bunch of medium-severity coverity issues for PDF parser regarding checking if a `pdf` pointer is NULL after dereferencing it. - Coverity-192930: bytes_remaining was being checked twice in a row without chainging it. Turns out we should have been changing it after moving the `index` pointer. - Coverity-192920: Switch to use CLI_REALLOC instead of cli_realloc2. This is because cli_realloc2 would free `pdf->objs` on failure and we still need it. --- libclamav/pdf.c | 151 ++++++++++++++++++++++++++++++------------------ libclamav/pdf.h | 1 + 2 files changed, 96 insertions(+), 56 deletions(-) diff --git a/libclamav/pdf.c b/libclamav/pdf.c index 8d824d13dd..85da7487c3 100644 --- a/libclamav/pdf.c +++ b/libclamav/pdf.c @@ -480,12 +480,9 @@ int pdf_findobj_in_objstm(struct pdf_struct *pdf, struct objstm_struct *objstm, /* Success! Add the object to the list of all objects found. */ pdf->nobjs++; - pdf->objs = cli_realloc2(pdf->objs, sizeof(struct pdf_obj *) * pdf->nobjs); - if (!pdf->objs) { - cli_warnmsg("pdf_findobj_in_objstm: out of memory finding objects in stream\n"); - status = CL_EMEM; - goto done; - } + CLI_REALLOC(pdf->objs, sizeof(struct pdf_obj *) * pdf->nobjs, + cli_warnmsg("pdf_findobj_in_objstm: out of memory finding objects in stream\n"), + status = CL_EMEM); pdf->objs[pdf->nobjs - 1] = obj; *obj_found = obj; @@ -548,11 +545,7 @@ cl_error_t pdf_findobj(struct pdf_struct *pdf) goto done; } pdf->nobjs++; - pdf->objs = cli_realloc2(pdf->objs, sizeof(struct pdf_obj *) * pdf->nobjs); - if (!pdf->objs) { - status = CL_EMEM; - goto done; - } + CLI_REALLOC(pdf->objs, sizeof(struct pdf_obj *) * pdf->nobjs, status = CL_EMEM); obj = malloc(sizeof(struct pdf_obj)); if (!obj) { @@ -927,6 +920,8 @@ static size_t find_length(struct pdf_struct *pdf, struct pdf_obj *obj, const cha if (!index) return 0; + bytes_remaining -= index - obj_start; + if (bytes_remaining < 1) { return 0; } @@ -1049,11 +1044,16 @@ static int run_pdf_hooks(struct pdf_struct *pdf, enum pdf_phase phase, int fd, i { int ret; struct cli_bc_ctx *bc_ctx; - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; fmap_t *map; UNUSEDPARAM(dumpid); + if (NULL == pdf) + return CL_EARG; + + ctx = pdf->ctx; + bc_ctx = cli_bytecode_context_alloc(); if (!bc_ctx) { cli_errmsg("run_pdf_hooks: can't allocate memory for bc_ctx\n"); @@ -1444,6 +1444,14 @@ cl_error_t pdf_extract_obj(struct pdf_struct *pdf, struct pdf_obj *obj, uint32_t return CL_SUCCESS; } + if (obj->extracted) { + // Should not attempt to extract the same object more than once. + return CL_SUCCESS; + } + // We're not done yet, but this is enough to say we've tried. + // Trying again won't help any. + obj->extracted = true; + if (obj->objstm) { cli_dbgmsg("pdf_extract_obj: extracting obj found in objstm.\n"); if (obj->objstm->streambuf == NULL) { @@ -1482,8 +1490,11 @@ cl_error_t pdf_extract_obj(struct pdf_struct *pdf, struct pdf_obj *obj, uint32_t return CL_ETMPFILE; } - if (!(flags & PDF_EXTRACT_OBJ_SCAN)) - obj->path = strdup(fullname); + if (!(flags & PDF_EXTRACT_OBJ_SCAN)) { + if (NULL != obj->path) { + obj->path = strdup(fullname); + } + } if ((NULL == obj->objstm) && (obj->flags & (1 << OBJ_STREAM))) { @@ -2919,7 +2930,7 @@ static void check_user_password(struct pdf_struct *pdf, int R, const char *O, size_t UE_len; compute_hash_r6(password, pwlen, (const unsigned char *)(U + 40), hash); - UE_len = UE ? strlen(UE) : 0; + UE_len = strlen(UE); if (UE_len != 32) { cli_dbgmsg("check_user_password: UE length is not 32: %zu\n", UE_len); noisy_warnmsg("check_user_password: UE length is not 32: %zu\n", UE_len); @@ -3727,6 +3738,10 @@ cl_error_t cli_pdf(const char *dir, cli_ctx *ctx, off_t offset) if (NULL != pdf.objs) { for (i = 0; i < pdf.nobjs; i++) { if (NULL != pdf.objs[i]) { + if (NULL != pdf.objs[i]->path) { + free(pdf.objs[i]->path); + pdf.objs[i]->path = NULL; + } free(pdf.objs[i]); pdf.objs[i] = NULL; } @@ -3849,7 +3864,7 @@ static void ASCIIHexDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struc UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nasciihexdecode++; @@ -3862,7 +3877,7 @@ static void ASCII85Decode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nascii85decode++; @@ -3875,7 +3890,7 @@ static void EmbeddedFile_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nembeddedfile++; @@ -3888,7 +3903,7 @@ static void FlateDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct p UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nflate++; @@ -3901,7 +3916,7 @@ static void Image_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nimage++; @@ -3914,7 +3929,7 @@ static void LZWDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdf UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nlzw++; @@ -3927,7 +3942,7 @@ static void RunLengthDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, stru UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nrunlengthdecode++; @@ -3940,7 +3955,7 @@ static void CCITTFaxDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struc UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nfaxdecode++; @@ -3950,15 +3965,17 @@ static void CCITTFaxDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struc #if HAVE_JSON static void JBIG2Decode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; struct json_object *pdfobj, *jbig2arr; UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -3985,7 +4002,7 @@ static void DCTDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdf UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.ndctdecode++; @@ -3998,7 +4015,7 @@ static void JPXDecode_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdf UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.njpxdecode++; @@ -4011,7 +4028,7 @@ static void Crypt_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.ncrypt++; @@ -4024,7 +4041,7 @@ static void Standard_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfn UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nstandard++; @@ -4037,7 +4054,7 @@ static void Sig_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_a UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nsigned++; @@ -4066,7 +4083,7 @@ static void OpenAction_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pd UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nopenaction++; @@ -4079,7 +4096,7 @@ static void Launch_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfnam UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nlaunch++; @@ -4092,7 +4109,7 @@ static void Page_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_ UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.npage++; @@ -4102,13 +4119,15 @@ static void Page_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_ #if HAVE_JSON static void Author_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4130,13 +4149,15 @@ static void Author_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfnam #if HAVE_JSON static void Creator_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4158,13 +4179,15 @@ static void Creator_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfna #if HAVE_JSON static void ModificationDate_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4186,13 +4209,15 @@ static void ModificationDate_cb(struct pdf_struct *pdf, struct pdf_obj *obj, str #if HAVE_JSON static void CreationDate_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4214,13 +4239,15 @@ static void CreationDate_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct #if HAVE_JSON static void Producer_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4242,13 +4269,15 @@ static void Producer_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfn #if HAVE_JSON static void Title_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4270,13 +4299,15 @@ static void Title_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname #if HAVE_JSON static void Keywords_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4298,13 +4329,15 @@ static void Keywords_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfn #if HAVE_JSON static void Subject_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4329,7 +4362,7 @@ static void RichMedia_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdf UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nrichmedia++; @@ -4342,7 +4375,7 @@ static void AcroForm_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfn UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nacroform++; @@ -4355,7 +4388,7 @@ static void XFA_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_a UNUSEDPARAM(obj); UNUSEDPARAM(act); - if (!(pdf)) + if (NULL == pdf) return; pdf->stats.nxfa++; @@ -4365,7 +4398,7 @@ static void XFA_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_a #if HAVE_JSON static void Pages_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; struct pdf_array *array; const char *objstart = (obj->objstm) ? (const char *)(obj->start + obj->objstm->streambuf) : (const char *)(obj->start + pdf->map); @@ -4381,6 +4414,8 @@ static void Pages_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname if (!(pdf) || !(pdf->ctx->wrkproperty)) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4444,7 +4479,7 @@ static void Pages_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname #if HAVE_JSON static void Colors_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfname_action *act) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; json_object *colorsobj, *pdfobj; unsigned long ncolors; long temp_long; @@ -4457,6 +4492,8 @@ static void Colors_cb(struct pdf_struct *pdf, struct pdf_obj *obj, struct pdfnam if (!(pdf) || !(pdf->ctx) || !(pdf->ctx->wrkproperty)) return; + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA)) return; @@ -4568,17 +4605,19 @@ static void pdf_free_stats(struct pdf_struct *pdf) #if HAVE_JSON static void pdf_export_json(struct pdf_struct *pdf) { - cli_ctx *ctx = pdf->ctx; + cli_ctx *ctx = NULL; json_object *pdfobj; unsigned long i; - if (!(pdf)) + if (NULL == pdf) return; if (!(pdf->ctx)) { goto cleanup; } + ctx = pdf->ctx; + if (!(SCAN_COLLECT_METADATA) || !(pdf->ctx->wrkproperty)) { goto cleanup; } diff --git a/libclamav/pdf.h b/libclamav/pdf.h index 8c317dfd88..3d53d5b7a4 100644 --- a/libclamav/pdf.h +++ b/libclamav/pdf.h @@ -49,6 +49,7 @@ struct pdf_obj { size_t stream_size; // size of stream contained in object. struct objstm_struct *objstm; // Should be NULL unless the obj exists in an object stream (separate buffer) char *path; + bool extracted; // We've attempted to extract this object. Check to prevent doing it more than once! }; enum pdf_array_type { PDF_ARR_UNKNOWN = 0, From 2d31e0c23b69b1d40a4cc49bfe56009ea2b2d805 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 19 Apr 2023 17:50:22 -0700 Subject: [PATCH 11/14] Assorted clamd unit test Coverity fixes Coverity-344510: Fix unitialized sock variable in check_clamd test program. Only close the socket on error if is a valid file descriptor. Coverity-344507: Remove unused file-open from clamd test. Coverity-344497: clamd test recvpartial convenience function is was reusing the `len` variable used for "how long is the reply" also as the buffer length. Coverity appears to be confused here and thinks that the length of the buffer may not be long enough for the NULL terminating character. I have reworked this to use a separate variable for managing the length of the buffer. --- unit_tests/check_clamd.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/unit_tests/check_clamd.c b/unit_tests/check_clamd.c index b80edf5968..114defa585 100644 --- a/unit_tests/check_clamd.c +++ b/unit_tests/check_clamd.c @@ -224,23 +224,30 @@ static struct basic_test { static void *recvpartial(int sd, size_t *len, int partial) { - char *buf = NULL; - size_t off = 0; - int rc; + char *buf = NULL; + size_t buf_size = 0; + size_t off = 0; + ssize_t bread; *len = 0; + do { - if (off + BUFSIZ > *len) { - *len += BUFSIZ + 1; - buf = realloc(buf, *len); + if (off + BUFSIZ > buf_size) { + buf_size += BUFSIZ + 1; + buf = realloc(buf, buf_size); ck_assert_msg(!!buf, "Cannot realloc buffer\n"); } - rc = recv(sd, buf + off, BUFSIZ, 0); - ck_assert_msg(rc != -1, "recv() failed: %s\n", strerror(errno)); - off += rc; - } while (rc && (!partial || !memchr(buf, '\0', off))); - *len = off; - buf[*len] = '\0'; + bread = recv(sd, buf + off, BUFSIZ, 0); + ck_assert_msg(bread != -1, "recv() failed: %s\n", strerror(errno)); + + off += bread; + } while (bread && (!partial || !memchr(buf, '\0', off))); + + // Make sure the response buffer is NULL terminated. + // But note that off-1 will likely be a '\n' newline character, and we don't want to overwrite that. + buf[MIN(off, buf_size - 1)] = '\0'; + + *len = off; return buf; } @@ -516,7 +523,7 @@ static struct cmds { START_TEST(test_fildes) { char nreply[BUFSIZ], nsend[BUFSIZ]; - int fd = open(SCANFILE, O_RDONLY); + int fd; int closefd = 0; int singlemsg = 0; const struct cmds *cmd; @@ -672,6 +679,7 @@ START_TEST(test_connections) num_fds = MIN(rlim.rlim_cur - 5, 250); sock = malloc(sizeof(int) * num_fds); + memset(sock, -1, sizeof(int) * num_fds); ck_assert_msg(!!sock, "malloc failed\n"); @@ -681,8 +689,10 @@ START_TEST(test_connections) if (sockd == -1) { /* close the previous one, to leave space for one more connection */ i--; - close(sock[i]); - sock[i] = -1; + if (sock[i] > 0) { + close(sock[i]); + sock[i] = -1; + } num_fds = i; break; From d9c3da6152f74b5348c5fcbff031d75bbeb8b67b Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 19 Apr 2023 20:56:17 -0700 Subject: [PATCH 12/14] Resolve Coverity assignment of overlapping memory warnings Coverity is unhappy with the use of the EC32, cli_readint32, and cli_writeint32 macros (and the 64bit equivalents to potentially change the endianess of variables in place. It claims: overlapping_assignment: Assigning ... to ..., which have overlapping memory locations and different types. Using a temporary variable in between reading and writing should resolve these "high impact" complaints. Resolves: Coverity-225232. 225225, 225215, 225212, 225180, 225170, 225165, 225161, 225159. --- libclamav/pe.c | 36 +++++++++++++++++++++++++++--------- libclamav/upack.c | 6 ++++-- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/libclamav/pe.c b/libclamav/pe.c index 1aa2002ec8..cf0e7cb368 100644 --- a/libclamav/pe.c +++ b/libclamav/pe.c @@ -2331,9 +2331,12 @@ static inline int hash_impfns(cli_ctx *ctx, void **hashctx, uint32_t *impsz, str while ((num_fns < PE_MAXIMPORTS) && (fmap_readn(map, &thunk32, thuoff, sizeof(struct pe_image_thunk32)) == sizeof(struct pe_image_thunk32)) && (thunk32.u.Ordinal != 0)) { char *funcname = NULL; + uint32_t temp; + thuoff += sizeof(struct pe_image_thunk32); - thunk32.u.Ordinal = EC32(thunk32.u.Ordinal); + temp = EC32(thunk32.u.Ordinal); + thunk32.u.Ordinal = temp; if (!(thunk32.u.Ordinal & PE_IMAGEDIR_ORDINAL_FLAG32)) { offset = cli_rawaddr(thunk32.u.Function, peinfo->sections, peinfo->nsections, &err, fsize, peinfo->hdr_size); @@ -2367,9 +2370,14 @@ static inline int hash_impfns(cli_ctx *ctx, void **hashctx, uint32_t *impsz, str while ((num_fns < PE_MAXIMPORTS) && (fmap_readn(map, &thunk64, thuoff, sizeof(struct pe_image_thunk64)) == sizeof(struct pe_image_thunk64)) && (thunk64.u.Ordinal != 0)) { char *funcname = NULL; + + // Temporary variable so we don't have overlapping writes with the EC32 reads. + uint64_t temp; + thuoff += sizeof(struct pe_image_thunk64); - thunk64.u.Ordinal = EC64(thunk64.u.Ordinal); + temp = EC64(thunk64.u.Ordinal); + thunk64.u.Ordinal = temp; if (!(thunk64.u.Ordinal & PE_IMAGEDIR_ORDINAL_FLAG64)) { offset = cli_rawaddr(thunk64.u.Function, peinfo->sections, peinfo->nsections, &err, fsize, peinfo->hdr_size); @@ -2474,6 +2482,9 @@ static cl_error_t hash_imptbl(cli_ctx *ctx, unsigned char **digest, uint32_t *im while (left > sizeof(struct pe_image_import_descriptor) && nimps < PE_MAXIMPORTS) { char *dllname = NULL; + // Temporary variable so we don't have overlapping writes with the EC32 reads. + uint32_t temp; + /* Get copy of image import descriptor to work with */ memcpy(&image, impdes, sizeof(struct pe_image_import_descriptor)); @@ -2489,11 +2500,16 @@ static cl_error_t hash_imptbl(cli_ctx *ctx, unsigned char **digest, uint32_t *im impdes++; /* Endian Conversion */ - image.u.OriginalFirstThunk = EC32(image.u.OriginalFirstThunk); - image.TimeDateStamp = EC32(image.TimeDateStamp); - image.ForwarderChain = EC32(image.ForwarderChain); - image.Name = EC32(image.Name); - image.FirstThunk = EC32(image.FirstThunk); + temp = EC32(image.u.OriginalFirstThunk); + image.u.OriginalFirstThunk = temp; + temp = EC32(image.TimeDateStamp); + image.TimeDateStamp = temp; + temp = EC32(image.ForwarderChain); + image.ForwarderChain = temp; + temp = EC32(image.Name); + image.Name = temp; + temp = EC32(image.FirstThunk); + image.FirstThunk = temp; /* DLL name acquisition */ offset = cli_rawaddr(image.Name, peinfo->sections, peinfo->nsections, &err, fsize, peinfo->hdr_size); @@ -4508,7 +4524,7 @@ cl_error_t cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, uint32_t stored_opt_hdr_size; struct pe_image_file_hdr *file_hdr; struct pe_image_optional_hdr32 *opt32; - struct pe_image_optional_hdr64 *opt64 = NULL; + struct pe_image_optional_hdr64 *opt64 = NULL; struct pe_image_section_hdr *section_hdrs = NULL; size_t i, j, section_pe_idx; unsigned int err; @@ -4519,6 +4535,7 @@ cl_error_t cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, uint32_t is_exe = 0; int native = 0; size_t read; + uint32_t temp; #if HAVE_JSON int toval = 0; @@ -4557,7 +4574,8 @@ cl_error_t cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, goto done; } - peinfo->e_lfanew = EC32(peinfo->e_lfanew); + temp = EC32(peinfo->e_lfanew); + peinfo->e_lfanew = temp; if (opts & CLI_PEHEADER_OPT_DBG_PRINT_INFO) { cli_dbgmsg("e_lfanew == %d\n", peinfo->e_lfanew); } diff --git a/libclamav/upack.c b/libclamav/upack.c index 46bae6479e..3cfb480c9d 100644 --- a/libclamav/upack.c +++ b/libclamav/upack.c @@ -331,8 +331,10 @@ int unupack(int upack, char *dest, uint32_t dsize, char *buff, uint32_t vma, uin /* fix values */ if (!CLI_ISCONTAINED(dest, dsize, loc_ebx - 4, (12 + 4 * 4)) || !CLI_ISCONTAINED(dest, dsize, loc_esi + 0x24, 4) || !CLI_ISCONTAINED(dest, dsize, loc_esi + 0x40, 4)) return -1; - for (j = 2; j < 6; j++) - cli_writeint32(loc_ebx + (j << 2), cli_readint32(loc_ebx + (j << 2))); + for (j = 2; j < 6; j++) { + int32_t temp = cli_readint32(loc_ebx + (j << 2)); + cli_writeint32(loc_ebx + (j << 2), temp); + } paddr = dest + cli_readint32(loc_ebx - 4) - base; save1 = loc_ecx; pushed_esi = loc_edi; From 9af020198782dff06de4c6b714d7f0b805e07063 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 19 Apr 2023 21:09:48 -0700 Subject: [PATCH 13/14] Fix assorted Coverity warnings in EGG parser Coverity-225186, 225156: Fix possible leak of comment message in case parsing the comment header fails after allocating the comment buffer. Coverity-225184: Fix possible leak of egg block if the archive is not solid and contains no files. Additional improvements to egg parser error handling for functions that pass back allocated memory through the parameters. Instead of checking for failure before freeing the allocated memory, we'll hand off ownership of the allocated memory to the parameter variable by setting to NULL afterwards, and then always free the variable if not NULL after the `done` label. --- libclamav/egg.c | 96 +++++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 38 deletions(-) diff --git a/libclamav/egg.c b/libclamav/egg.c index 6ebca48c4d..046dd0a240 100644 --- a/libclamav/egg.c +++ b/libclamav/egg.c @@ -99,7 +99,7 @@ typedef uint16_t WCHAR; * general defines */ #define EOFARC 0x08E28222 /* Signals end of each header, or end of archive. */ -//#define EOFAR_ 0x2282E208 +// #define EOFAR_ 0x2282E208 /* * egg_header */ @@ -579,11 +579,12 @@ static cl_error_t egg_parse_encrypt_header(const uint8_t* index, size_t size, eg } *encryptInfo = encrypt; - status = CL_SUCCESS; + encrypt = NULL; -done: + status = CL_SUCCESS; - if (CL_SUCCESS != status) { +done: + if (NULL != encrypt) { egg_free_encrypt(encrypt); } @@ -608,8 +609,7 @@ static cl_error_t egg_parse_comment_header(const uint8_t* index, size_t size, ex /* * comment is encrypted, nothing to be done. */ - *commentInfo = cli_strdup(""); - status = CL_EUNPACK; + status = CL_EUNPACK; goto done; } @@ -637,9 +637,15 @@ static cl_error_t egg_parse_comment_header(const uint8_t* index, size_t size, ex cli_dbgmsg("egg_parse_comment_header: comment: %s\n", comment_utf8); *commentInfo = comment_utf8; - status = CL_SUCCESS; + comment_utf8 = NULL; + + status = CL_SUCCESS; done: + if (NULL != comment_utf8) { + free(comment_utf8); + } + return status; } @@ -739,14 +745,14 @@ static cl_error_t egg_parse_block_headers(egg_handle* handle, egg_block** block) eggBlock->compressedData = (char*)index; handle->offset += blockHeader->compress_size; - *block = eggBlock; + *block = eggBlock; + eggBlock = NULL; + status = CL_SUCCESS; done: - if (CL_SUCCESS != status) { - if (eggBlock) { - egg_free_egg_block(eggBlock); - } + if (NULL != eggBlock) { + egg_free_egg_block(eggBlock); } return status; @@ -1446,14 +1452,14 @@ static cl_error_t egg_parse_file_headers(egg_handle* handle, egg_file** file) } } - *file = eggFile; + *file = eggFile; + eggFile = NULL; + status = CL_SUCCESS; done: - if (CL_SUCCESS != status) { - if (eggFile) { - egg_free_egg_file(eggFile); - } + if (NULL != eggFile) { + egg_free_egg_file(eggFile); } return status; @@ -1607,6 +1613,10 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t return CL_EARG; } + *hArchive = NULL; + *comments = NULL; + *nComments = 0; + handle = (egg_handle*)cli_calloc(1, sizeof(egg_handle)); if (NULL == handle) { cli_errmsg("cli_egg_open: Failed to allocate memory for egg_handle.\n"); @@ -1721,6 +1731,7 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t if (handle->nFiles == 0) { cli_dbgmsg("cli_egg_open: No file found for block in non-solid archive.\n"); // TODO: create an unamed block. + egg_free_egg_block(found_block); } else { egg_block** blocks_tmp; @@ -1833,18 +1844,19 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t } } - *hArchive = handle; - *comments = handle->comments; + *comments = handle->comments; + handle->comments = NULL; + *nComments = handle->nComments; + *hArchive = handle; + handle = NULL; + status = CL_SUCCESS; done: - if (CL_SUCCESS != status) { - if (handle) { - egg_free_egg_handle(handle); - } - *hArchive = NULL; + if (NULL != handle) { + egg_free_egg_handle(handle); } return status; } @@ -2040,7 +2052,9 @@ cl_error_t cli_egg_deflate_decompress(char* compressed, size_t compressed_size, break; } - *decompressed = (char*)decoded; + *decompressed = (char*)decoded; + decoded = NULL; + *decompressed_size = declen; status = CL_SUCCESS; @@ -2051,7 +2065,7 @@ cl_error_t cli_egg_deflate_decompress(char* compressed, size_t compressed_size, (void)inflateEnd(&stream); } - if (CL_SUCCESS != status) { + if (NULL != decoded) { free(decoded); } @@ -2157,7 +2171,9 @@ cl_error_t cli_egg_bzip2_decompress(char* compressed, size_t compressed_size, ch break; } - *decompressed = (char*)decoded; + *decompressed = (char*)decoded; + decoded = NULL; + *decompressed_size = declen; status = CL_SUCCESS; @@ -2166,7 +2182,7 @@ cl_error_t cli_egg_bzip2_decompress(char* compressed, size_t compressed_size, ch (void)BZ2_bzDecompressEnd(&stream); - if (CL_SUCCESS != status) { + if (NULL != decoded) { free(decoded); } @@ -2274,7 +2290,9 @@ cl_error_t cli_egg_lzma_decompress(char* compressed, size_t compressed_size, cha break; } - *decompressed = (char*)decoded; + *decompressed = (char*)decoded; + decoded = NULL; + *decompressed_size = declen; status = CL_SUCCESS; @@ -2285,7 +2303,7 @@ cl_error_t cli_egg_lzma_decompress(char* compressed, size_t compressed_size, cha (void)cli_LzmaShutdown(&stream); } - if (CL_SUCCESS != status) { + if (NULL != decoded) { free(decoded); } @@ -2307,6 +2325,7 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha goto done; } + *filename = NULL; *output_buffer = NULL; *output_buffer_length = 0; @@ -2522,21 +2541,22 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha } cli_dbgmsg("cli_egg_extract_file: File extracted: %s\n", currFile->filename.name_utf8); - *filename = strdup(currFile->filename.name_utf8); - *output_buffer = decompressed; + *filename = strdup(currFile->filename.name_utf8); + + *output_buffer = decompressed; + decompressed = NULL; + *output_buffer_length = decompressed_size; - status = CL_SUCCESS; + + status = CL_SUCCESS; done: if (NULL != handle) { handle->fileExtractionIndex += 1; } - if (CL_SUCCESS != status) { - /* Free buffer */ - if (NULL != decompressed) { - free(decompressed); - } + if (NULL != decompressed) { + free(decompressed); } return status; From 231aeebd0aff77503efbe786afd03de50be4d54e Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Fri, 21 Apr 2023 12:41:38 -0700 Subject: [PATCH 14/14] EGG parser: Fix error handling when comment can't be converted to utf8 For some reason we're generating a filename wiith a random hash in it to use for the comment content in the event that codepage converstion to utf8 fails for the comment. This makes no sense. So I'm removing it and letting it just fail out. The calling functions ignore the failure anyways and move on which is good. Note: I think the "cli_genfname" call that I'm removing was a copypaste from the logic for converting the filename to utf8. We still do that. I'm not sure about the consequence of failing to have a filename in that case, so I'm going to leave it as-is. --- libclamav/egg.c | 220 +++++++++++++++++++----------------------------- 1 file changed, 86 insertions(+), 134 deletions(-) diff --git a/libclamav/egg.c b/libclamav/egg.c index 046dd0a240..d8121524d2 100644 --- a/libclamav/egg.c +++ b/libclamav/egg.c @@ -623,7 +623,8 @@ static cl_error_t egg_parse_comment_header(const uint8_t* index, size_t size, ex */ if (CL_SUCCESS != cli_codepage_to_utf8((char*)index, size, CODEPAGE_UTF8, &comment_utf8, &comment_utf8_size)) { cli_dbgmsg("egg_parse_comment_header: failed to convert codepage \"0\" to UTF-8\n"); - comment_utf8 = cli_genfname(NULL); + status = CL_EUNPACK; + goto done; } } else { /* Should already be UTF-8. Use as-is.. */ @@ -1213,34 +1214,21 @@ static cl_error_t egg_parse_file_extra_field(egg_handle* handle, egg_file* eggFi if (CL_SUCCESS != (retval = egg_parse_comment_header(index, size, extraField, &comment))) { cli_dbgmsg("egg_parse_file_extra_field: Issue parsing comment header. Error code: %u\n", retval); - break; + // Don't fail out with a `goto done;`. We're making a best effort to process the file, so just move on. + } else if (comment == NULL) { + cli_errmsg("egg_parse_file_extra_field: Logic error! Succesfully parsed comment header," + " but did not return egg_comment information!\n"); + goto done; } else { /* - * Success? + * Comment found. Add comment to our list. */ - if (comment == NULL) { - /* Uh... no. */ - cli_errmsg("egg_parse_file_extra_field: Logic error! Succesfully parsed comment header," - " but did not return egg_comment information!\n"); - goto done; - } else { - /* - * Comment found. Add comment to our list. - */ - char** comments_tmp; - - comments_tmp = (char**)cli_realloc( - (void*)eggFile->comments, - sizeof(char*) * (eggFile->nComments + 1)); - if (NULL == comments_tmp) { - free(comment); - status = CL_EMEM; - goto done; - } - eggFile->comments = comments_tmp; - eggFile->comments[eggFile->nComments] = comment; - eggFile->nComments++; - } + CLI_REALLOC(eggFile->comments, + sizeof(char*) * (eggFile->nComments + 1), + free(comment), + status = CL_EMEM); + eggFile->comments[eggFile->nComments] = comment; + eggFile->nComments++; } break; } @@ -1681,17 +1669,10 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t goto done; } else { /* Add file to list. */ - egg_file** files_tmp; - - files_tmp = (egg_file**)cli_realloc( - (void*)handle->files, - sizeof(egg_file*) * (handle->nFiles + 1)); - if (NULL == files_tmp) { - egg_free_egg_file(found_file); - status = CL_EMEM; - goto done; - } - handle->files = files_tmp; + CLI_REALLOC(handle->files, + sizeof(egg_file*) * (handle->nFiles + 1), + egg_free_egg_file(found_file), + status = CL_EMEM); handle->files[handle->nFiles] = found_file; handle->nFiles++; } @@ -1710,17 +1691,10 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t } else { /* Add block to list. */ if (handle->bSolid) { - egg_block** blocks_tmp; - - blocks_tmp = (egg_block**)cli_realloc( - (void*)handle->blocks, - sizeof(egg_block*) * (handle->nBlocks + 1)); - if (NULL == blocks_tmp) { - egg_free_egg_block(found_block); - status = CL_EMEM; - goto done; - } - handle->blocks = blocks_tmp; + CLI_REALLOC(handle->blocks, + sizeof(egg_block*) * (handle->nBlocks + 1), + egg_free_egg_block(found_block), + status = CL_EMEM); handle->blocks[handle->nBlocks] = found_block; handle->nBlocks++; } else { @@ -1733,20 +1707,12 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t // TODO: create an unamed block. egg_free_egg_block(found_block); } else { - egg_block** blocks_tmp; - eggFile = handle->files[handle->nFiles - 1]; - /* Add block to list. */ - blocks_tmp = (egg_block**)cli_realloc( - (void*)eggFile->blocks, - sizeof(egg_block*) * (eggFile->nBlocks + 1)); - if (NULL == blocks_tmp) { - egg_free_egg_block(found_block); - status = CL_EMEM; - goto done; - } - eggFile->blocks = blocks_tmp; + CLI_REALLOC(eggFile->blocks, + sizeof(egg_block*) * (eggFile->nBlocks + 1), + egg_free_egg_block(found_block), + status = CL_EMEM); eggFile->blocks[eggFile->nBlocks] = found_block; eggFile->nBlocks++; } @@ -1756,7 +1722,6 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t /* * Parse extra field for archive comment header. */ - char** comments_tmp; extra_field* extraField = NULL; char* comment = NULL; uint32_t size = 0; @@ -1806,24 +1771,32 @@ cl_error_t cli_egg_open(fmap_t* map, void** hArchive, char*** comments, uint32_t goto done; } - retval = egg_parse_comment_header(index, size, extraField, &comment); - if (CL_SUCCESS != retval) { - cli_dbgmsg("cli_egg_open: Failed to parse archive comment extra_field data.\n"); - goto done; - } - - comments_tmp = (char**)cli_realloc( - (void*)handle->comments, - sizeof(char*) * (handle->nComments + 1)); - if (NULL == comments_tmp) { - free(comment); - status = CL_EMEM; - goto done; + if (CL_SUCCESS != (retval = egg_parse_comment_header(index, size, extraField, &comment))) { + cli_dbgmsg("cli_egg_open: Issue parsing comment header. Error code: %u\n", retval); + // Don't fail out with a `goto done;`. We're making a best effort to process the file, so just move on. + } else { + /* + * Success? + */ + if (comment == NULL) { + /* Uh... no. */ + cli_errmsg("cli_egg_open: Logic error! Succesfully parsed comment header," + " but did not return egg_comment information!\n"); + goto done; + } else { + /* + * Comment found. Add comment to our list. + */ + CLI_REALLOC(handle->comments, + sizeof(char*) * (handle->nComments + 1), + free(comment), + status = CL_EMEM); + handle->comments[handle->nComments] = comment; + handle->nComments++; + } } - handle->comments = comments_tmp; - handle->comments[handle->nComments] = comment; - handle->nComments++; handle->offset += size; + } else { cli_dbgmsg("cli_egg_open: unexpected header magic: %08x (%s)\n", magic, getMagicHeaderName(magic)); status = CL_EPARSE; @@ -1949,7 +1922,6 @@ cl_error_t cli_egg_deflate_decompress(char* compressed, size_t compressed_size, { cl_error_t status = CL_EPARSE; - uint8_t* decoded_tmp; uint8_t* decoded = NULL; uint32_t declen = 0, capacity = 0; @@ -2002,12 +1974,10 @@ cl_error_t cli_egg_deflate_decompress(char* compressed, size_t compressed_size, while (zstat == Z_OK && stream.avail_in) { /* extend output capacity if needed,*/ if (stream.avail_out == 0) { - if (!(decoded_tmp = cli_realloc(decoded, capacity + BUFSIZ))) { - cli_errmsg("cli_egg_deflate_decompress: cannot reallocate memory for decompressed output\n"); - status = CL_EMEM; - goto done; - } - decoded = decoded_tmp; + CLI_REALLOC(decoded, + capacity + BUFSIZ, + cli_errmsg("cli_egg_deflate_decompress: cannot reallocate memory for decompressed output\n"), + status = CL_EMEM); stream.next_out = decoded + capacity; stream.avail_out = BUFSIZ; declen += BUFSIZ; @@ -2077,7 +2047,6 @@ cl_error_t cli_egg_bzip2_decompress(char* compressed, size_t compressed_size, ch { cl_error_t status = CL_EPARSE; - char* decoded_tmp; char* decoded = NULL; uint32_t declen = 0, capacity = 0; @@ -2127,12 +2096,10 @@ cl_error_t cli_egg_bzip2_decompress(char* compressed, size_t compressed_size, ch while (bzstat == BZ_OK && stream.avail_in) { /* extend output capacity if needed,*/ if (stream.avail_out == 0) { - if (!(decoded_tmp = cli_realloc(decoded, capacity + BUFSIZ))) { - cli_errmsg("cli_egg_bzip2_decompress: cannot reallocate memory for decompressed output\n"); - status = CL_EMEM; - goto done; - } - decoded = decoded_tmp; + CLI_REALLOC(decoded, + capacity + BUFSIZ, + cli_errmsg("cli_egg_bzip2_decompress: cannot reallocate memory for decompressed output\n"); + status = CL_EMEM); stream.next_out = decoded + capacity; stream.avail_out = BUFSIZ; declen += BUFSIZ; @@ -2194,7 +2161,6 @@ cl_error_t cli_egg_lzma_decompress(char* compressed, size_t compressed_size, cha { cl_error_t status = CL_EPARSE; - uint8_t* decoded_tmp; uint8_t* decoded = NULL; uint32_t declen = 0, capacity = 0; @@ -2247,12 +2213,10 @@ cl_error_t cli_egg_lzma_decompress(char* compressed, size_t compressed_size, cha while (lzmastat == LZMA_RESULT_OK && stream.avail_in) { /* extend output capacity if needed,*/ if (stream.avail_out == 0) { - if (!(decoded_tmp = cli_realloc(decoded, capacity + BUFSIZ))) { - cli_errmsg("cli_egg_lzma_decompress: cannot reallocate memory for decompressed output\n"); - status = CL_EMEM; - goto done; - } - decoded = decoded_tmp; + CLI_REALLOC(decoded, + capacity + BUFSIZ, + cli_errmsg("cli_egg_lzma_decompress: cannot reallocate memory for decompressed output\n"); + status = CL_EMEM); stream.next_out = decoded + capacity; stream.avail_out = BUFSIZ; declen += BUFSIZ; @@ -2379,7 +2343,6 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha } for (i = 0; i < currFile->nBlocks; i++) { - char* decompressed_tmp; egg_block* currBlock = currFile->blocks[i]; cl_error_t retval = CL_EPARSE; @@ -2399,14 +2362,12 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha cli_warnmsg("cli_egg_extract_file: blockHeader compress_size != uncompress_size!\n"); break; } - decompressed_tmp = cli_realloc(decompressed, (size_t)decompressed_size + currBlock->blockHeader->compress_size); - if (NULL == decompressed_tmp) { - cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", - decompressed_size); - status = CL_EMEM; - goto done; - } - decompressed = decompressed_tmp; + + CLI_REALLOC(decompressed, + (size_t)decompressed_size + currBlock->blockHeader->compress_size, + cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", + decompressed_size), + status = CL_EMEM); memcpy(decompressed + decompressed_size, currBlock->compressedData, currBlock->blockHeader->compress_size); decompressed_size += currBlock->blockHeader->compress_size; @@ -2427,15 +2388,12 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha goto done; } /* Decompressed block. Add it to the file data */ - decompressed_tmp = cli_realloc(decompressed, (size_t)decompressed_size + decompressed_block_size); - if (NULL == decompressed_tmp) { - cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", - decompressed_size); - free(decompressed_block); - status = CL_EMEM; - goto done; - } - decompressed = decompressed_tmp; + CLI_REALLOC(decompressed, + (size_t)decompressed_size + decompressed_block_size, + cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", + decompressed_size), + free(decompressed_block), + status = CL_EMEM); memcpy(decompressed + decompressed_size, decompressed_block, decompressed_block_size); decompressed_size += decompressed_block_size; @@ -2459,15 +2417,12 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha goto done; } /* Decompressed block. Add it to the file data */ - decompressed_tmp = cli_realloc(decompressed, (size_t)decompressed_size + decompressed_block_size); - if (NULL == decompressed_tmp) { - cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", - decompressed_size); - free(decompressed_block); - status = CL_EMEM; - goto done; - } - decompressed = decompressed_tmp; + CLI_REALLOC(decompressed, + (size_t)decompressed_size + decompressed_block_size, + cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", + decompressed_size), + free(decompressed_block), + status = CL_EMEM); memcpy(decompressed + decompressed_size, decompressed_block, decompressed_block_size); decompressed_size += decompressed_block_size; @@ -2501,15 +2456,12 @@ cl_error_t cli_egg_extract_file(void* hArchive, const char** filename, const cha // goto done; // } // /* Decompressed block. Add it to the file data */ - // decompressed_tmp = cli_realloc(decompressed, (size_t)decompressed_size + decompressed_block_size); - // if (NULL == decompressed_tmp) { - // cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", - // decompressed_size); - // free(decompressed_block); - // status = CL_EMEM; - // goto done; - // } - // decompressed = decompressed_tmp; + // CLI_REALLOC(decompressed, + // (size_t)decompressed_size + decompressed_block_size, + // cli_errmsg("cli_egg_extract_file: Failed to allocate %" PRIu64 " bytes for decompressed file!\n", + // decompressed_size), + // free(decompressed_block), + // status = CL_EMEM); // memcpy(decompressed + decompressed_size, decompressed_block, decompressed_block_size); // decompressed_size += decompressed_block_size;