ensure that rules files are owned and writable only by root,

along their entire path, change docs accordingly. This ensures
that people don't accidentally use the $HOME config files to
override real settings unless root meant to do it.

2 files changed, 103 insertions, 27 deletions

diff --git a/usr.sbin/authpf/authpf.8 b/usr.sbin/authpf/authpf.8
index d57e0418a52..9cf5e73f42b 100644
--- a/usr.sbin/authpf/authpf.8
+++ b/usr.sbin/authpf/authpf.8
@@ -1,4 +1,4 @@
-\" $OpenBSD: authpf.8,v 1.6 2002/04/02 17:29:47 mpech Exp $
+\" $OpenBSD: authpf.8,v 1.7 2002/04/05 03:06:52 beck Exp $
 .\"
 .\" Copyright (c) 2002 Bob Beck (beck@openbsd.org>.  All rights reserved.
 .\"
@@ -95,6 +95,17 @@ which is defined to the connecting ip address whenever
 .Nm
 is run.
 .Pp
+Filter and nat rules will be searched for first in
+.Pa $HOME/.authpf/
+and then in 
+.Pa /etc/authpf/ .
+Per-user rules from the   
+.Pa $HOME/.authpf/
+directory are intended to be used when non-default rules
+are needed on an individual user basis. It is important to ensure
+that a user can not write or change these configuration files in
+this case.
+.Pp
 Filter rules are loaded from the file
 .Pa $HOME/.authpf/authpf.rules .
 If this file does not exist the file
@@ -102,13 +113,14 @@ If this file does not exist the file
 is used.
 The
 .Pa authpf.rules
-file must exist in either the user's
-.Pa $HOME/.authpf/
-directory, or in
-.Pa /etc/authpf ,
-for
+file must exist in one of the above locations for
 .Nm
-to run.
+to run. Additionally, all directories on the path to the 
+.Pa authpf.rules 
+file as well as the file itself must be owned by root
+and be writable only to root or
+.Nm 
+will not run.
 .Pp
 Translation rules are loaded from the file
 .Pa $HOME/.authpf/authpf.nat .
@@ -117,7 +129,13 @@ If this file does not exist the file
 is used.
 The use of translation rules in an
 .Pa authpf.nat
-file is optional.
+file is optional, but if an
+.Pa authpf.nat
+file exists in either of the above locations, all directories on the path,
+as well as the file itself must be 
+owned by root and be writable only to root or
+.Nm
+will not run
 .Sh CONFIGURATION
 Options are controlled by the
 .Pa /etc/authpf/authpf.conf
diff --git a/usr.sbin/authpf/authpf.c b/usr.sbin/authpf/authpf.c
index 4b8fafefa59..6c8d0547d0a 100644
--- a/usr.sbin/authpf/authpf.c
+++ b/usr.sbin/authpf/authpf.c
@@ -45,6 +45,8 @@
 #include <err.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <libgen.h>
+#include <login_cap.h>
 #include <netdb.h>
 #include <signal.h>
 #include <stdio.h>
@@ -90,6 +92,7 @@ static int	changefilter(int, char *, char *);
 static void	authpf_kill_states(void);
 static void	terminator(int s);
 static __dead void	go_away(void);
+static int	secure_fullpath(char *);
 
 /*
  * authpf:
@@ -296,6 +299,10 @@ read_config(void)
 	if (f == NULL) 
 		exit(1); /* exit silently if we have no config file */
 
+	if (secure_fullpath(configfile) != 0)
+		/* config file exists, but is not secure */
+		exit(1);
+
 	openlog("authpf", LOG_PID | LOG_NDELAY, LOG_DAEMON);
 	
 	do {
@@ -600,17 +607,22 @@ changefilter(int add, char *luser, char *ipsrc)
 			if (unlink(template) == -1)
 				syslog(LOG_ERR, "can't unlink %s", template);
 			goto error;
-		}
+		}			
 	}
-	snprintf(rulesfile, sizeof rulesfile, PATH_PFRULES);
-	if (from_fd == -1 &&
-	    (from_fd = open(rulesfile, O_RDONLY, 0)) == -1) {
-		syslog(LOG_ERR, "can't open %s (%m)", rulesfile);
-		if (unlink(template) == -1)
-			syslog(LOG_ERR, "can't unlink %s", template);
-		goto error;
+	if (from_fd == -1) {
+		snprintf(rulesfile, sizeof rulesfile, PATH_PFRULES);
+		if  ((from_fd = open(rulesfile, O_RDONLY, 0)) == -1) {
+			syslog(LOG_ERR, "can't open %s (%m)", rulesfile);
+			if (unlink(template) == -1)
+				syslog(LOG_ERR, "can't unlink %s", template);
+			goto error;
+		}
 	}
 
+	if (secure_fullpath(rulesfile) != 0)
+		/* rules file exists, but is not secure */
+		goto error;
+	
 	while ((rcount = read(from_fd, buf, sizeof(buf))) > 0) {
 		wcount = write(tmpfile, buf, rcount);
 		if (rcount != wcount || wcount == -1) {
@@ -660,7 +672,6 @@ changefilter(int add, char *luser, char *ipsrc)
 	}
 
 	/* now, for NAT, if we have some */
-
 	if ((cp = getenv("HOME")) == NULL) {
 		syslog(LOG_ERR, "No Home Directory!");
 		goto error;
@@ -679,18 +690,24 @@ changefilter(int add, char *luser, char *ipsrc)
 			goto error;
 		}
 	}
-	snprintf(natfile, sizeof natfile, PATH_NATRULES);
-	if (from_fd == -1 &&
-	    (from_fd = open(natfile, O_RDONLY, 0)) == -1) {
-		if (errno == ENOENT)
-			goto out; /* NAT is optional */
-		else {
-			syslog(LOG_ERR, "can't open %s (%m)", natfile);
-			if (unlink(template) == -1)
-				syslog(LOG_ERR, "can't unlink %s", template);
-			goto error;
+	if (from_fd == -1) {
+		snprintf(natfile, sizeof natfile, PATH_NATRULES);
+		if ((from_fd = open(natfile, O_RDONLY, 0)) == -1) {
+			if (errno == ENOENT)
+				goto out; /* NAT is optional */
+			else {
+				syslog(LOG_ERR, "can't open %s (%m)", natfile);
+				if (unlink(template) == -1)
+					syslog(LOG_ERR, "can't unlink %s",
+					    template);
+				goto error;
+			}
 		}
 	}
+	if (from_fd != -1 && secure_fullpath(natfile) != 0) 
+		/* nat file exists, but is not secure */
+		goto error;
+	
 	tmpfile = mkstemp(template2);
 	if (tmpfile == -1) {
 		syslog(LOG_ERR, "Can't open temp file %s (%m)",
@@ -838,6 +855,47 @@ go_away(void)
 }
 
 /*
+ * secure_fullpath:
+ * akin to secure_path, but for a directory - needed to ensure
+ * users can't get something they aren't supposed to by moveing
+ * files aside or linking other directories, such as the default
+ * one.
+ */
+static int
+secure_fullpath(char *path)
+{
+	struct stat sb;
+	char *cp;
+
+	if (secure_path(path) < 0)
+		return(-1);
+
+	cp = path;
+
+	do {
+		cp = dirname(cp);
+		memset(&sb, 0, sizeof(sb)); 
+		/*
+		 * if it's owned or writable by someone
+		 * other than root, it's bad. since these are directories,
+		 * not the end path, they are allowed to be symbolic links
+		 * and other such things (unlike the file itself). 
+		 */
+		if (lstat(cp, &sb) < 0) {
+			syslog(LOG_ERR, "cannot stat %s: %m", cp);
+			return (-1);
+		} else if (sb.st_uid != 0) {
+			syslog(LOG_ERR, "%s: not owned by root", cp);
+			return (-1);
+		} else if (sb.st_mode & (S_IWGRP | S_IWOTH)) {
+			syslog(LOG_ERR, "%s: writeable by non-root", cp);
+			return (-1);
+		}
+	} while (strlen(cp) > 1);
+        return (0);
+}
+
+/*
  * pfctl_add_rules:
  * callback for rule add, used by parser in parse_rules
  */