(possible) DOS for malformed mails containing NULL-bytes in body
Hi!
I’ve run across an issue where piler-smtp hangs for certain malformed mails. The mail in question had a malformed attachment:
Content-Type: application/octet-stream; name="xxx.png"
Content-Transfer-Encoding: 7bit
Content-ID: <logo>
Content-Disposition: attachment; filename="xxx.png"
which was in fact just binary data containing (at least) a NULL-byte. Postfix, amavis/spammassassin and Exchange have no problem proccessing these mails, spamassassin even has an NULL_IN_BODY Rule.
I’ve tracked the issue down to read_one_line() in misc.c called from handle_data() in session.c as follows:
1. read_one_line() will copy the start of the line from char *s to char *buf until it reaches the NULL the first time it is called (for loop is broken by ‘*s == NULL’ once reached) and returns the number of chars copied, rc = ERR.
2. do-loop in session.c calls read_one_line() once more, this will immediately return 0, making handle_data() return.
At this point the buffer in handle_data() still contains some data that has not been copied (and never will be, even when called again).
Possible fix: evaluate rc in the do{}, it should be ERR in this case, and make handle_data() return an error, and stop processing of the broken mail.
I’m not familiar enough with the code to guess if this would work.
piler 1.3.6, build 998, Janos SUTO sj@acts.hu - but irrelevant, I’ve checked against master for the links above.
Comments (11)
-
repo owner -
reporter - attached mail-redacted.txt
stripped of most headers and mail content. NULL in attachment should still trigger the behavior
-
repo owner Thank you. Anyway, it’s odd that this mail client failed to encode the binary attachment. I’ll come up with a fix soon, and let you know.
-
reporter These are automated mails sent by some web portal via AWS. Looks like something is broken there (I have notified them).
This should however not lead to piler-smtp being blocked by the mail content. piler-smtp is processing totally untrusted input and should not make any assumptions about its contents and whether the input is valid.
read_one_line() is basically an extended strncpy() with additional stop chars, so it assumes there will never be NULL in the input. -
reporter - edited description
insert missing newline
-
repo owner https://bitbucket.org/jsuto/piler/commits/e1ea14374a57ae4a517a789b298620ecc9fa7b7f
This commit should fix the issue. When receiving such a message, piler-smtp now properly receives it, then moves it to /var/piler/error along with a syslog message like
ERROR: 2UERGKNS5GWF16CA contains an invalid NUL-byte, moving it to /var/piler/error
Btw. I managed to import this attached test email in the test environment, however, the png attachment became garbage. Not sure if it’s worth trying to fix to retrieve such message from the archive. That’s the reasoning for moving to the error dir for later inspection.
-
reporter I just skimmed the diffs, should work.
Testing now.
I’ve had some trouble rebuilding on debian/stretch (needs to be upgraded soonish) and had to disable TLS1.3:
gcc -std=c99 -O2 -fPIC -Wall -Wextra -Wuninitialized -Wno-format-truncation -g -I. -I.. -I/usr/include/mysql -D_GNU_SOURCE -DHAVE_TRE -DNEED_MYSQL -c cfg.c -o cfg.o cfg.c: In function ‘get_tls_protocol_number’: cfg.c:156:20: error: ‘TLS1_3_VERSION’ undeclared (first use in this function) { "TLSv1.3", TLS1_3_VERSION }, ^~~~~~~~~~~~~~ # openssl version OpenSSL 1.1.0l 10 Sep 2019 # apt-cache policy libssl-dev libssl-dev: Installed: 1.1.0l-1~deb9u3 Candidate: 1.1.0l-1~deb9u3 # grep OPENSSL_VERSION_NUMBER /usr/include/openssl/opensslv.h # define OPENSSL_VERSION_NUMBER 0x101000cfL
piler/src/cfg.c:
155 #if OPENSSL_VERSION_NUMBER >= 0x10100000L
156 { "TLSv1.3", TLS1_3_VERSION },
157 #endifI didn’t check what the minimum OPEN_SSL_VERSION_NUMBER is supposed to be for TLS1_3_VERSION to be available.
-
repo owner Try the following instead:
#ifdef TLS1_3_VERSION { "TLSv1.3", TLS1_3_VERSION }, #endif
-
reporter This was meant as a side note and is somewhat off-topic.
TLS1.3 support was introduced in OpenSSL 1.1.1, so the check should probably be
#if OPENSSL_VERSION_NUMBER >= 0x10101000L
-
repo owner I think it’s better to test for the given feature, not for version number.
-
repo owner - changed status to closed
Anyway, I believe that the original issue has been fixed.
- Log in to comment
Can you attach such email to the issue to verify and fix it with?