From: Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject: Re: snmpd closefrom
To: tech@openbsd.org
Date: Tue, 23 Apr 2024 12:32:13 +0200

On Wed, 2024-04-17 at 16:11 +0200, Claudio Jeker wrote:
> On Wed, Apr 17, 2024 at 02:07:29PM +0200, Martijn van Duren wrote:
> > On Fri, 2024-04-12 at 17:48 +0200, Alexander Bluhm wrote:
> > > On Tue, Apr 09, 2024 at 07:33:58AM -0600, Theo de Raadt wrote:
> > > > Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> > > > 
> > > > > On Tue, Apr 09, 2024 at 02:13:30PM +0200, Alexander Bluhm wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > fstat output shows that snmpd_metrics uses file descriptors 0, 1,
> > > > > > 2 for regular communication.  This should not happen as any output
> > > > > > to stderr would interfere with other data.
> > > > > > 
> > > > > > stdin, stdout, stderr are reserverd.  They should point to a terminal
> > > > > > or /dev/null.  Redirects to other files is also fine.  But closing
> > > > > > and then opening some files or sockets to 0, 1, 2 is not allowed.
> > > > > > 
> > > > > > The closefrom(1) in snmpd is the culprit.  With closefrom(4)
> > > > > > descriptors 0, 1, 2 are /dev/null, 3 is a socketpair shared with
> > > > > > the parent, and higher numbers are used for other files.
> > > > > > 
> > > > > > ok?
> > > > > 
> > > > > Why call closefrom() in the first place? If the code used O_CLOEXEC etc
> > > > > there would be no need for that. snmpd_backend() is called once from
> > > > > main() in the setup code so it feels strange to need closefrom() there.
> > > > 
> > > >    ^^^  Yep.  It smells.
> > > 
> > > I committed my quick fix with correct closefrom() argument.  Of
> > > course getting rid of closefrom would be better.
> > > 
> > > Any volunteers?  martijn?
> > > 
> > The closefrom() is needed because proc.c doesn't put O_CLOEXEC on its
> > imsg sockepairs, and parent<->snmpe would leak into snmpd_metrics.
> > And even if we decide to put the O_CLOEXEC on these sockets I reckon
> > it's still good practice in case someone adds some additional fd
> > in the future (for whatever yet unknown reason) and forgets to place
> > O_CLOEXEC on it.
> 
> This reasoning is somewhat wrong. Privsep fork/exec daemons should use
> O_CLOEXEC all the time. Not doing so is a bug. closefrom() is for the
> cases where you have no clue what fds where inherited and the process just
> wants a "clean" slate.
> In some cases one can not use O_CLOEXEC but then the parent should call
> fcntl F_SETFD with FD_CLOEXEC after.
> 
> 
So my reasoning above wasn't correct and I bodged something else in my
test: proc.c does set O_CLOEXEC.

While I still don't see how this approach should be considered a bug,
here's a diff to change it. With this approach there's no need to do
the dup2 dance.

While from testing it looks like the opendir()'s fd has the CLOEXEC bit
set, POSIX states that this is implementation dependant, so I explicitly
close it as well from the child process.

martijn@

Index: snmpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.c,v
retrieving revision 1.52
diff -u -p -r1.52 snmpd.c
--- snmpd.c	12 Apr 2024 14:17:42 -0000	1.52
+++ snmpd.c	23 Apr 2024 10:31:07 -0000
@@ -22,6 +22,7 @@
 #include <dirent.h>
 #include <err.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <pwd.h>
 #include <signal.h>
 #include <stdio.h>
@@ -343,9 +344,9 @@ snmpd_backend(struct snmpd *env)
 {
 	DIR *dir;
 	struct dirent *file;
-	int pair[2];
+	int pair[2], ce;
 	char *argv[8];
-	char execpath[PATH_MAX];
+	char execpath[PATH_MAX], xnum[11];
 	size_t i = 0;
 
 	if ((dir = opendir(SNMPD_BACKEND)) == NULL)
@@ -361,22 +362,26 @@ snmpd_backend(struct snmpd *env)
 	if (env->sc_flags & SNMPD_F_DEBUG)
 		argv[i++] = "-d";
 	argv[i++] = "-x";
-	argv[i++] = "3";
+	argv[i++] = xnum;
 	argv[i] = NULL;
 	while ((file = readdir(dir)) != NULL) {
 		if (file->d_name[0] == '.')
 			continue;
-		if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) == -1)
+		if (socketpair(AF_UNIX,
+		    SOCK_STREAM | SOCK_CLOEXEC, PF_UNSPEC, pair) == -1)
 			fatal("socketpair");
 		switch (fork()) {
 		case -1:
 			fatal("fork");
 		case 0:
+			closedir(dir);
 			close(pair[1]);
-			if (dup2(pair[0], 3) == -1)
-				fatal("dup2");
-			if (closefrom(4) == -1)
-				fatal("closefrom");
+			if (fcntl(pair[0], F_GETFD, &ce) == -1)
+				fatal("fcntl");
+			ce &= ~FD_CLOEXEC;
+			if (fcntl(pair[0], F_SETFD, ce) == -1)
+				fatal("fcntl");
+			(void)snprintf(xnum, sizeof(xnum), "%d", pair[0]);
 			(void)snprintf(execpath, sizeof(execpath), "%s/%s",
 			    SNMPD_BACKEND, file->d_name);
 			execv(argv[0], argv);
@@ -389,4 +394,6 @@ snmpd_backend(struct snmpd *env)
 			continue;
 		}
 	}
+
+	closedir(dir);
 }