Clam 2947 improved email attachment extraction - #1720
Conversation
Email parsing could disagree with common mail clients when duplicate or malformed MIME boundary declarations were present. That allowed attachments behind the client-selected boundary to be skipped by the scanner. Use the last Content-Type boundary value, keep invalid commented header names from being normalized, parse quoted semicolons correctly, and ignore boundary parameters on Content-Disposition. Add regression inputs for the reported evasion cases. Credit: Artem Danilov at Positive Technologies CLAM-2947
Malformed MIME messages could drive unbounded work in nested part headers and partial-message reassembly. A few parser helpers also relied on assertions or undefined ctype input handling. Apply the existing header and MIME count limits while parsing multipart parts, validate partial-message numbers before reassembly, handle allocation failures explicitly, and add regression fixtures for the related MIME edge cases. CLAM-2947
Email parsing checked every body line for MIME boundaries. The helpers allocated and rescanned strings before rejecting ordinary content. Skip non-boundary lines before allocation, scan semicolon-only lines in one pass, and reuse the MIME argument buffer length while parsing headers. CLAM-2947
The email parser still had a few unsafe or expensive paths around decoded line output, MIME argument handling, boundary matching, and message/partial reassembly. Bound decoded writes, preserve message list state on allocation failures, parse all Content-Disposition parameters, and reassemble message/partial inputs with a single checked directory pass. CLAM-2947
Malformed quoted-printable input could consume the final output byte without updating the remaining capacity. Content-Disposition parsing also skipped the MIME argument cap, and text move or append failures could drop or silently truncate message body data. Multipart part construction could also proceed after allocation failures. Tighten the fixed-buffer decode guard, account for malformed quoted-printable output, and apply the existing MIME argument limit to Content-Disposition parameters. Make text moves and appends report allocation failure without losing the current chain. Treat multipart part construction allocation failures as parse failures so partially built parts are not scanned as complete. CLAM-2947
The mbox parser had a couple of internal error paths that could leak cl_error_t values through APIs that use local status or negative failure returns. That confused enum types and could report failed partial-message work as success. Add an explicit mbox format-error status for MIME subtype parsing and normalize partial-message helper failures to negative return values so callers preserve the existing error checks. CLAM-2947
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b107097b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR hardens ClamAV’s email/MIME parsing to improve attachment extraction and close multiple evasion edge cases reported in CLAM-2947, and adds encrypted regression fixtures to ensure those cases stay covered.
Changes:
- Adds new encrypted
.emlregression fixtures and wires them into the unit test input set. - Improves MIME argument parsing (multipart boundary selection, Content-Disposition parameter parsing), adds bounds/heuristics, and reworks
message/partialhandling. - Replaces some
assert()paths with explicit allocation-failure handling and fixes severalctype()callsites to cast viaunsigned char.
Reviewed changes
Copilot reviewed 7 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| unit_tests/input/clamav_hdb_scanfiles/clam.mail-*.eml.xor | Adds encrypted .eml regression fixtures for MIME parsing evasions. |
| unit_tests/input/CMakeLists.txt | Registers the new encrypted .eml fixtures for test setup/decryption. |
| unit_tests/check_clamav.c | Updates expected decrypted testfile count to include new fixtures. |
| libclamav/text.h | Adds status-reporting variants for text/message merge helpers. |
| libclamav/text.c | Implements status-reporting text merge/copy operations and replaces asserts with error handling. |
| libclamav/message.h | Adds messageFindArgumentLast() to support “last boundary wins”. |
| libclamav/message.c | Improves MIME argument parsing and allocation-failure robustness; adds messageFindArgumentLast(). |
| libclamav/mbox.c | Main parser hardening: boundary selection, MIME parameter iteration, heuristics, message/partial reassembly changes, and error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MIME header lookups stopped stripping RFC822 comments from header names before matching them against the RFC821 table. That regressed malformed but accepted headers such as Content-Type(comment): and could skip boundary or attachment metadata. Route RFC821 header-name lookups through a helper that strips RFC822 comments, handle allocation failures in the MIME continuation path, and tighten the comment-header fixtures so detection depends on the commented header. CLAM-2947
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
libclamav/text.c:261
textAddMessageWithStatus()doesn’t handlemessageToText()returning NULL (e.g., OOM). In that casetextMoveWithStatus()treatst == NULLas success,statusremains 0, and the message body is silently dropped. Add an explicit NULL check aftermessageToText()and set*status = -1(and returnaText) when conversion fails.
text *anotherText = messageToText(aMessage);
if (aText) {
int moveStatus = 0;
text *newHead = textMoveWithStatus(aText, anotherText, &moveStatus);
if (moveStatus < 0) {
textDestroy(anotherText);
if (status)
*status = -1;
return aText;
}
free(anotherText);
return newHead;
}
return anotherText;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
textMoveWithStatus() empties the temporary text head on success. The wrapper still needs textDestroy() for consistent cleanup. Destroy the temporary wrapper after each move attempt. If moving fails, preserve the existing accumulated text. Thank-you to Jiasheng Jiang for identifying this issue.
jhumlick
left a comment
jhumlick
left a comment
There was a problem hiding this comment.
Found a couple of issue:
libclamav/mbox.c:3144
boundaryEnd() no longer accepts trailing whitespace after closing MIME boundaries like --boundary-- . The old code trimmed trailing spaces before matching, so this can regress malformed-but-supported messages.
libclamav/mbox.c:1974 and libclamav/mbox.c:2159
The new multipart header byte/fold limits don’t count folded continuation lines consumed by next_is_folded_header(). A large folded MIME-part header can still grow fullline while bypassing the new counters.
Recent mbox parser hardening stopped accepting whitespace after closing multipart boundaries and missed folded MIME-part lines consumed while assembling a logical header. That could regress malformed but supported messages and still allow oversized folded headers to grow in memory. Accept only transport whitespace after --boundary-- markers, and count each consumed folded continuation against the part header byte and fold limits before appending it to the accumulated header line. CLAM-2947
|
PARSE_HEADER_ALLOC_FAIL is still swallowed in the top-level header parsers. parseEmailFile() treats any parseEmailHeader(...) < 0 as “continue folding/ignore” at libclamav/mbox.c#L984 and #L1105; parseEmailHeaders() does the same at #L1330. Since this PR added a distinct allocation-failure return, these call sites should handle PARSE_HEADER_ALLOC_FAIL separately and abort/mark truncated instead of continuing with partially parsed MIME headers. The non-default #ifndef SAVE_TO_DISC path still calls the old parseEmailHeaders(aMessage, table) signature at libclamav/mbox.c#L4865. If that path is kept buildable, this needs the new bool heuristicFound argument and handling. |
Two review findings showed that top-level email header parsers still treated parseEmailHeader() allocation failures like malformed headers. That could continue parsing partially populated MIME metadata instead of reporting the message as truncated. Return NULL from textToFileblob() for NULL input, matching textToBlob(). Propagate header allocation failures through the existing truncated-message path, and update the non-default in-memory message path to use the current parseEmailHeaders() signature. CLAM-2947
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review follow-up found mail parser paths that still relied on split cleanup and stale ownership assumptions. That made future fixes more likely to introduce double frees or missed cleanup when error handling changed. Convert cli_parse_mbox() to a single done: cleanup path and clear ownership transfers between body and m. Also remove stale partial files only when temporary files are not being kept, and make textAddWithStatus() report copy failures without a caller-supplied status pointer. CLAM-2947
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5b0db1345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review follow-up pushed and the inline threads are resolved.
Verification run locally: git diff --check, make -j12, and ctest -V -R "libclamav|clamscan". |
The stale message/partial cleanup path unlinked candidate files while the test descriptor was still open. That can fail on Windows and cause reassembly to abort before scanning the reconstructed message. Close the descriptor before calling cli_unlink() for stale partial files, and keep the shared cleanup close for non-stale files. CLAM-2947
jhumlick
left a comment
jhumlick
left a comment
There was a problem hiding this comment.
- Suppressed Copilot concern still looks valid. In textAddMessageWithStatus(), messageToText(aMessage) can return NULL, but the function does not set *status = -1. If aText already exists, textMoveWithStatus(aText, NULL, ...) returns success, so the decoded body is silently dropped while callers like parseEmailBody() see textStatus == 0. See libclamav/text.c at #L246 and callers at libclamav/mbox.c#L1817 / #L2512. This undercuts the PR’s allocation-failure hardening.
textAddMessageWithStatus() could treat a failed messageToText() conversion as a successful append when existing text had already been collected. That let mail parsing continue after silently dropping the current decoded body. Check the converted text before calling textMoveWithStatus(). Preserve any existing accumulated text, but report failure through the status output so callers can stop parsing safely. CLAM-2947
|
@jhumlick I pushed 3286694 to address the textAddMessageWithStatus() review finding. The function now checks messageToText() before calling textMoveWithStatus(); if conversion returns NULL, it preserves any accumulated text but sets *status = -1 so the mbox callers stop parsing safely instead of treating the append as successful. Verification run locally: git diff --check, make -j12, and ctest -V -R "libclamav|clamscan". |
Summary
This PR improves email attachment extraction for several MIME parsing edge
cases reported in CLAM-2947.
The changes add regression coverage for the reported
.emlevasion patternsand harden the mail parser around MIME boundary selection, header argument
parsing, partial-message reassembly, allocation failures, and malformed input.
Details
multiple boundary-like parameters, semicolon-delimited parameters, and
comments in header names.
boundaryargument for multipart parsing whileignoring
boundaryparameters fromContent-Disposition.Content-Dispositionparameters instead of only the firstparameter.
and
message/partialinputs.message/partialreassembly to validate numeric arguments, check pathlengths, avoid unsafe
atoibehavior, and assemble parts with one checkeddirectory pass.
assert()checks in text/message paths with explicit errorhandling so allocation failures preserve parser state.
ctype()inputs throughunsigned charin touched email parser paths..emlregression fixtures for the reported detection gaps.Testing
git diff --checkmake -j12ctest -V -R libclamavCredit: Artem Danilov at Positive Technologies
CLAM-2947