From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: snmpd closefrom
To: tech@openbsd.org
Date: Tue, 9 Apr 2024 14:51:18 +0200

On Tue, Apr 09, 2024 at 02:20:19PM +0200, Claudio Jeker 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.

I don't know what snmpd is doing with all its file descriptors.
Instead of searching all open(2) and socket(2) calls in the code
and adding O_CLOEXEC, I just fixed the one place where wrong
closefrom(1) is called.

> > Index: usr.sbin/snmpd/snmpd.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/snmpd/snmpd.c,v
> > diff -u -p -r1.51 snmpd.c
> > --- usr.sbin/snmpd/snmpd.c	8 Apr 2024 13:18:54 -0000	1.51
> > +++ usr.sbin/snmpd/snmpd.c	9 Apr 2024 11:56:08 -0000
> > @@ -358,14 +358,10 @@ snmpd_backend(struct snmpd *env)
> >  	}
> >  	if (env->sc_flags & SNMPD_F_VERBOSE)
> >  		argv[i++] = "-vv";
> > -	if (env->sc_flags & SNMPD_F_DEBUG) {
> > +	if (env->sc_flags & SNMPD_F_DEBUG)
> >  		argv[i++] = "-d";
> > -		argv[i++] = "-x";
> > -		argv[i++] = "3";
> > -	} else {
> > -		argv[i++] = "-x";
> > -		argv[i++] = "0";
> > -	}
> > +	argv[i++] = "-x";
> > +	argv[i++] = "3";
> >  	argv[i] = NULL;
> >  	while ((file = readdir(dir)) != NULL) {
> >  		if (file->d_name[0] == '.')
> > @@ -377,10 +373,9 @@ snmpd_backend(struct snmpd *env)
> >  			fatal("fork");
> >  		case 0:
> >  			close(pair[1]);
> > -			if (dup2(pair[0],
> > -			    env->sc_flags & SNMPD_F_DEBUG ? 3 : 0) == -1)
> > +			if (dup2(pair[0], 3) == -1)
> >  				fatal("dup2");
> > -			if (closefrom(env->sc_flags & SNMPD_F_DEBUG ? 4 : 1) == -1)
> > +			if (closefrom(4) == -1)
> >  				fatal("closefrom");
> >  			(void)snprintf(execpath, sizeof(execpath), "%s/%s",
> >  			    SNMPD_BACKEND, file->d_name);
> > 
> 
> -- 
> :wq Claudio