Skip to content

mime: reject CR and LF in mail part name and filename - #22247

Closed
alhudz wants to merge 2 commits into
curl:masterfrom
alhudz:mime-mail-name-crlf
Closed

mime: reject CR and LF in mail part name and filename#22247
alhudz wants to merge 2 commits into
curl:masterfrom
alhudz:mime-mail-name-crlf

Conversation

@alhudz

@alhudz alhudz commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Repro: send mail over SMTP (or IMAP APPEND) with a mime part whose filename (or name) contains a CR or LF, e.g. curl_mime_filename(part, "a\r\nX-Injected: 1").

Cause: add_content_disposition() builds the Content-Disposition header and, for the mail strategy (and the legacy mime_formescape mode), escape_string() only backslash-quotes the value. A CR or LF in the part name/filename is written raw into the quoted-string and splits the generated MIME header, so a crafted filename smuggles extra header lines into the message. The HTTP form strategy already percent-encodes CR/LF (%0D/%0A), so only the mail/backslash path was affected.

Fix: reject a CR or LF in the part name or filename in add_content_disposition() when the backslash strategy is in effect, before the header is built. unit3227 covers the mail rejection, a clean mail part, and that the form path stays accepted (encoded).

@github-actions github-actions Bot added the tests label Jul 2, 2026
@testclutch

Copy link
Copy Markdown

Analysis of PR #22247 at a90b6182:

Test 3227 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 6 different CI jobs (the link just goes to one of them).

Generated by Testclutch

The test allocated the easy and mime handles then ran its assertions
with cleanup at the end, so any abort_unless jumped past the cleanup.
Under torture testing an early allocation failure leaked those handles.
Move the handles ahead of UNITTEST_BEGIN and free them from the
UNITTEST_END stop function so all abort paths tear down cleanly.

Signed-off-by: Alhuda Khan <al.hudz.k@gmail.com>
@alhudz

alhudz commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

The torture failure was in the test, not the fix. unit3227 allocated the easy and mime handles up front but only freed them at the end, so an abort_unless triggered by an injected allocation failure jumped past the cleanup and leaked. Moved the handles ahead of UNITTEST_BEGIN and free them from the UNITTEST_END stop function, matching unit1396.

Verified locally with CURL_MEMLIMIT across all 29 allocations: 0 leaks now, whereas the previous version leaked at 21 of them.

@alhudz

alhudz commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

any update?

@bagder bagder closed this in c3f9ef1 Jul 13, 2026
@bagder

bagder commented Jul 13, 2026

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants