there's no good reason to allow smtpd to execute custom command set by root

in a .forward file so disallow custom commands and file reading, only allow
setting forward addresses and users.

as root is no longer allowed to run any MDA but mbox, we can be stricter on
the setup of the MDA process and refuse to exec anything that's not an mbox
dispatcher.

tested by op@ who edited a root envelope to simulate an exploit injecting a
custom command in a root envelope, smtpd refused to exec.

ok millert@ and op@

3 files changed, 20 insertions, 13 deletions

diff --git a/usr.sbin/smtpd/lka_session.c b/usr.sbin/smtpd/lka_session.c
index 60429999a16..a62f6c6bb0e 100644
--- a/usr.sbin/smtpd/lka_session.c
+++ b/usr.sbin/smtpd/lka_session.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: lka_session.c,v 1.98 2023/11/03 13:40:07 op Exp $	*/
+/*	$OpenBSD: lka_session.c,v 1.99 2024/02/02 22:02:12 gilles Exp $	*/
 
 /*
  * Copyright (c) 2011 Gilles Chehade <gilles@poolp.org>
@@ -397,6 +397,7 @@ lka_expand(struct lka_session *lks, struct rule *rule, struct expandnode *xn)
 			break;
 		}
 		xn->realuser = 1;
+		xn->realuser_uid = lk.userinfo.uid;
 
 		if (xn->sameuser && xn->parent->forwarded) {
 			log_trace(TRACE_EXPAND, "expand: lka_expand: same "
@@ -423,6 +424,12 @@ lka_expand(struct lka_session *lks, struct rule *rule, struct expandnode *xn)
 		break;
 
 	case EXPAND_FILENAME:
+		if (xn->parent->realuser && xn->parent->realuser_uid == 0) {
+			log_trace(TRACE_EXPAND, "expand: filename not allowed in root's forward");
+			lks->error = LKA_TEMPFAIL;
+			break;
+		}
+
 		dsp = dict_xget(env->sc_dispatchers, rule->dispatcher);
 		if (dsp->u.local.forward_only) {
 			log_trace(TRACE_EXPAND, "expand: filename matched on forward-only rule");
@@ -451,6 +458,12 @@ lka_expand(struct lka_session *lks, struct rule *rule, struct expandnode *xn)
 		break;
 
 	case EXPAND_FILTER:
+		if (xn->parent->realuser && xn->parent->realuser_uid == 0) {
+			log_trace(TRACE_EXPAND, "expand: filter not allowed in root's forward");
+			lks->error = LKA_TEMPFAIL;
+			break;
+		}
+
 		dsp = dict_xget(env->sc_dispatchers, rule->dispatcher);
 		if (dsp->u.local.forward_only) {
 			log_trace(TRACE_EXPAND, "expand: filter matched on forward-only rule");
diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 024b9b1ea23..789aa17968f 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.347 2024/01/20 09:01:03 claudio Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.348 2024/02/02 22:02:12 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@poolp.org>
@@ -1425,16 +1425,9 @@ forkmda(struct mproc *p, uint64_t id, struct deliver *deliver)
 		pw_dir = deliver->userinfo.directory;
 	}
 
-	if (pw_uid == 0 && deliver->mda_exec[0]) {
-		pw_name = deliver->userinfo.username;
-		pw_uid = deliver->userinfo.uid;
-		pw_gid = deliver->userinfo.gid;
-		pw_dir = deliver->userinfo.directory;
-	}
-
-	if (pw_uid == 0 && !dsp->u.local.is_mbox) {
-		(void)snprintf(ebuf, sizeof ebuf, "not allowed to deliver to: %s",
-		    deliver->userinfo.username);
+	if (pw_uid == 0 && (!dsp->u.local.is_mbox || deliver->mda_exec[0])) {
+		(void)snprintf(ebuf, sizeof ebuf, "MDA not allowed to deliver to: %s",
+			       deliver->userinfo.username);
 		m_create(p_dispatcher, IMSG_MDA_DONE, 0, 0, -1);
 		m_add_id(p_dispatcher, id);
 		m_add_int(p_dispatcher, MDA_PERMFAIL);
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 5e1b16b6a78..a0e8add90a8 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.680 2024/01/03 08:11:15 op Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.681 2024/02/02 22:02:12 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@poolp.org>
@@ -428,6 +428,7 @@ struct expandnode {
 	enum expand_type	type;
 	int			sameuser;
 	int			realuser;
+	uid_t			realuser_uid;
 	int			forwarded;
 	struct rule	       *rule;
 	struct expandnode      *parent;