mod_smtp_delivery_external: Fix outbound AUTH handling.

* Use AUTH PLAIN only if AUTH PLAIN is supported, not AUTH LOGIN.
* mod_mailscript: Fix regression from commit 9327c0c
  that prevented MailScript rules from running on mail submissions,
  which have a COMBINED scope.
* mod_smtp_client: Fix spurious warning about unknown capability.

modules/mod_mailscript.c

@@ -38,7 +38,7 @@
 
 #define REQUIRE_ARG(s) \
 	if (strlen_zero(s)) { \
-		bbs_warning("Incomplete condition on line %d\n", lineno); \
+		bbs_warning("Incomplete condition on line %d (%s must be nonempty)\n", lineno, #s); \
 		return 0; \
 	}
 
@@ -660,6 +660,11 @@ static int mailscript(struct smtp_msg_process *mproc)
 	char filepath[256];
 	const char *mboxmaildir;
 
+	/* COMBINED scope is only for outbound mail submissions */
+	if (mproc->scope != SMTP_SCOPE_INDIVIDUAL && mproc->dir != SMTP_DIRECTION_SUBMIT) {
+		return 0;
+	}
+
 	/* Calculate maildir path, if we have a mailbox */
 	if (mproc->userid) {
 		snprintf(filepath, sizeof(filepath), "%s/%d", mailbox_maildir(NULL), mproc->userid);
@@ -701,12 +706,14 @@ static int mailscript(struct smtp_msg_process *mproc)
 struct smtp_message_processor proc = {
 	.callback = mailscript,
 	.dir = SMTP_DIRECTION_ALL,
-	/* Filters are only run for individual delivery.
+	/* Filters are typically only run for individual delivery.
 	 * Even global rules should use SMTP_SCOPE_INDIVIDUAL,
 	 * since they could manipulate the mailbox in some way,
 	 * and we don't have a single mailbox if processing
-	 * a message that will get delivered to multiple recipients. */
-	.scope = SMTP_SCOPE_INDIVIDUAL,
+	 * a message that will get delivered to multiple recipients.
+	 * However, mail submissions do use COMBINED scope, as they are not run per-recipient,
+	 * since in that case the mailbox belongs to the sender, not the recipient. */
+	.scope = SMTP_SCOPE_INDIVIDUAL | SMTP_SCOPE_COMBINED,
 	.iteration = FILTER_ALL_PASSES, /* We handle all passes, with more granular logic in the callback */
 };
 

modules/mod_smtp_client.c

@@ -100,7 +100,12 @@ static void process_capabilities(int *restrict caps, int *restrict maxsendsize,
 	} else if (!strcmp(capname, "OK")) {
 		/* This is not a real capability, just ignore it. Yahoo seems to do this. */
 	} else {
-		/* Capabilities should be all uppercase and a single word */
+		/* Capabilities should be all uppercase, and could be one or multiple words
+		 * The first line is often the hostname of the server followed by some friendly greeting, ignore.
+		 * This callback doesn't know if it's the first line, but the hostname should have at least one period */
+		if (strchr(capname, '.')) {
+			return; /* Ignore, probably the hostname banner, not a real capability */
+		}
 		if (!strchr(capname, ' ')) {
 			bbs_warning("Unknown capability advertised: %s\n", capname);
 		}

modules/mod_smtp_delivery_external.c

@@ -382,7 +382,7 @@ static int __attribute__ ((nonnull (2, 3, 9, 17))) try_send(struct smtp_session
 	struct bbs_smtp_client smtpclient;
 	off_t send_offset;
 	char sendercopy[64];
-	char *user, *domain, *saslstr = NULL;
+	char *user, *domain, *saslstr = NULL; /* saslstr is scoped here for cleanup */
 
 #define SMTP_EOM "\r\n.\r\n"
 
@@ -537,7 +537,7 @@ static int __attribute__ ((nonnull (2, 3, 9, 17))) try_send(struct smtp_session
 					goto cleanup;
 				}
 			}
-		} else if (smtpclient.caps & SMTP_CAPABILITY_AUTH_LOGIN) {
+		} else if (smtpclient.caps & SMTP_CAPABILITY_AUTH_PLAIN) {
 			saslstr = bbs_sasl_encode(username, username, password);
 			if (!saslstr) {
 				res = -1;
@@ -546,6 +546,38 @@ static int __attribute__ ((nonnull (2, 3, 9, 17))) try_send(struct smtp_session
 			bbs_smtp_client_send(&smtpclient, "AUTH PLAIN\r\n"); /* AUTH PLAIN is preferred to the deprecated AUTH LOGIN */
 			SMTP_EXPECT(&smtpclient, SEC_MS(10), "334");
 			bbs_smtp_client_send(&smtpclient, "%s\r\n", saslstr);
+			SMTP_EXPECT(&smtpclient, SEC_MS(10), "235");
+		} else if (smtpclient.caps & SMTP_CAPABILITY_AUTH_LOGIN) {
+			char *encoded;
+			int encodedlen;
+			/* AUTO LOGIN is obsoleted in favor of AUTH PLAIN, so only use as last resort */
+			bbs_smtp_client_send(&smtpclient, "AUTH LOGIN\r\n");
+
+			/* In both cases, we free before SMTP_EXPECT to avoid memory leaks on failure. */
+
+			/* Send username */
+			SMTP_EXPECT(&smtpclient, SEC_MS(10), "334");
+			encoded = base64_encode(username, (int) strlen(username), &encodedlen);
+			if (!encoded) {
+				bbs_error("Base64 encoding failed\n");
+				res = -1;
+				goto cleanup;
+			}
+			bbs_smtp_client_send(&smtpclient, "%s\r\n", encoded);
+			free(encoded);
+
+			/* Send password */
+			SMTP_EXPECT(&smtpclient, SEC_MS(10), "334");
+			encoded = base64_encode(password, (int) strlen(password), &encodedlen);
+			if (!encoded) {
+				bbs_error("Base64 encoding failed\n");
+				res = -1;
+				goto cleanup;
+			}
+			bbs_smtp_client_send(&smtpclient, "%s\r\n", encoded);
+			bbs_memzero(encoded, strlen(encoded)); /* Destroy encoded password */
+			free(encoded);
+
 			SMTP_EXPECT(&smtpclient, SEC_MS(10), "235");
 		} else {
 			bbs_warning("No mutual login methods available\n");
@@ -610,7 +642,11 @@ static int __attribute__ ((nonnull (2, 3, 9, 17))) try_send(struct smtp_session
 	/* RFC 5321 4.5.3.2.6 */
 	SMTP_CLIENT_EXPECT_FINAL(&smtpclient, MIN_MS(10), "250"); /* Okay, this email is somebody else's problem now. */
 
-	bbs_debug(3, "Message successfully delivered to %s\n", recipient);
+	if (recipient) {
+		bbs_debug(3, "Message successfully delivered to %s\n", recipient);
+	} else { /* recipients (which are already freed now) */
+		bbs_debug(3, "Message successfully delivered\n");
+	}
 	res = 0;
 
 cleanup:
@@ -1868,7 +1904,7 @@ static int relay(struct smtp_session *smtp, struct smtp_msg_process *mproc, int
 		res = -1;
 	} else {
 		struct smtp_tx_data tx; /* Capture but ignore */
-		char buf[132];
+		char buf[512];
 		char prepend[256] = "";
 		int prependlen = 0;
 		char timestamp[40];

nets/net_smtp.c

@@ -786,6 +786,7 @@ static int handle_auth(struct smtp_session *smtp, char *s)
 		}
 		bbs_strterm((char*) user, '@'); /* Strip domain */
 		res = bbs_authenticate(smtp->node, (char*) user, (char*) pass);
+		bbs_memzero((unsigned char*) pass, strlen((char*) pass)); /* Destroy the password from memory before we free it */
 		free(user);
 		free(pass);
 		goto logindone;
@@ -1638,6 +1639,11 @@ int __smtp_register_processor(struct smtp_message_processor *processor, void *mo
 {
 	struct smtp_processor *proc;
 
+	if (!processor->callback) {
+		bbs_error("Processor has no callback?\n");
+		return -1;
+	}
+
 	proc = calloc(1, sizeof(*proc));
 	if (ALLOC_FAILURE(proc)) {
 		return -1;
@@ -1673,8 +1679,9 @@ static inline int __run_callbacks(struct smtp_msg_process *mproc, enum msg_proce
 
 	mproc->iteration = iteration;
 
-	__bbs_log(LOG_DEBUG, 3, file, line, func, "Running SMTP callbacks for %s scope, %s direction, %s pass\n",
+	__bbs_log(LOG_DEBUG, 3, file, line, func, "Running SMTP callbacks for %s scope, %s (%s) direction, %s pass\n",
 		mproc->scope == SMTP_SCOPE_INDIVIDUAL ? "INDIVIDUAL" : "COMBINED",
+		mproc->direction == SMTP_MSG_DIRECTION_IN ? "IN" : "OUT",
 		mproc->dir == SMTP_DIRECTION_IN ? "IN" : mproc->dir == SMTP_DIRECTION_SUBMIT ? "SUBMIT" : "OUT",
 		mproc->iteration == FILTER_BEFORE_MAILBOX ? "pre-mailbox" : mproc->iteration == FILTER_AFTER_MAILBOX ? "post-mailbox" : "mailbox");
 
@@ -1692,7 +1699,7 @@ static inline int __run_callbacks(struct smtp_msg_process *mproc, enum msg_proce
 		 * The module can't unregister the processor without WRLOCK'ing the list,
 		 * and we have it locked for this traversal. */
 		bbs_module_ref(proc->mod, 3);
-		res = processor->callback(mproc);
+		res = processor->callback(mproc); /* callback is guaranteed to be non-NULL here */
 		bbs_module_unref(proc->mod, 3);
 		if (res) {
 			__bbs_log(LOG_DEBUG, 4, file, line, func, "Message processor returned %d\n", res);
@@ -1712,7 +1719,10 @@ int __smtp_run_callbacks(struct smtp_msg_process *mproc, enum smtp_filter_scope
 	 * with some always executing before or after a mailbox's rules. */
 	RWLIST_RDLOCK(&processors);
 	res |= __run_callbacks(mproc, FILTER_BEFORE_MAILBOX, file, line, func);
-	if (!res && scope == SMTP_SCOPE_INDIVIDUAL) {
+	if (!res && mproc->userid) {
+		/* The mailbox pass can happen for both INDIVIDUAL and COMBINED scope.
+		 * mproc->mbox is also NULL for submissions.
+		 * mproc->userid is the best thing to use. */
 		res |= __run_callbacks(mproc, FILTER_MAILBOX, file, line, func);
 	}
 	if (!res) {
@@ -2616,8 +2626,8 @@ static int do_deliver(struct smtp_session *smtp, const char *filename, size_t da
 		int srcfd = -1;
 		smtp_mproc_init(smtp, &mproc);
 		mproc.size = (int) datalen;
-		mproc.dir = smtp->msa ? SMTP_DIRECTION_SUBMIT : SMTP_DIRECTION_OUT;
-		mproc.direction = SMTP_MSG_DIRECTION_OUT;
+		mproc.dir = SMTP_DIRECTION_SUBMIT;
+		mproc.direction = SMTP_MSG_DIRECTION_OUT; /* It's an outbound message from the user */
 		mproc.mbox = NULL;
 		mproc.userid = smtp->node->user->id;
 		mproc.user = smtp->node->user;
@@ -2731,7 +2741,7 @@ static int do_deliver(struct smtp_session *smtp, const char *filename, size_t da
 						continue;
 					}
 					bbs_module_ref(h->mod, 6);
-					/* provided by mod_smtp_delivery_local */
+					/* provided by mod_smtp_delivery_external */
 					res = h->agent->relay(smtp, &mproc, srcfd, datalen, &smtp->recipients);
 					bbs_module_unref(h->mod, 6);
 					if (!res) {