- Refuse delivery to mbox that is a symlink, pipe, chardev, etc. etc.

- Introduce secure_file, based on secure_filename from OpenSSH, it
checks that mbox has right perms, and that path components are
trustworthy, too.

ok gilles@

3 files changed, 68 insertions, 9 deletions

diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 313d0d614cd..a296a4a702c 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.37 2009/03/01 21:36:50 jacekm Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.38 2009/03/01 21:58:53 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -943,7 +943,7 @@ int
 parent_mailbox_open(char *path, struct passwd *pw, struct batch *batchp)
 {
 	int fd;
-	int mode = O_CREAT|O_APPEND|O_RDWR|O_EXLOCK|O_NONBLOCK;
+	int mode = O_CREAT|O_APPEND|O_RDWR|O_EXLOCK|O_NONBLOCK|O_NOFOLLOW;
 
 	if (! parent_mailbox_init(pw, path)) {
 		batchp->message.status |= S_MESSAGE_TEMPFAILURE;
@@ -965,23 +965,28 @@ parent_mailbox_open(char *path, struct passwd *pw, struct batch *batchp)
 		case EMFILE:
 		case ENFILE:
 		case ENOSPC:
-			batchp->message.status |= S_MESSAGE_TEMPFAILURE;
-			break;
+			goto tempfail;
 		case EWOULDBLOCK:
-			goto lockfail;
+			batchp->message.status |= S_MESSAGE_LOCKFAILURE;
+			goto tempfail;
 		default:
 			batchp->message.status |= S_MESSAGE_PERMFAILURE;
 		}
 		return -1;
 	}
 
+	if (! secure_file(fd, path, pw)) {
+		log_warnx("refusing delivery to unsecure path: %s", path);
+		goto tempfail;
+	}
+
 	return fd;
 
-lockfail:
+tempfail:
 	if (fd != -1)
 		close(fd);
 
-	batchp->message.status |= S_MESSAGE_TEMPFAILURE|S_MESSAGE_LOCKFAILURE;
+	batchp->message.status |= S_MESSAGE_TEMPFAILURE;
 	return -1;
 }
 
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 3fa2c67cfa5..a5614b3e983 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.78 2009/03/01 12:12:58 jacekm Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.79 2009/03/01 21:58:53 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -912,3 +912,4 @@ int		 valid_domainpart(char *);
 char		*ss_to_text(struct sockaddr_storage *);
 int		 valid_message_id(char *);
 int		 valid_message_uid(char *);
+int		 secure_file(int, char *, struct passwd *);
diff --git a/usr.sbin/smtpd/util.c b/usr.sbin/smtpd/util.c
index 05c5c7bc74e..ed7462eba85 100644
--- a/usr.sbin/smtpd/util.c
+++ b/usr.sbin/smtpd/util.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: util.c,v 1.15 2009/02/24 12:07:47 gilles Exp $	*/
+/*	$OpenBSD: util.c,v 1.16 2009/03/01 21:58:53 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -21,13 +21,16 @@
 #include <sys/queue.h>
 #include <sys/tree.h>
 #include <sys/socket.h>
+#include <sys/stat.h>
 
 #include <ctype.h>
 #include <errno.h>
 #include <event.h>
+#include <libgen.h>
 #include <netdb.h>
 #include <pwd.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
@@ -258,3 +261,53 @@ valid_message_uid(char *muid)
 
 	return (cnt != 0);
 }
+
+/*
+ * Check file for security. Based on usr.bin/ssh/auth.c.
+ */
+int
+secure_file(int fd, char *path, struct passwd *pw)
+{
+	char		 buf[MAXPATHLEN];
+	char		 homedir[MAXPATHLEN];
+	struct stat	 st;
+	char		*cp;
+
+	if (realpath(path, buf) == NULL)
+		return 0;
+
+	if (realpath(pw->pw_dir, homedir) == NULL)
+		homedir[0] = '\0';
+
+	/* Check the open file to avoid races. */
+	if (fstat(fd, &st) < 0 ||
+	    !S_ISREG(st.st_mode) ||
+	    (st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
+	    (st.st_mode & 066) != 0)
+		return 0;
+
+	/* For each component of the canonical path, walking upwards. */
+	for (;;) {
+		if ((cp = dirname(buf)) == NULL)
+			return 0;
+		strlcpy(buf, cp, sizeof(buf));
+
+		if (stat(buf, &st) < 0 ||
+		    (st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
+		    (st.st_mode & 022) != 0)
+			return 0;
+
+		/* We can stop checking after reaching homedir level. */
+		if (strcmp(homedir, buf) == 0)
+			break;
+
+		/*
+		 * dirname should always complete with a "/" path,
+		 * but we can be paranoid and check for "." too
+		 */
+		if ((strcmp("/", buf) == 0) || (strcmp(".", buf) == 0))
+			break;
+	}
+
+	return 1;
+}