~/.forward files handling was fixed recently so that it is the privileged

process that does the opening, this commit does some cleanup, and fixes a
bug I experienced today which was caused by a use-after-free.

I did some testing to make sure a user cannot cause smtpd to deadlock, or
loop, with broken setups (self-referencing forwards/aliases, empty files,
broken files...), but if you are playing with aliases/forwards PLEASE let
me know of any bug you run into.

4 files changed, 104 insertions, 152 deletions

diff --git a/usr.sbin/smtpd/lka.c b/usr.sbin/smtpd/lka.c
index 5e04bf671d6..521e33d446f 100644
--- a/usr.sbin/smtpd/lka.c
+++ b/usr.sbin/smtpd/lka.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: lka.c,v 1.31 2009/03/04 00:00:40 gilles Exp $	*/
+/*	$OpenBSD: lka.c,v 1.32 2009/03/08 17:54:20 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Pierre-Yves Ritschard <pyr@openbsd.org>
@@ -60,7 +60,8 @@ int		lka_match_mask(struct sockaddr_storage *, struct netaddr *);
 int		aliases_virtual_get(struct smtpd *, struct aliaseslist *, struct path *);
 int		aliases_virtual_exist(struct smtpd *, struct path *);
 int		lka_resolve_path(struct smtpd *, struct path *);
-int		lka_expand_aliases(struct smtpd *, struct aliaseslist *, struct lkasession *);
+void		lka_expand_rcpt(struct smtpd *, struct aliaseslist *, struct lkasession *);
+int		lka_expand_rcpt_iteration(struct smtpd *, struct aliaseslist *, struct lkasession *);
 void		lka_rcpt_action(struct smtpd *, struct path *);
 void		lka_clear_aliaseslist(struct aliaseslist *);
 
@@ -115,12 +116,9 @@ lka_dispatch_parent(int sig, short event, void *p)
 		switch (imsg.hdr.type) {
 		case IMSG_PARENT_FORWARD_OPEN: {
 			int fd;
-			int ret;
 			struct forward_req	*fwreq;
 			struct lkasession	key;
 			struct lkasession	*lkasession;
-			struct alias		*alias;
-			struct message		message;
 
 			fwreq = imsg.data;
 
@@ -133,73 +131,18 @@ lka_dispatch_parent(int sig, short event, void *p)
 
 			if (fd == -1) {
 				if (fwreq->pw_name[0] != '\0') {
-					lkasession->ss->code = 530;
+					lkasession->ss.code = 530;
 					lkasession->flags |= F_ERROR;
 				}
 			}
 			else {
 				if (! forwards_get(fd, &lkasession->aliaseslist)) {
-					lkasession->ss->code = 530;
+					lkasession->ss.code = 530;
 					lkasession->flags |= F_ERROR;
 				}
 				close(fd);
 			}
-
-			ret = 0;
-			while (! (lkasession->flags & F_ERROR) &&
-			    ! lkasession->pending && lkasession->iterations < 5) {
-				++lkasession->iterations;
-				ret = lka_expand_aliases(env, &lkasession->aliaseslist, lkasession);
-				if (ret == -1) {
-					lkasession->ss->code = 530;
-					lkasession->flags |= F_ERROR;
-				}
-
-				if (lkasession->pending || ret <= 0)
-					break;
-			}
-
-			if (lkasession->pending)
-				break;
-
-			if (lkasession->flags & F_ERROR) {
-				lka_clear_aliaseslist(&lkasession->aliaseslist);
-				imsg_compose(ibuf, IMSG_LKA_RCPT, 0, 0,
-				    -1, lkasession->ss, sizeof(*lkasession->ss));
-			}
-			else if (ret == 0) {
-				log_debug("expansion resulted in empty list");
-				message = lkasession->message;
-				message.recipient = lkasession->path;
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
-				    &message, sizeof(struct message));
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1,
-				    &message, sizeof(struct message));
-			}
-			else {
-				log_debug("a list of aliases is available");
-				message = lkasession->message;
-				while ((alias = TAILQ_FIRST(&lkasession->aliaseslist)) != NULL) {
-					bzero(&message.recipient, sizeof(struct path));
-					
-					lka_resolve_alias(&message.recipient, alias);
-					lka_rcpt_action(env, &message.recipient);
-					
-					imsg_compose(env->sc_ibufs[PROC_QUEUE],
-					    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
-					    &message, sizeof(struct message));
-					
-					TAILQ_REMOVE(&lkasession->aliaseslist, alias, entry);
-					free(alias);
-				}
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1, &message,
-				    sizeof(struct message));
-			}
-			SPLAY_REMOVE(lkatree, &env->lka_sessions, lkasession);
-			free(lkasession);
+			lka_expand_rcpt(env, &lkasession->aliaseslist, lkasession);
 			break;
 		}
 		default:
@@ -267,9 +210,8 @@ lka_dispatch_mfa(int sig, short event, void *p)
 		}
 		case IMSG_LKA_RCPT: {
 			struct submit_status	*ss;
-			struct message		 message;
+			struct message		message;
 			struct lkasession	*lkasession;
-			struct alias		*alias;
 			struct forward_req	 fwreq;
 			int ret;
 
@@ -303,11 +245,11 @@ lka_dispatch_mfa(int sig, short event, void *p)
 			lkasession->id = queue_generate_id();
 			lkasession->path = ss->u.path;
 			lkasession->message = ss->msg;
-			lkasession->ss = ss;
+			lkasession->ss = *ss;
+
 			TAILQ_INIT(&lkasession->aliaseslist);
 
 			SPLAY_INSERT(lkatree, &env->lka_sessions, lkasession);
-			log_debug("LKA SESSION !");
 
 			ret = 0;
 			if (lkasession->path.flags & F_ACCOUNT) {
@@ -336,60 +278,8 @@ lka_dispatch_mfa(int sig, short event, void *p)
 				break;
 			}
 
-			ret = 0;
-			while (! lkasession->pending && lkasession->iterations < 5) {
-				++lkasession->iterations;
-				ret = lka_expand_aliases(env, &lkasession->aliaseslist, lkasession);
-				if (ret == -1) {
-					lkasession->ss->code = 530;
-					lkasession->flags |= F_ERROR;
-				}
+			lka_expand_rcpt(env, &lkasession->aliaseslist, lkasession);
 
-				if (lkasession->pending || ret <= 0)
-					break;
-			}
-
-			if (lkasession->pending)
-				break;
-
-			if (lkasession->flags & F_ERROR) {
-				lka_clear_aliaseslist(&lkasession->aliaseslist);
-				imsg_compose(ibuf, IMSG_LKA_RCPT, 0, 0,
-				    -1, lkasession->ss, sizeof(*lkasession->ss));
-			}
-			else if (ret == 0) {
-				log_debug("expansion resulted in empty list");
-				message = ss->msg;
-				message.recipient = ss->u.path;
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
-				    &message, sizeof(struct message));
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1,
-				    &message, sizeof(struct message));
-			}
-			else {
-				log_debug("a list of aliases is available");
-				message = ss->msg;
-				while ((alias = TAILQ_FIRST(&lkasession->aliaseslist)) != NULL) {
-					bzero(&message.recipient, sizeof(struct path));
-					
-					lka_resolve_alias(&message.recipient, alias);
-					lka_rcpt_action(env, &message.recipient);
-					
-					imsg_compose(env->sc_ibufs[PROC_QUEUE],
-					    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
-					    &message, sizeof(struct message));
-					
-					TAILQ_REMOVE(&lkasession->aliaseslist, alias, entry);
-					free(alias);
-				}
-				imsg_compose(env->sc_ibufs[PROC_QUEUE],
-				    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1, &message,
-				    sizeof(struct message));
-			}
-			SPLAY_REMOVE(lkatree, &env->lka_sessions, lkasession);
-			free(lkasession);
 			break;
 		}
 		default:
@@ -960,8 +850,72 @@ lka_resolve_alias(struct path *path, struct alias *alias)
 	return 1;
 }
 
+void
+lka_expand_rcpt(struct smtpd *env, struct aliaseslist *aliases, struct lkasession *lkasession)
+{
+	int	ret;
+	struct alias	*alias;
+	struct message	message;
+
+	ret = 0;
+	while (! (lkasession->flags & F_ERROR) &&
+	    ! lkasession->pending && lkasession->iterations < 5) {
+		++lkasession->iterations;
+		ret = lka_expand_rcpt_iteration(env, &lkasession->aliaseslist, lkasession);
+		if (ret == -1) {
+			lkasession->ss.code = 530;
+			lkasession->flags |= F_ERROR;
+		}
+		
+		if (lkasession->pending || ret <= 0)
+			break;
+	}
+	
+	if (lkasession->pending)
+		return;
+	
+	if (lkasession->flags & F_ERROR) {
+		lka_clear_aliaseslist(&lkasession->aliaseslist);
+		imsg_compose(env->sc_ibufs[PROC_MFA], IMSG_LKA_RCPT, 0, 0,
+		    -1, &lkasession->ss, sizeof(struct submit_status));
+	}
+	else if (ret == 0) {
+		log_debug("expansion resulted in empty list");
+		message = lkasession->message;
+		message.recipient = lkasession->path;
+		imsg_compose(env->sc_ibufs[PROC_QUEUE],
+		    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
+		    &message, sizeof(struct message));
+		imsg_compose(env->sc_ibufs[PROC_QUEUE],
+		    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1,
+		    &message, sizeof(struct message));
+	}
+	else {
+		log_debug("a list of aliases is available");
+		message = lkasession->message;
+		while ((alias = TAILQ_FIRST(&lkasession->aliaseslist)) != NULL) {
+			bzero(&message.recipient, sizeof(struct path));
+			
+			lka_resolve_alias(&message.recipient, alias);
+			lka_rcpt_action(env, &message.recipient);
+			
+			imsg_compose(env->sc_ibufs[PROC_QUEUE],
+			    IMSG_QUEUE_SUBMIT_ENVELOPE, 0, 0, -1,
+			    &message, sizeof(struct message));
+			
+			TAILQ_REMOVE(&lkasession->aliaseslist, alias, entry);
+			free(alias);
+		}
+		imsg_compose(env->sc_ibufs[PROC_QUEUE],
+		    IMSG_QUEUE_COMMIT_ENVELOPES, 0, 0, -1, &message,
+		    sizeof(struct message));
+	}
+	SPLAY_REMOVE(lkatree, &env->lka_sessions, lkasession);
+	free(lkasession);
+}
+
 int
-lka_expand_aliases(struct smtpd *env, struct aliaseslist *aliases, struct lkasession *lkasession)
+lka_expand_rcpt_iteration(struct smtpd *env, struct aliaseslist *aliases, struct lkasession *lkasession)
 {
 	u_int8_t done = 1;
 	struct alias *rmalias = NULL;
@@ -1005,8 +959,10 @@ lka_expand_aliases(struct smtpd *env, struct aliaseslist *aliases, struct lkases
 		rmalias = NULL;
 	}
 
-	if (!done && lkasession->iterations == 5)
+	if (!done && lkasession->iterations == 5) {
+		log_debug("loop detected");
 		return -1;
+	}
 
 	if (TAILQ_FIRST(aliases) == NULL)
 		return 0;
diff --git a/usr.sbin/smtpd/mfa.c b/usr.sbin/smtpd/mfa.c
index 412bd322762..52187013c6a 100644
--- a/usr.sbin/smtpd/mfa.c
+++ b/usr.sbin/smtpd/mfa.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: mfa.c,v 1.15 2009/02/22 19:07:33 chl Exp $	*/
+/*	$OpenBSD: mfa.c,v 1.16 2009/03/08 17:54:20 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -214,14 +214,12 @@ mfa_dispatch_lka(int sig, short event, void *p)
 			struct submit_status	 *ss;
 
 			ss = imsg.data;
-			if (ss->msg.flags & F_MESSAGE_ENQUEUED) {
+			if (ss->msg.flags & F_MESSAGE_ENQUEUED)
 				imsg_compose(env->sc_ibufs[PROC_CONTROL], IMSG_MFA_RCPT,
 				    0, 0, -1, ss, sizeof(*ss));
-			}
-			else {
+			else
 				imsg_compose(env->sc_ibufs[PROC_SMTP], IMSG_MFA_RCPT,
 				    0, 0, -1, ss, sizeof(*ss));
-			}
 			break;
 		}
 		default:
diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 98b4bdade42..6d3217e8683 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.41 2009/03/04 00:00:40 gilles Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.42 2009/03/08 17:54:20 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -200,7 +200,6 @@ parent_dispatch_lka(int fd, short event, void *p)
 			if (ret == -1)
 				if (errno == ENOENT)
 					fwreq->pw_name[0] = '\0';
-			log_debug("parent will return fd %d", ret);
 			imsg_compose(ibuf, IMSG_PARENT_FORWARD_OPEN, 0, 0, ret, fwreq, sizeof(*fwreq));
 			break;
 		}
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 5390f76aa43..23ad369074f 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.83 2009/03/06 23:45:00 gilles Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.84 2009/03/08 17:54:20 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -620,28 +620,6 @@ struct session {
 
 };
 
-struct forward_req {
-	u_int64_t			 id;
-	char				 pw_name[MAXLOGNAME];
-};
-
-enum lkasession_flags {
-	F_ERROR		= 0x1
-};
-
-struct lkasession {
-	SPLAY_ENTRY(lkasession)		 nodes;
-	u_int64_t			 id;
-
-	struct path			 path;
-	struct aliaseslist		 aliaseslist;
-	u_int8_t			 iterations;
-	u_int32_t			 pending;
-	enum lkasession_flags		 flags;
-	struct message			 message;
-	struct submit_status		 *ss;
-};
-
 struct smtpd {
 #define SMTPD_OPT_VERBOSE			 0x00000001
 #define SMTPD_OPT_NOACTION			 0x00000002
@@ -737,6 +715,27 @@ struct message_recipient {
 	struct message			 msg;
 };
 
+struct forward_req {
+	u_int64_t			 id;
+	char				 pw_name[MAXLOGNAME];
+};
+
+enum lkasession_flags {
+	F_ERROR		= 0x1
+};
+
+struct lkasession {
+	SPLAY_ENTRY(lkasession)		 nodes;
+	u_int64_t			 id;
+
+	struct path			 path;
+	struct aliaseslist		 aliaseslist;
+	u_int8_t			 iterations;
+	u_int32_t			 pending;
+	enum lkasession_flags		 flags;
+	struct message			 message;
+	struct submit_status		 ss;
+};
 
 /* aliases.c */
 int aliases_exist(struct smtpd *, char *);