mod_smtp_filter_arc: Don't add bare LFs to message.

libopenarc violates the RFCs by producing bare line feeds
in the headers that it generates. To work around this,
we now detect bare line feeds and replace them with CR LF.
This fixes RFC compliance and delivery to MTAs that reject
messages containing bare LFs. This also adds some checking
in other places to detect bare LFs and warn about them.

On top of this, ARC headers were being added to messages
submitted directly for submission (which is unnecessary,
and probably also just wrong). We now only add ARC headers
if the message already had a DKIM signature.

bbs/socket.c

@@ -2033,6 +2033,29 @@ int bbs_node_flush_input(struct bbs_node *node)
 	return res;
 }
 
+ssize_t bbs_timed_read(int fd, char *restrict buf, size_t len, int ms)
+{
+	ssize_t bytes = 0;
+
+	for (;;) {
+		ssize_t res = read(fd, buf, len);
+		if (res <= 0) {
+			return res;
+		}
+		bytes += res;
+		buf += res;
+		len -= (long unsigned) res;
+		if (len <= 0) {
+			bbs_debug(7, "Buffer (size %lu) is now full\n", len);
+			return res;
+		}
+		if (bbs_poll(fd, ms) <= 0) {
+			return res;
+		}
+	}
+	__builtin_unreachable();
+}
+
 static ssize_t timed_write(struct pollfd *pfd, int fd, const char *restrict buf, size_t len, int ms)
 {
 	size_t left = len;

bbs/string.c

@@ -263,6 +263,79 @@ int bbs_term_line(char *restrict c)
 	return len;
 }
 
+int bbs_str_contains_bare_lf(const char *s)
+{
+	int prev_cr = 0;
+	int bare_lf = 0;
+
+	for (; *s; s++) {
+		if (*s == '\r') {
+			prev_cr = 1;
+		} else {
+			if (*s == '\n' && !prev_cr) {
+				bare_lf++;
+				/* For just a boolean answer, we could break (return 1) now,
+				 * but since the common case is, presumably, no bare LFs,
+				 * we'll end up searching the entire string most of the time,
+				 * so we might as well do that now and return a specific answer. */
+			}
+			prev_cr = 0;
+		}
+	}
+	return bare_lf;
+}
+
+char *bbs_str_bare_lf_to_crlf(const char *inbuf)
+{
+	size_t newlen, oldlen = 0;
+	const char *s;
+	char *newbuf, *d;
+	size_t bare_lf = 0;
+	int prev_cr;
+
+	/* First, see how many bare LFs there are, so we can compute
+	 * the length of the new string.
+	 * We'll compute the original length too as we go,
+	 * so we only need 1 pass rather than 2. */
+	for (s = inbuf, prev_cr = 0; *s; s++, oldlen++) {
+		if (*s == '\r') {
+			prev_cr = 1;
+		} else {
+			if (*s == '\n' && !prev_cr) {
+				bare_lf++;
+			}
+			prev_cr = 0;
+		}
+	}
+
+	bbs_debug(5, "Input has %lu bare LFs\n", bare_lf);
+	if (!bare_lf) {
+		/* No bare line feeds detected. */
+		return strdup(inbuf);
+	}
+
+	newlen = oldlen + bare_lf + 1; /* Plus NUL */
+	newbuf = malloc(newlen);
+	if (ALLOC_FAILURE(newbuf)) {
+		return NULL;
+	}
+
+	for (s = inbuf, d = newbuf, prev_cr = 0; *s; s++) {
+		if (*s == '\r') {
+			prev_cr = 1;
+		} else {
+			if (*s == '\n' && !prev_cr) {
+				/* Insert the missing CR */
+				*d++ = '\r';
+			}
+			prev_cr = 0;
+		}
+		*d++ = *s;
+	}
+	*d = '\0';
+	return newbuf;
+}
+
 /* XXX Have to pass -Wno-null-dereference to the linker for this function with -flto */
 void safe_strncpy(char *restrict dst, const char *restrict src, size_t size)
 {

bbs/utils.c

@@ -957,14 +957,14 @@ ssize_t bbs_send_file(const char *filepath, int wfd)
 	return sent;
 }
 
-char *bbs_file_to_string(const char *filename, size_t maxsize, int *length)
+char *bbs_file_to_string(const char *filename, size_t maxsize, int *restrict length)
 {
 	char *s = NULL;
 	FILE *fp;
 	size_t size;
 	size_t res;
 
-	fp = fopen(filename, "r");
+	fp = fopen(filename, "rb");
 	if (!fp) {
 		bbs_error("fopen(%s) failed: %s\n", filename, strerror(errno));
 		return NULL;
@@ -989,8 +989,11 @@ char *bbs_file_to_string(const char *filename, size_t maxsize, int *length)
 	res = fread(s, 1, size, fp);
 	if (res != size) {
 		bbs_error("Wanted to read %lu bytes but only read %lu\n", size, res);
+		free(s);
+		return NULL;
 	}
 	s[res] = '\0'; /* Safe */
+
 cleanup:
 	fclose(fp);
 	return s;

include/node.h

@@ -494,12 +494,24 @@ ssize_t bbs_node_write(struct bbs_node *node, const char *buf, size_t len);
  */
 ssize_t bbs_write(int fd, const char *buf, size_t len);
 
+/*!
+ * \brief Read a specified amount of data from a file descriptor
+ * \param fd File descriptor
+ * \param[out] buf
+ * \param len
+ * \param ms Maximum time in ms to wait after each read call for further data
+ * \retval Same as read
+ */
+ssize_t bbs_timed_read(int fd, char *restrict buf, size_t len, int ms);
+
 /*!
  * \brief Write to a file descriptor without blocking
  * \param fd File descriptor
  * \param buf Buffer to write
  * \param len Length of buf
  * \param ms Maximum number of milliseconds to wait before aborting
+ * \return Number of bytes written to out_fd
+ * \retval -1 on failure
  */
 ssize_t bbs_timed_write(int fd, const char *buf, size_t len, int ms);
 

include/string.h

@@ -84,6 +84,22 @@ int bbs_strncount(const char *restrict s, size_t len, char c) __attribute__ ((pu
  */
 int bbs_term_line(char *restrict c);
 
+/*!
+ * \brief Whether a NUL-terminated string contains bare line feeds (LF characters not immediately preceded by a CR)
+ * \param s String to search
+ * \retval 0 if no bare LFs
+ * \return Number of bare LFs in string
+ */
+int bbs_str_contains_bare_lf(const char *s);
+
+/*!
+ * \brief Convert all bare LFs in a NUL-terminated string to CR LF sequences
+ * \param inbuf Input string
+ * \return NULL on allocation failure
+ * \return Allocated string on success (including no bare LFs), which must be freed
+ */
+char *bbs_str_bare_lf_to_crlf(const char *inbuf);
+
 /*!
  * \brief Size-limited null-terminating string copy.
  * \param dst The destination buffer.

include/utils.h

@@ -299,7 +299,7 @@ ssize_t bbs_send_file(const char *filepath, int wfd);
  * \param[out] length The length of the output string, not including the NUL terminator
  * \returns string on success, NULL on failure
  */
-char *bbs_file_to_string(const char *filename, size_t maxsize, int *length);
+char *bbs_file_to_string(const char *filename, size_t maxsize, int *restrict length);
 
 /*! \brief Get a timeval for the current time */
 struct timeval bbs_tvnow(void);

modules/mod_smtp_delivery_external.c

@@ -398,6 +398,45 @@ static void smtp_tx_data_reset(struct smtp_tx_data *tx)
 	tx->stage = NULL;
 }
 
+#ifdef DEBUG_MAIL_DATA
+#define debug_data(srcfd, offset, writelen) __debug_data(srcfd, offset, writelen, __LINE__)
+static int __debug_data(int srcfd, off_t offset, size_t writelen, int lineno)
+{
+	/* Some built in dumping is included,
+	 * since most connections probably use STARTTLS,
+	 * making it more difficult to use tcpdump / tcpflow to debug. */
+	/* WARNING: This could malloc a lot of data. Do not define DEBUG_MAIL_DATA in production!
+	 * Only compile with it when actively debugging a delivery issue. */
+	char *debugbuf;
+
+	if (lseek(srcfd, offset, SEEK_SET) == -1) {
+		bbs_error("lseek failed: %s\n", strerror(errno));
+		return -1;
+	}
+
+	debugbuf = malloc(writelen + 1); /* NUL terminate for bbs_str_contains_bare_lf */
+	if (ALLOC_SUCCESS(debugbuf)) {
+		ssize_t rres = bbs_timed_read(srcfd, debugbuf, writelen, 50);
+		if (rres > 0) {
+			int bare_lf;
+			debugbuf[writelen] = '\0'; /* NUL terminate for bbs_str_contains_bare_lf */
+			bare_lf = bbs_str_contains_bare_lf(debugbuf);
+			if (bare_lf) {
+				bbs_warning("Line %d: message contains %d bare LF%s and may be rejected by receiving MTA\n", lineno, bare_lf, ESS(bare_lf));
+				bbs_debug(7, "Dumping %ld-byte body:\n", rres);
+				bbs_dump_mem((const unsigned char*) debugbuf, (size_t) rres);
+				free(debugbuf);
+				return 1;
+			}
+		} else if (rres < 0) {
+			bbs_error("read failed: %s\n", strerror(errno));
+		}
+		free(debugbuf);
+	}
+	return 0;
+}
+#endif
+
 /*!
  * \brief Attempt to send an external message to another mail transfer agent or message submission agent
  * \param smtp SMTP session. Generally, this will be NULL except for relayed messages, which are typically the only time this is needed.
@@ -425,11 +464,13 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 	ssize_t wrote = 0;
 	struct bbs_tcp_client client;
 	struct bbs_url url;
-	off_t send_offset = offset;
+	off_t send_offset;
 	int caps = 0, maxsendsize = 0;
 	char sendercopy[64];
 	char *user, *domain, *saslstr = NULL;
 
+#define SMTP_EOM "\r\n.\r\n"
+
 	bbs_assert(datafd != -1);
 	bbs_assert(writelen > 0);
 
@@ -447,6 +488,23 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 	url.host = hostname;
 	url.port = port;
 
+#ifdef DEBUG_MAIL_DATA
+	/* Dump the DATA of the transaction to the CLI for debugging purposes. */
+	if (prepend && prependlen) {
+		bbs_dump_mem((const unsigned char*) prepend, prependlen);
+	}
+	if (debug_data(datafd, offset, writelen)) {
+		/* Proactively reject the message ourselves,
+		 * before even establishing a connection to another MTA,
+		 * which would make us look bad. */
+		if (strstr(hostname, "me.com") || strstr(hostname, "icloud.com")) {
+			snprintf(buf, len, "Bare <LF> detected (in DATA command)");
+			return 1; /* Return permanent failure */
+		}
+	}
+	bbs_dump_mem((const unsigned char*) SMTP_EOM, STRLEN(SMTP_EOM));
+#endif
+
 	tx->prot = "x-tcp";
 	if (bbs_tcp_client_connect(&client, &url, secure, buf, len)) {
 		/* Unfortunately, we can't try an alternate port as there is no provision
@@ -616,10 +674,11 @@ static int try_send(struct smtp_session *smtp, struct smtp_tx_data *tx, const ch
 	}
 
 	/* sendfile will be much more efficient than reading the file ourself, as email body could be quite large, and we don't need to involve userspace. */
+	send_offset = offset;
 	res = (int) bbs_sendfile(client.wfd, datafd, &send_offset, writelen);
 
 	/* XXX If email doesn't end in CR LF, we need to tack that on. But ONLY if it doesn't already end in CR LF. */
-	smtp_client_send(&client, "\r\n.\r\n"); /* (end of) EOM */
+	smtp_client_send(&client, SMTP_EOM); /* (end of) EOM */
 	tx->stage = "end of DATA";
 	if (res != (int) writelen) { /* Failed to write full message */
 		res = -1;
@@ -717,7 +776,7 @@ static int mailq_file_load(struct mailq_file *restrict mqf, const char *dir_name
 	struct stat st;
 
 	snprintf(mqf->fullname, sizeof(mqf->fullname), "%s/%s", dir_name, filename);
-	mqf->fp = fopen(mqf->fullname, "r");
+	mqf->fp = fopen(mqf->fullname, "rb");
 	if (!mqf->fp) {
 		bbs_error("Failed to open %s: %s\n", mqf->fullname, strerror(errno));
 		return -1;
@@ -1207,6 +1266,7 @@ static int external_delivery(struct smtp_session *smtp, struct smtp_response *re
 			return -1;
 		}
 		close(fd);
+
 #ifndef BUGGY_SEND_IMMEDIATE
 		doasync = send_async;
 		if (doasync) {
@@ -1225,9 +1285,10 @@ static int external_delivery(struct smtp_session *smtp, struct smtp_response *re
 				 * Do note that this is mainly a WORKAROUND for BUGGY_SEND_IMMEDIATE. */
 				if (bbs_pthread_create_detached(&sendthread, NULL, smtp_async_send, filenamedup)) {
 					free(filenamedup);
+				} else {
+					bbs_debug(4, "Successfully queued message for immediate delivery: <%s> -> %s\n", from, recipient);
 				}
 			}
-			bbs_debug(4, "Successfully queued message for immediate delivery: <%s> -> %s\n", from, recipient);
 		} else {
 			bbs_debug(4, "Successfully queued message for delayed delivery: <%s> -> %s\n", from, recipient);
 		}

modules/mod_smtp_filter_arc.c

@@ -167,6 +167,12 @@ static int arc_filter_sign_cb(struct smtp_filter_data *f)
 	const char *domain;
 	const unsigned char *error;
 
+	/* If there wasn't already a DKIM-Signature,
+	 * then there's no point in adding ARC headers. */
+	if (!smtp_should_validate_dkim(f->smtp)) {
+		return 0;
+	}
+
 	if (!smtp_message_body(f)) {
 		return 0;
 	}
@@ -221,9 +227,50 @@ static int arc_filter_sign_cb(struct smtp_filter_data *f)
 	 * ARC-Authentication-Results
 	 * However, here they appear in reverse order (amongst these headers). Not actually sure that matters, just pointing it out. */
 	for (; arc_seal; arc_seal = arc_hdr_next(arc_seal)) {
+		char real_header_name[128];
+		const char *colon;
+		char *fixedval;
 		const char *hdr = (const char*) arc_hdr_name(arc_seal, NULL);
 		const char *val = (const char*) arc_hdr_value(arc_seal);
-		smtp_filter_add_header(f, hdr, val);
+		/* libopenarc is broken, in the most horribly way.
+		 * It inserts bare LF's into the ARC-Message-Signature header when adding line breaks,
+		 * rather than CR LF as it should:
+		 * https://github.com/trusteddomainproject/OpenARC/blob/develop/libopenarc/arc.c#L644
+		 *
+		 * THIS IS NOT RFC 5322 COMPLIANT!!!
+		 *
+		 * This may cause other MTAs to reject any mail we send that is ARC signed,
+		 * since the signing is not done properly.
+		 *
+		 * libopenarc is basically abandonware and this is unlikely to be fixed upstream,
+		 * so we convert all bare LFs to CR LFs, after the fact.
+		 *
+		 * Then, to add insult to injury, arc_hdr_name is frequently not parsed right.
+		 * It often includes part of the header value as well (it appears libopenarc
+		 * may be delimiting on ": " rather than ":"...)
+		 * So, work around that as well.
+		 */
+
+		colon = strchr(hdr, ':');
+		if (colon) {
+			bbs_strncpy_until(real_header_name, hdr, sizeof(real_header_name), ':');
+			hdr = real_header_name;
+			bbs_debug(3, "libopenarc did not parse header/value correctly for header '%s', fixing that...\n", hdr);
+		}
+
+		if (bbs_str_contains_bare_lf(val)) {
+			bbs_debug(1, "%s header contains bare LFs, converting them to CR LF for RFC compliance\n", hdr);
+			fixedval = bbs_str_bare_lf_to_crlf(val);
+			if (ALLOC_FAILURE(fixedval)) {
+				/* If allocation fails, skip the header entirely,
+				 * rather than appending an invalid header. */
+				continue;
+			}
+			smtp_filter_add_header(f, hdr, fixedval);
+			free(fixedval);
+		} else {
+			smtp_filter_add_header(f, hdr, val);
+		}
 	}
 
 cleanup: