net_smtp: Prevent SMTP smuggling attacks.

Reject messages containing a bare CR or LF anywhere, to avoid
being vulnerable to SMTP smuggling attacks. These could occur
if an outbound mail server ignores bare CR or LF and an inbound
server tolerates (and interprets) them. Previously, we ignored
them, so while not vulnerable on the inbound side, we were
vulnerable on the outbound side, i.e. could have been tricked into
sending a malformed message with a stuffed SMTP transaction to a
vulnerable inbound server, which would then process the stuffed
message that appeared to legitimately originate from us. Explicitly
rejecting (rather than ignoring) bare line endings prevents this.

Author: InterLinked1

bbs/string.c

@@ -263,6 +263,17 @@ int bbs_term_line(char *restrict c)
 	return len;
 }
 
+int bbs_str_contains_line_ending(const char *s)
+{
+	while (*s) {
+		if (*s == '\r' || *s == '\n') {
+			return *s;
+		}
+		s++;
+	}
+	return 0;
+}
+
 int bbs_str_contains_bare_lf(const char *s)
 {
 	int prev_cr = 0;

include/string.h

@@ -87,6 +87,14 @@ int bbs_strncount(const char *restrict s, size_t len, char c) __attribute__ ((pu
  */
 int bbs_term_line(char *restrict c);
 
+/*!
+ * \brief Whether a string contains a CR or LF character
+ * \param s A NUL-terminated string
+ * \retval '\r' if CR present, '\n' if LF present, 0 if neither present
+ * \note If both are present, only the first one encountered is returned
+ */
+int bbs_str_contains_line_ending(const char *s);
+
 /*!
  * \brief Whether a NUL-terminated string contains bare line feeds (LF characters not immediately preceded by a CR)
  * \param s String to search

nets/net_smtp.c

@@ -3260,6 +3260,7 @@ static int handle_data(struct smtp_session *smtp, char *s, struct readline_data
 	FILE *fp;
 	char template[256];
 	int datafail = 0;
+	int bare_cr_lf = 0;
 	int indataheaders = 1;
 
 	REQUIRE_HELO();
@@ -3279,6 +3280,7 @@ static int handle_data(struct smtp_session *smtp, char *s, struct readline_data
 
 	for (;;) {
 		size_t len;
+		int crlfres;
 		ssize_t res = bbs_node_readline(smtp->node, rldata, "\r\n", MIN_MS(3)); /* RFC 5321 4.5.3.2.5 */
 		if (res < 0) {
 			bbs_delete_file(template);
@@ -3291,7 +3293,7 @@ static int handle_data(struct smtp_session *smtp, char *s, struct readline_data
 				 * In practice, most messages that violate this seem to be spam anyways,
 				 * so we have no reason to tolerate noncompliant messages.
 				 * However, we need to reject it properly with the right error message before disconnecting. */
-				smtp_reply_nostatus(smtp, 550, "Maximum line length exceeded");
+				smtp_reply_nostatus(smtp, 550, "Maximum line length exceeded (violates RFC 5322 2.3)");
 				smtp->failures += 3; /* Semantically, this is a bad client. However, we're just going to disconnect now anyways, so this doesn't really matter. */
 			}
 			return -1;
@@ -3307,6 +3309,10 @@ static int handle_data(struct smtp_session *smtp, char *s, struct readline_data
 				if (smtp->tflags.datalen >= smtp_max_session_message_size(smtp)) {
 					/* Message too large. */
 					smtp_reply(smtp, 552, 5.2.3, "Your message exceeded our message size limits");
+				} else if (bare_cr_lf) {
+					smtp_reply(smtp, 554, 5.0.0, "Message rejected, bare %s detected (violates RFC 5322 2.3)", bare_cr_lf == '\r' ? "CR" : "LF");
+					smtp->failures += 2;
+					return 0;
 				} else {
 					/* Message not successfully received in totality, so reject it. */
 					smtp_reply(smtp, 451, 4.3.0, "Message not received successfully, try again");
@@ -3351,6 +3357,23 @@ static int handle_data(struct smtp_session *smtp, char *s, struct readline_data
 			continue; /* Corruption already happened, just ignore the rest of the message for now. */
 		}
 
+		crlfres = bbs_str_contains_line_ending(rldata->buf);
+		if (crlfres) {
+			/* The normal CR LF line ending will be consumed by bbs_node_readline.
+			 * Therefore, there should NOT be any bare CR or LF on its own.
+			 * Not only is it not RFC-compliant, but it would also open us up
+			 * to SMTP smuggling attacks where we could inadvertently relay
+			 * messages to another server that we did not intend to, which happens
+			 * when the outbound server ignores bare CR or LF and the inbound
+			 * server interprets them, allowing for a separate transaction to
+			 * be "stuffed" on the outbound side (see https://smtpsmuggling.com/).
+			 * By strictly rejecting such messages, we are not vulnerable as either
+			 * the outbound or inbound server. */
+			bare_cr_lf = crlfres;
+			datafail = 1;
+			continue;
+		}
+
 		if (indataheaders) {
 			if ((smtp->fromlocal || smtp->msa) && STARTS_WITH(s, "From:")) {
 				const char *newfromhdraddr = S_IF(s + 5);

tests/test_smtp_mta.c

@@ -138,8 +138,6 @@ static int run(void)
 	/* Ensure mail loops are prevented */
 	SWRITE(clientfd, "RSET" ENDL);
 	CLIENT_EXPECT(clientfd, "250");
-	SWRITE(clientfd, "EHLO " TEST_EXTERNAL_DOMAIN ENDL);
-	CLIENT_EXPECT_EVENTUALLY(clientfd, "250 ");
 	SWRITE(clientfd, "MAIL FROM:<" TEST_EMAIL_EXTERNAL ">\r\n");
 	CLIENT_EXPECT(clientfd, "250");
 	SWRITE(clientfd, "RCPT TO:<" TEST_EMAIL ">\r\n");
@@ -155,7 +153,7 @@ static int run(void)
 	SWRITE(clientfd, "." ENDL); /* EOM */
 	CLIENT_EXPECT(clientfd, "554"); /* Mail loop detected */
 
-	/* Test messages that are exactly as long as the readline buffer. */
+	/* Test message with a bare line feed. Should be rejected. */
 	SWRITE(clientfd, "RSET" ENDL);
 	CLIENT_EXPECT(clientfd, "250");
 	SWRITE(clientfd, "EHLO " TEST_EXTERNAL_DOMAIN ENDL);
@@ -166,6 +164,28 @@ static int run(void)
 	CLIENT_EXPECT(clientfd, "250");
 	SWRITE(clientfd, "DATA\r\n");
 	CLIENT_EXPECT(clientfd, "354");
+	SWRITE(clientfd, "Subject: Test\r\n"
+		"\r\n"
+		"Hello there\n, this is a malformed message\r\n"
+		"The end.\r\n");
+	SWRITE(clientfd, "." ENDL); /* EOM */
+	CLIENT_EXPECT(clientfd, "554");
+
+	/* Reconnect since we'd now get tarpitted */
+	close(clientfd);
+	clientfd = test_make_socket(25);
+	REQUIRE_FD(clientfd);
+	CLIENT_EXPECT_EVENTUALLY(clientfd, "220 ");
+	SWRITE(clientfd, "EHLO " TEST_EXTERNAL_DOMAIN ENDL);
+	CLIENT_EXPECT_EVENTUALLY(clientfd, "250 ");
+
+	/* Test messages that are exactly as long as the readline buffer. */
+	SWRITE(clientfd, "MAIL FROM:<" TEST_EMAIL_EXTERNAL ">\r\n");
+	CLIENT_EXPECT(clientfd, "250");
+	SWRITE(clientfd, "RCPT TO:<" TEST_EMAIL ">\r\n");
+	CLIENT_EXPECT(clientfd, "250");
+	SWRITE(clientfd, "DATA\r\n");
+	CLIENT_EXPECT(clientfd, "354");
 	SWRITE(clientfd, "Subject: Test\r\n"
 		"Content-Type: text/plain; charset=utf-8; format=flowed\r\n"
 		"Content-Transfer-Encoding: 8bit\r\n"
@@ -196,8 +216,6 @@ static int run(void)
 	/* Ensure messages with lines over 1,000 characters are rejected */
 	SWRITE(clientfd, "RSET" ENDL);
 	CLIENT_EXPECT(clientfd, "250");
-	SWRITE(clientfd, "EHLO " TEST_EXTERNAL_DOMAIN ENDL);
-	CLIENT_EXPECT_EVENTUALLY(clientfd, "250 ");
 	SWRITE(clientfd, "MAIL FROM:<" TEST_EMAIL_EXTERNAL ">\r\n");
 	CLIENT_EXPECT(clientfd, "250");
 	SWRITE(clientfd, "RCPT TO:<" TEST_EMAIL ">\r\n");