smtp: Fix bugs and add tests for a couple edge conditions.

Additional tests added:
* test_smtp_msa: Ensure malformed From header is rejected.
* mod_mailscript: Add RELAY test.

Bug fixes, respectively:
* utils.c: Reject email addresses with extraneous trailing '>' characters.
* mod_mailscript: Fix bug in processing nested IF blocks and improve
  EXTRA_DEBUG logs.
* net_smtp: Fix file descriptor leak for RELAY followed by DISCARD.

bbs/utils.c

@@ -285,7 +285,11 @@ int bbs_parse_email_address(char *addr, char **name, char **user, char **host)
 		if (!end) {
 			return -1; /* Email address must be enclosed in <> */
 		}
-		*end = '\0'; /* Now start refers to just the portion in the <> */
+		*end++ = '\0'; /* Now start refers to just the portion in the <> */
+		if (*end) {
+			/* There shouldn't be anything after the trailing '>' */
+			return -1;
+		}
 	} else {
 		start = addr; /* Not enclosed in <> */
 	}

modules/mod_mailscript.c

@@ -135,7 +135,7 @@ static int header_match(struct smtp_msg_process *mproc, const char *header, cons
 			/* This is relatively efficient since we don't do any copying to make comparisons. */
 			found = !strncmp(start, find, findlen); /* Values are case-sensitive */
 #ifdef EXTRA_DEBUG
-			bbs_debug(7, "Comparison(%d) = %.*s with %s\n", found, findlen, start, find);
+			bbs_debug(7, "Comparison(%d) = %.*s with %s\n", found, (int) findlen, start, find);
 #endif
 			start += findlen;
 			if (found && (!strlen_zero(start) && *start != '\r')) {
@@ -343,7 +343,12 @@ static int test_condition(struct smtp_msg_process *mproc, int lineno, int lastre
 	} else {
 		bbs_warning("Invalid condition: %s %s\n", next, S_IF(s));
 	}
-	return negate ? !match : match;
+	match = negate ? !match : match;
+#ifdef EXTRA_DEBUG
+	/* Can't print condition since we mangled it with strsep */
+	bbs_debug(7, "Evaluated condition => %s\n", match ? "1 (TRUE)" : "0 (FALSE)");
+#endif
+	return match;
 }
 
 static int exec_cmd(struct smtp_msg_process *mproc, char *s)
@@ -517,10 +522,10 @@ static int run_rules(struct smtp_msg_process *mproc, const char *rulesfile, cons
 	int multilinecomment = 0;
 	int in_rule = 0;
 	int skip_rule = 0;
-	int want_endif = 0;
+	int if_count = 0; /* Level of nested IF */
+	int want_endif = 0; /* How many ENDIF's we need to encounter before we can start processing lines (we're skipping false blocks) */
 	int retval = 0;
 	int lineno = 0;
-	int if_count = 0;
 
 	if (!bbs_file_exists(rulesfile)) {
 		bbs_debug(7, "MailScript %s doesn't exist\n", rulesfile);
@@ -577,14 +582,24 @@ static int run_rules(struct smtp_msg_process *mproc, const char *rulesfile, cons
 #ifdef EXTRA_DEBUG
 				bbs_debug(6, "if_count=%d, want_endif=%d\n", if_count, want_endif);
 #endif
-				if (want_endif == if_count) {
-					want_endif = 0;
+				if (want_endif) {
+					--want_endif;
 				}
 				if_count--;
 			} else {
 				bbs_warning("No IF block scope at line %d\n", lineno);
 			}
 		} else if (want_endif) {
+			if (STARTS_WITH(s, "IF ")) {
+				/* Don't care what condition is, or whether it's true or false,
+				 * we just need to adjust if_count/want_endif,
+				 * so that we properly handle nested IF blocks. */
+				if_count++;
+				want_endif++;
+			}
+#ifdef EXTRA_DEBUG
+			bbs_debug(10, "Skipping rest of if condition...\n");
+#endif
 			continue;
 		} else if (STARTS_WITH(s, "TEST ")) {
 			s += STRLEN("TEST ");
@@ -633,8 +648,8 @@ static int run_rules(struct smtp_msg_process *mproc, const char *rulesfile, cons
 				cond = !negate;
 			}
 			if (!cond) {
-				want_endif = if_count;
-				bbs_debug(5, "Skipping IF conditional\n");
+				want_endif++;
+				bbs_debug(5, "Skipping IF conditional, condition at line %d is false\n", lineno);
 			}
 		} else {
 			bbs_warning("Invalid command: %s\n", s);
@@ -643,7 +658,7 @@ static int run_rules(struct smtp_msg_process *mproc, const char *rulesfile, cons
 		if (!was_skip && skip_rule) { /* Rule statement just evaluated as false */
 #ifdef EXTRA_DEBUG
 			/* We butchered the rule statement with strsep so can't print it out again */
-			bbs_debug(5, "Skipping rule, condition false\n");
+			bbs_debug(5, "Skipping rule, condition at line %d false\n", lineno);
 #endif
 		}
 	}

nets/net_smtp.c

@@ -2774,6 +2774,7 @@ static int do_deliver(struct smtp_session *smtp, const char *filename, size_t da
 			}
 			bbs_debug(5, "Discarding message and ceasing all further processing\n");
 			free_if(mproc.bouncemsg);
+			close_if(srcfd);
 			return 0;
 		}
 		close_if(srcfd);

tests/.rules

@@ -0,0 +1,7 @@
+RULE
+MATCH DIRECTION OUT
+MATCH HEADER Subject EQUALS Relayed Message
+ACTION RELAY smtp://[email protected]:P@[email protected]:587
+ACTION DISCARD
+ACTION EXIT # Stop processing all rules
+ENDRULE

tests/before.rules

@@ -55,7 +55,7 @@ COMMENT
 	ACTION DISCARD
 ENDCOMMENT
 ACTION NOOP This should be visible
-ACTION EXIT
+ACTION RETURN # Stop processing this rules file
 ENDRULE
 
 RULE

tests/test_mailscript.c

@@ -31,6 +31,7 @@ static int pre(void)
 	test_preload_module("mod_mail.so");
 	test_preload_module("net_smtp.so");
 	test_load_module("mod_smtp_delivery_local.so");
+	test_load_module("mod_smtp_delivery_external.so"); /* In order for RELAY to work */
 	test_load_module("mod_mailscript.so");
 
 	TEST_ADD_CONFIG("mod_auth_static.conf");
@@ -40,22 +41,27 @@ static int pre(void)
 	system("rm -rf /tmp/test_lbbs/maildir"); /* Purge the contents of the directory, if it existed. */
 	mkdir(TEST_MAIL_DIR, 0700); /* Make directory if it doesn't exist already (of course it won't due to the previous step) */
 	system("cp before.rules " TEST_MAIL_DIR); /* Global before MailScript */
+	mkdir(TEST_MAIL_DIR "/1", 0700);
+	system("cp .rules " TEST_MAIL_DIR "/1"); /* Individual user MailScript */
 	return 0;
 }
 
-#define STANDARD_ENVELOPE_BEGIN() \
+#define ENVELOPE_BEGIN(ehlo, mailfrom) \
 	SWRITE(clientfd, "RSET" ENDL); \
 	CLIENT_EXPECT(clientfd, "250"); \
-	SWRITE(clientfd, "EHLO " TEST_EXTERNAL_DOMAIN ENDL); \
+	SWRITE(clientfd, "EHLO " ehlo ENDL); \
 	CLIENT_EXPECT_EVENTUALLY(clientfd, "250 "); \
-	SWRITE(clientfd, "MAIL FROM:<" TEST_EMAIL_EXTERNAL ">\r\n"); \
+	SWRITE(clientfd, "MAIL FROM:<" mailfrom ">\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, "Date: Sun, 1 Jan 2023 05:33:29 -0700" ENDL);
 
+#define STANDARD_ENVELOPE_BEGIN() ENVELOPE_BEGIN(TEST_EXTERNAL_DOMAIN, TEST_EMAIL_EXTERNAL)
+#define CLIENT_ENVELOPE_BEGIN() ENVELOPE_BEGIN("127.0.0.1", TEST_EMAIL2)
+
 #define STANDARD_DATA() \
 	SWRITE(clientfd, ENDL); \
 	SWRITE(clientfd, "This is a test email message." ENDL); \
@@ -221,6 +227,45 @@ static int run(void)
 	CLIENT_EXPECT(clientfd, "250");
 	DIRECTORY_EXPECT_FILE_COUNT(TEST_MAIL_DIR "/1/new", 3); /* Message dropped silently */
 
+	SWRITE(clientfd, "QUIT");
+	close(clientfd);
+
+	/* Relay a message as user 2 */
+	clientfd = test_make_socket(587);
+	if (clientfd < 0) {
+		return -1;
+	}
+
+	CLIENT_EXPECT_EVENTUALLY(clientfd, "220 ");
+
+	/* Test each of the rules in test/before.rules, one by one */
+
+	/* Should be moved to .Junk */
+	SWRITE(clientfd, "EHLO " TEST_USER2 ENDL);
+	CLIENT_EXPECT_EVENTUALLY(clientfd, "250 "); /* "250 " since there may be multiple "250-" responses preceding it */
+
+	/* Log in */
+	SWRITE(clientfd, "AUTH PLAIN\r\n");
+	CLIENT_EXPECT(clientfd, "334");
+	SWRITE(clientfd, TEST_SASL "\r\n");
+	CLIENT_EXPECT(clientfd, "235");
+
+	CLIENT_ENVELOPE_BEGIN();
+	SWRITE(clientfd, "From: " TEST_EMAIL ENDL);
+	SWRITE(clientfd, "Subject: Relayed Message" ENDL);
+	SWRITE(clientfd, "To: " TEST_EMAIL ENDL);
+	STANDARD_DATA();
+	CLIENT_EXPECT(clientfd, "550"); /* Since this message should get relayed, it would actually be submitted as user 2, which is not authorized to send as user 1 */
+	DIRECTORY_EXPECT_FILE_COUNT(TEST_MAIL_DIR "/1/new", 3); /* Message shouldn't have been accepted */
+
+	CLIENT_ENVELOPE_BEGIN();
+	SWRITE(clientfd, "From: " TEST_EMAIL2 ENDL); /* Correct From (for relaying) this time */
+	SWRITE(clientfd, "Subject: Relayed Message" ENDL);
+	SWRITE(clientfd, "To: " TEST_EMAIL ENDL);
+	STANDARD_DATA();
+	CLIENT_EXPECT(clientfd, "250");
+	DIRECTORY_EXPECT_FILE_COUNT(TEST_MAIL_DIR "/1/new", 4); /* Message should have been accepted this time */
+
 	SWRITE(clientfd, "QUIT");
 	res = 0;
 

tests/test_smtp_msa.c

@@ -135,6 +135,22 @@ static int run(void)
 	/* Verify that the email message does NOT exist on disk. */
 	DIRECTORY_EXPECT_FILE_COUNT(TEST_MAIL_DIR "/1/new", -1); /* Folder not created yet */
 
+	/* Malformed From: header (extraneous trailing '>') */
+	if (handshake(clientfd, 1)) {
+		goto cleanup;
+	}
+	SWRITE(clientfd, "MAIL FROM:<" TEST_EMAIL ">\r\n");
+	CLIENT_EXPECT(clientfd, "250");
+	SWRITE(clientfd, "RCPT TO:<" TEST_EMAIL ">\r\n");
+	CLIENT_EXPECT(clientfd, "250");
+	if (send_body(clientfd, "<" TEST_EMAIL ">>")) {
+		goto cleanup;
+	}
+	CLIENT_EXPECT(clientfd, "550");
+
+	/* Verify that the email message does NOT exist on disk. */
+	DIRECTORY_EXPECT_FILE_COUNT(TEST_MAIL_DIR "/1/new", -1); /* Folder not created yet */
+
 	/* All right, let's get it right this time. */
 	if (handshake(clientfd, 1)) {
 		goto cleanup;