Hello,

After investigating a user bug, I figured that there's a small change that could make OpenSMTPD
far more resistant to privileges escalation when a bug happens.

OpenSMTPD only accepts mail deliveries to root with mbox MDA because OpenBSD expects root to be
able to receive mail out of the box on a new install for the welcome mail as well as the daily,
weekly and monthly mail... but if you try to deliver to root with another MDA, say maildir, you
will be greeted with an error forcing you to forward root mail to an unprivileged user.

However, root is still allowed to use forward files and these allow to plug alternate MDA which
are executed with the user permission (root here), so basically root can workaround restriction
by executing maildir within a forward file.

I don't think there's any good reason for root to plug commands in a forward file, nor is there
any good reason for smtpd to execute root commands from a forward file. By preventing root from
using commands in forward files, we can make smtpd's MDA execution more tight: if you are root,
and the mda is not "mail.local", do not execute the mda.

Why prevent root from shooting itself in the foot with a bad decision ?

It also raises the bar for attacks and makes it much more complicated for attackers to inject a
custom command in an envelope in hope it gets executed by root even if they've obtained control
of the queue process somehow.

I believe, this would have prevented the root escalation exploit that happened 5 years ago.

The diff below:

a- forbids the inclusion of a filename or command in a privileged user's .forward
b- disallow execution of an MDA that's not mbox for privileged users with no override possible,
   be it from an mda wrapper or a .forward (second later check, just in case).

You can still add addresses or users in root's forward file.

This could still be abused if someone managed to corrupt memory enough that the dispatcher will
have its default command point to another one, but if this gets in I'll craft another diff that
will perform a literal const check for root's mbox MDA killing the remaining potential.

Tested on my laptop but could use some more testing.

Opinion ?





Index: lka_session.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/lka_session.c,v
retrieving revision 1.98
diff -u -p -r1.98 lka_session.c
--- lka_session.c	3 Nov 2023 13:40:07 -0000	1.98
+++ lka_session.c	29 Jan 2024 21:44:09 -0000
@@ -397,6 +397,7 @@ lka_expand(struct lka_session *lks, stru
 			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, stru
 		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, stru
 		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");
Index: smtpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
retrieving revision 1.347
diff -u -p -r1.347 smtpd.c
--- smtpd.c	20 Jan 2024 09:01:03 -0000	1.347
+++ smtpd.c	29 Jan 2024 21:44:09 -0000
@@ -1425,16 +1425,9 @@ forkmda(struct mproc *p, uint64_t id, st
 		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);
Index: smtpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
retrieving revision 1.680
diff -u -p -r1.680 smtpd.h
--- smtpd.h	3 Jan 2024 08:11:15 -0000	1.680
+++ smtpd.h	29 Jan 2024 21:44:09 -0000
@@ -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;