[PATCH] Simplify chroot code in smbd

[Andreas Schneider]

On Wednesday 10 February 2016 14:31:53 Jeremy Allison wrote:
> On Wed, Feb 10, 2016 at 04:25:29PM +0100, Andreas Schneider wrote:
> > Review and push appreciated ...
> > 
> > Thanks,
> > 
> > 	-- andreas
> 
> Andreas - I think this:
> 
> 	chroot_dir = lp_root_directory(talloc_tos());
> 	if (chroot_dir != NULL) {
> 
> is incorrect. lp_XXX() strings don't return NULL
> if a parameter is unset, they return a pointer to '\0'.
> 
> So this should be:
> 
> 	chroot_dir = lp_root_directory(talloc_tos());
> 	if (*chroot_dir != '\0') {

Yes, you're right.

> 
> > From 2afc034fb7857b96702186327b6f8d75562f478a Mon Sep 17 00:00:00 2001
> > From: Andreas Schneider <asn at samba.org>
> > Date: Wed, 10 Feb 2016 16:19:56 +0100
> > Subject: [PATCH] smbd: Simplify chroot option in smbd
> > 
> > rpmlint has a check for this and prefers to call chdir() before
> > chroot(). If not it will complain with
> > missing-call-to-chdir-with-chroot. The old code equivalent secure. See
> > 
> >     http://unixwiz.net/techtips/chroot-practices.html
> > 
> > This removes several unneeded talloc_tos() calls.
> > 
> > Signed-off-by: Andreas Schneider <asn at samba.org>
> > ---
> > 
> >  source3/smbd/process.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/source3/smbd/process.c b/source3/smbd/process.c
> > index 25c6d05..8668ee4 100644
> > --- a/source3/smbd/process.c
> > +++ b/source3/smbd/process.c
> > @@ -3903,6 +3903,8 @@ void smbd_process(struct tevent_context *ev_ctx,
> > 
> >  	NTSTATUS status;
> >  	struct timeval tv = timeval_current();
> >  	NTTIME now = timeval_to_nttime(&tv);
> > 
> > +	const char *chroot_dir = NULL;
> > +	int rc;
> > 
> >  	status = smbXsrv_client_create(ev_ctx, ev_ctx, msg_ctx, now, &client);
> >  	if (!NT_STATUS_IS_OK(status)) {
> > 
> > @@ -4024,17 +4026,22 @@ void smbd_process(struct tevent_context *ev_ctx,
> > 
> >  		exit_server("Could not open account policy tdb.\n");
> >  	
> >  	}
> > 
> > -	if (*lp_root_directory(talloc_tos())) {
> > -		if (chroot(lp_root_directory(talloc_tos())) != 0) {
> > -			DEBUG(0,("Failed to change root to %s\n",
> > -				 lp_root_directory(talloc_tos())));
> > -			exit_server("Failed to chroot()");
> > +	chroot_dir = lp_root_directory(talloc_tos());
> > +	if (chroot_dir != NULL) {
> > +		rc = chdir(chroot_dir);
> > +		if (rc != 0) {
> > +			DBG_ERR("Failed to chdir to %s\n", chroot_dir);
> > +			exit_server("Failed to chdir()");
> > 
> >  		}
> > 
> > -		if (chdir("/") == -1) {
> > -			DEBUG(0,("Failed to chdir to / on chroot to %s\n",
> > lp_root_directory(talloc_tos()))); +
> > +		rc = chroot(chroot_dir);
> > +		if (rc != 0) {
> > +			DBG_ERR("Failed to change root to %s\n", chroot_dir);
> > 
> >  			exit_server("Failed to chroot()");
> >  		
> >  		}
> > 
> > -		DEBUG(0,("Changed root to %s\n", lp_root_directory(talloc_tos())));
> > +		DBG_WARNING("Changed root to %s\n", chroot_dir);
> > +
> > +		TALLOC_FREE(chroot_dir);
> > 
> >  	}
> >  	
> >  	if (!file_init(sconn)) {

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org