getpwnam failure that results in setting errno could confuse the

check for non-existent ~/.forward, so make the check more robust;
fix a fd leak under rare circumstances; use secure_file for
testing .forward file security.

3 files changed, 25 insertions, 36 deletions

diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 207adeb2e23..3a57907672d 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.82 2009/08/07 20:21:48 gilles Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.83 2009/08/27 09:21:28 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -232,7 +232,7 @@ parent_send_config_ruleset(struct smtpd *env, int proc)
 }
 
 void
-parent_dispatch_lka(int fd, short event, void *p)
+parent_dispatch_lka(int imsgfd, short event, void *p)
 {
 	struct smtpd		*env = p;
 	struct imsgev		*iev;
@@ -268,17 +268,20 @@ parent_dispatch_lka(int fd, short event, void *p)
 		switch (imsg.hdr.type) {
 		case IMSG_PARENT_FORWARD_OPEN: {
 			struct forward_req *fwreq = imsg.data;
-			int ret;
+			int fd;
 
 			IMSG_SIZE_CHECK(fwreq);
 
-			ret = parent_forward_open(fwreq->pw_name);
+			fd = parent_forward_open(fwreq->pw_name);
 			fwreq->status = 0;
-			if (ret == -1) {
-				if (errno == ENOENT)
-					fwreq->status = 1;
-			}
-			imsg_compose_event(iev, IMSG_PARENT_FORWARD_OPEN, 0, 0, ret, fwreq, sizeof(*fwreq));
+			if (fd == -2) {
+				/* user has no ~/.forward.  it is optional, so
+				 * set status to ok. */
+				fwreq->status = 1;
+				fd = -1;
+			} else if (fd != -1)
+				fwreq->status = 1;
+			imsg_compose_event(iev, IMSG_PARENT_FORWARD_OPEN, 0, 0, fd, fwreq, sizeof(*fwreq));
 			break;
 		}
 		default:
@@ -1536,7 +1539,6 @@ int
 parent_forward_open(char *username)
 {
 	struct passwd *pw;
-	struct stat sb;
 	char pathname[MAXPATHLEN];
 	int fd;
 
@@ -1545,36 +1547,23 @@ parent_forward_open(char *username)
 		return -1;
 
 	if (! bsnprintf(pathname, sizeof (pathname), "%s/.forward", pw->pw_dir))
-		return -1;
+		fatal("snprintf");
 
 	fd = open(pathname, O_RDONLY);
 	if (fd == -1) {
 		if (errno == ENOENT)
-			goto err;
+			return -2;
+		log_warn("parent_forward_open: %s", pathname);
 		return -1;
 	}
 
-	/* make sure ~/ is not writable by anyone but owner */
-	if (stat(pw->pw_dir, &sb) == -1)
-		goto errlog;
-
-	if (sb.st_uid != pw->pw_uid || sb.st_mode & (S_IWGRP|S_IWOTH))
-		goto errlog;
-
-	/* make sure ~/.forward is not writable by anyone but owner */
-	if (fstat(fd, &sb) == -1)
-		goto errlog;
-
-	if (sb.st_uid != pw->pw_uid || sb.st_mode & (S_IWGRP|S_IWOTH))
-		goto errlog;
+	if (! secure_file(fd, pathname, pw, 1)) {
+		log_warnx("%s: unsecure file", pathname);
+		close(fd);
+		return -1;
+	}
 
 	return fd;
-
-errlog:
-	log_info("cannot process forward file for user %s due to wrong permissions", username);
-
-err:
-	return -1;
 }
 
 int
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index aac0017a85d..bd4f37c0f8e 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.135 2009/08/07 19:02:55 gilles Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.136 2009/08/27 09:21:28 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -939,7 +939,7 @@ char		*ss_to_text(struct sockaddr_storage *);
 int		 valid_message_id(char *);
 int		 valid_message_uid(char *);
 char		*time_to_text(time_t);
-int		 secure_file(int, char *, struct passwd *);
+int		 secure_file(int, char *, struct passwd *, int);
 void		 lowercase(char *, char *, size_t);
 void		 message_set_errormsg(struct message *, char *, ...);
 char		*message_get_errormsg(struct message *);
diff --git a/usr.sbin/smtpd/util.c b/usr.sbin/smtpd/util.c
index e644a63eb6c..80425db3efb 100644
--- a/usr.sbin/smtpd/util.c
+++ b/usr.sbin/smtpd/util.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: util.c,v 1.25 2009/08/08 00:18:38 gilles Exp $	*/
+/*	$OpenBSD: util.c,v 1.26 2009/08/27 09:21:28 jacekm Exp $	*/
 
 /*
  * Copyright (c) 2000,2001 Markus Friedl.  All rights reserved.
@@ -276,7 +276,7 @@ time_to_text(time_t when)
  * Check file for security. Based on usr.bin/ssh/auth.c.
  */
 int
-secure_file(int fd, char *path, struct passwd *pw)
+secure_file(int fd, char *path, struct passwd *pw, int mayread)
 {
 	char		 buf[MAXPATHLEN];
 	char		 homedir[MAXPATHLEN];
@@ -293,7 +293,7 @@ secure_file(int fd, char *path, struct passwd *pw)
 	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)
+	    (st.st_mode & (mayread ? 022 : 066)) != 0)
 		return 0;
 
 	/* For each component of the canonical path, walking upwards. */