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.

> However, looking at this code I did notice that I never close the
> libexec/snmpd directory fd. With dup2/closefrom it doesn't need
> closing inside the child.
> 
> martijn@
> 
> diff /usr/src/usr.sbin/snmpd
> commit - 9d92eaaa1531d9e3edc987eead5b5918f22a0790
> path + /usr/src/usr.sbin/snmpd
> blob - 60af191fb6f00ff295542f65ed9c733b66038830
> file + snmpd.c
> --- snmpd.c
> +++ snmpd.c
> @@ -389,4 +389,6 @@ snmpd_backend(struct snmpd *env)
>  			continue;
>  		}
>  	}
> +
> +	closedir(dir);
>  }
> 

Diff is OK claudio@
-- 
:wq Claudio