[PATCH] Fix winbind connections as non-root and test winbind auth modules

[Andrew Bartlett]

On Fri, 2014-05-16 at 09:31 +0200, Volker Lendecke wrote:
> On Fri, May 16, 2014 at 02:47:12PM +1200, Andrew Bartlett wrote:
> > This patch series adds a new baseline test to validate winbind auth
> > modules, to ensure that they continue to return the correct details
> > while we change winbindd itself.
> > 
> > In writing this, I found a nasty little bug (caused by a
> > misinterpretation of the requirements) in the use of the new UID_WRAPPER
> > stuff.   That is, a check for root ownership of the socket (or current
> > user) was mis-configured to be some kind of privilege check.  
> > 
> > Please review/push.
> > 
> > Thanks,
> > 
> > Andrew Bartlett
> > -- 
> > Andrew Bartlett
> > http://samba.org/~abartlet/
> > Authentication Developer, Samba Team  http://samba.org
> > Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba
> > 
> > 
> > 
> 
> > >From 065475acda8c7d59b5a8a21f4043435e350b1361 Mon Sep 17 00:00:00 2001
> > From: Andrew Bartlett <abartlet at samba.org>
> > Date: Fri, 16 May 2014 14:23:15 +1200
> > Subject: [PATCH 1/4] nsswitch: Correct check for pipe ownership in wb_common
> > 
> > This is a security check for ownership, not a check for privileged
> > access.  This may have broken non-root use of winbindd and did break
> > the addition of tests using the winbindd client socket in some
> > situations.
> > 
> > Andrew Bartlett
> > 
> > Change-Id: Ic0d2ab75a9b0ab8e8c87bfe296683306a48c53a2
> > Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> > ---
> >  nsswitch/wb_common.c | 43 ++++++++++---------------------------------
> >  1 file changed, 10 insertions(+), 33 deletions(-)
> > 
> > diff --git a/nsswitch/wb_common.c b/nsswitch/wb_common.c
> > index b34ab33..9b05a9b 100644
> > --- a/nsswitch/wb_common.c
> > +++ b/nsswitch/wb_common.c
> > @@ -168,35 +168,6 @@ static int make_safe_fd(int fd)
> >  	return new_fd;
> >  }
> >  
> > -/**
> > - * @internal
> > - *
> > - * @brief Check if we have priviliged access.
> > - *
> > - * This checks if we have uid_wrapper running and if yes turns it of so that we
> > - * can check if we have access.
> > - *
> > - * @param[in]  uid      The uid to compare if we have access.
> > - *
> > - * @return              If we have access it returns true, else false.
> > - */
> > -static bool winbind_privileged_access(uid_t uid)
> > -{
> > -	uid_t euid;
> > -
> > -	if (uid_wrapper_enabled()) {
> > -		setenv("UID_WRAPPER_MYUID", "1", 1);
> > -	}
> > -
> > -	euid = geteuid();
> > -
> > -	if (uid_wrapper_enabled()) {
> > -		unsetenv("UID_WRAPPER_MYUID");
> > -	}
> > -
> > -	return (uid == euid);
> > -}
> > -
> >  /* Connect to winbindd socket */
> >  
> >  static int winbind_named_pipe_sock(const char *dir)
> > @@ -215,9 +186,12 @@ static int winbind_named_pipe_sock(const char *dir)
> >  		return -1;
> >  	}
> >  
> > -	/* This tells uid_wrapper to return the userid for the geteuid check */
> > +	/* 
> > +	 * This tells us that the pipe is owned by a privileged
> > +	 * process, as we will be sending passwords to it 
> > +	 */
> >  	if (!S_ISDIR(st.st_mode) ||
> > -	    !winbind_privileged_access(st.st_uid)) {
> > +	    (st.st_uid != 0 && !uid_wrapper_enabled())) {
> 
> Me being scared of even mildly complex if-expressions would
> rather see this stay in a properly named routine. There you
> could do a 
> 
> if (st.st_uid == 0) {
> 	return true;
> }
> 
> ... do other commented stuff.
> 
> In particular as this code is duplicated.

While I have your attention on this, do we need it at all?

Now that the winbind pipe is no longer in /tmp, do you think the client
should be asserting who the owner is?

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba