Skip to content

Reject CR/LF in SimpleSMTPHeader and SimpleNNTPHeader fields#397

Merged
garydgregory merged 2 commits into
apache:masterfrom
dxbjavid:reject-crlf-header-injection
Jun 1, 2026
Merged

Reject CR/LF in SimpleSMTPHeader and SimpleNNTPHeader fields#397
garydgregory merged 2 commits into
apache:masterfrom
dxbjavid:reject-crlf-header-injection

Conversation

@dxbjavid
Copy link
Copy Markdown
Contributor

@dxbjavid dxbjavid commented Jun 1, 2026

Fuzzing a small contact-form mailer built on SimpleSMTPHeader turned this up. The subject/from/cc and addHeaderField values get appended straight into the generated header block, so an embedded LF injects arbitrary mail headers or a message body (CWE-93). SimpleNNTPHeader has the same gap in its From/Subject/Newsgroups and header sinks. Reject CR/LF in those values, like the existing null-From check.

@garydgregory
Copy link
Copy Markdown
Member

@dxbjavid
Thank you for the PR.
You must provide a unit test that fails when the changes to main are not applied. Otherwise, this is a regression waiting to happen.

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Added tests for both classes covering the constructor, addCC/addNewsgroup, and addHeaderField sinks. They fail on master (8 failures, nothing thrown) and pass with the fix applied.

Copy link
Copy Markdown
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hello @dxbjavid

Thank you for your PR.
What about Unicode line breaks, for example U+2028? Should those matter?

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Only CR and LF actually terminate a line in the SMTP/NNTP wire protocol, so those are the bytes that let a value break out of its header. U+2028 and the other Unicode line breaks aren't line terminators there, they just get written out as their encoded bytes (E2 80 A8 for U+2028 in UTF-8, none of which is CR or LF), so they can't inject a header field. I kept the guard to CR/LF to match that and the existing null-From check. Can widen it if you'd prefer to be strict about them, but it isn't needed to close the injection.

@garydgregory
Copy link
Copy Markdown
Member

If the spec says CR and LF then that should be good enough.

@garydgregory garydgregory changed the title reject CR/LF in SimpleSMTPHeader and SimpleNNTPHeader fields Reject CR/LF in SimpleSMTPHeader and SimpleNNTPHeader fields Jun 1, 2026
@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Right. Header fields are CRLF-terminated on the wire: RFC 5322 section 2.2 for mail, RFC 3977/5536 for news. CR and LF are the only bytes that end a field there, so guarding those two closes the injection.

@garydgregory garydgregory merged commit 17a6955 into apache:master Jun 1, 2026
11 checks passed
@garydgregory
Copy link
Copy Markdown
Member

@dxbjavid
Thank you, merged 🚀 !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants