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

[Volker Lendecke]

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.


> >From 6e01f5d8d4c76a5be656a84f06daf699f1d5d4bf Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Wed, 14 May 2014 19:26:00 +1200
> Subject: [PATCH 2/4] selftest: Run pdbtest under valgrind if specified
> 
> Change-Id: I21e169ba563551e13c46f07f86205625ad166c64
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  testprogs/blackbox/test_pdbtest.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testprogs/blackbox/test_pdbtest.sh b/testprogs/blackbox/test_pdbtest.sh
> index af822f9..77865a0 100755
> --- a/testprogs/blackbox/test_pdbtest.sh
> +++ b/testprogs/blackbox/test_pdbtest.sh
> @@ -45,7 +45,7 @@ test_smbclient() {
>  UID_WRAPPER_ROOT=1
>  export UID_WRAPPER_ROOT
>  
> -testit "pdbtest" $BINDIR/pdbtest -u pdbtest || failed=`expr $failed + 1`
> +testit "pdbtest" $VALGRIND $BINDIR/pdbtest -u pdbtest || failed=`expr $failed + 1`

Obvious goodness. Reviewed-by: me.

> >From 781da1158761c55359283aafc6bc55d127770e22 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Fri, 16 May 2014 14:29:43 +1200
> Subject: [PATCH 3/4] auth: Allow auth_samba4 to be forced to run a specific
>  auth module
> 
> This will allow new tests to be written to validate winbindd authentication results

Not commenting on the other two yet, this needs more time
for me to look at. Maybe someone else steps in before me?
:-)

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de