-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: master
Are you sure you want to change the base?
Conversation
Instead of checking for bare LF just on end of message, check every line. This helps to mitigate SMTP smuggling.
There was a problem hiding this 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.
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. |
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$/) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
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. |
Instead of checking for bare LF just on end of message, check every line. This helps to mitigate SMTP smuggling.
Fixes #317