Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend barelf check to whole message #318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bseibold
Copy link

Instead of checking for bare LF just on end of message, check every line. This helps to mitigate SMTP smuggling.

Fixes #317

Instead of checking for bare LF just on end of message, check every
line. This helps to mitigate SMTP smuggling.
Copy link
Member

@msimerson msimerson left a comment

Choose a reason for hiding this comment

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

Why move the EOM (".\r\n") test? Unless I'm reading this wrong, moving it makes no difference.

@bseibold
Copy link
Author

Why move the EOM (".\r\n") test? Unless I'm reading this wrong, moving it makes no difference.

Indeed it should not make a difference, but I thought it's easier to read and understand this way: First do checks that apply to every line, then do checks for more specific conditions (EOM), then do the actual processing.

@abh
Copy link
Member

abh commented Dec 30, 2023

Thanks for poking at this, @bseibold! I think the last time this bit of code had a meaningful change might have been in 2002 in e2cc2f7 (or maybe 11 years ago in ce18cf7). :-D

Some tests for this change would be valuable, I think.

@bseibold
Copy link
Author

Sure, tests are always a good idea, especially since this is the first line of Perl I have written in more than 15 years (and before that it was mostly code golf). But frankly I don't want to spend the effort to re-learn Perl just for these tests. Sorry.

I did however deploy the change to my (extremely low-volume) server, and so far I didn't find anything in the logs that would suggest that it doesn't work. A few test mails did also pass.

# Reject messages that have either bare LF or CR. rjkaes noticed a
# lot of spam that is malformed in the header.

if ($_ eq ".\n" || $_ eq ".\r") {
if ($_ !~ /\r\n$/) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, why isn't \n$ allowed inside the message body? It wouldn't be a new line, technically, just a \n inside a message.

I thought it should only be disallowed as a "end of message" marker.

Copy link
Member

Choose a reason for hiding this comment

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

RFC 5322 (2008), Section 4: "Finally, certain characters that were formerly allowed in messages appear in this section.... CR and LF were allowed to appear in messages other than as CRLF;"

Appendix B.27: "Free CR and LF not allowed."

@msimerson
Copy link
Member

Indeed it should not make a difference, but I thought it's easier to read and understand this way:

If you had not made that change, I would likely have already merged this PR. Previously, the checks were extremely cheap, now the EOM and every line in the message is hitting the regex engine. So the processing cost is raised somewhat. But I also think this is a good time to make this change. CPU is very cheap and every other MTA is also tightening their bare LF handling because of SMTP smuggling.

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

Successfully merging this pull request may close these issues.

SMTP Smuggling: qpsmtpd seems to be not affected
3 participants