[PATCH v3 0/2] gssd: allow gssd to work with KEYRING: credcaches

[PATCH v3 0/2] gssd: allow gssd to work with KEYRING: credcaches

[Jeff Layton]

Changes since original set:
v3:
- have parent check to see if child was signalled and log a warning if so
- drop supplimentary groups and change gid before acquiring creds. Keep
  suid and sgid as well to hamper ptrace.

v2:
- fix bisectability. The original set added includes in the wrong
  place in patch #1 and then fixed it in patch #2. The final result
  of this set is the same but should bisect cleanly.

This patchset fixes up gssd to work with KEYRING: style credcaches. At
the same time, it also fixes gssd not to need to trawl through likely
credcache locations by allowing GSSAPI to find them in the intended
fashion.

The basic idea is to have gssd fork() after reading off the pipe, but
before handling the upcall and to do a more thorough job of changing
credentials.

Jeff Layton (2):
  gssd: have process_krb5_upcall fork before handling upcall
  gssd: do a more thorough change of identity after forking

 utils/gssd/gssd_proc.c | 106 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/2] gssd: have process_krb5_upcall fork before handling upcall

[Jeff Layton]

Most krb5 installations use credcache locations that contain %{uid},
which expands to the real UID of the current process. In order for
GSSAPI to find those properly, we need to be able to switch the real UID
of the process to the designated one. That however, opens the door to
allowing gssd to be killed or reniced during the window where we've
switched credentials.

To combat this, change gssd to fork before trying to handle each upcall.
The child will do the work to establish the context and the parent task
will just wait for it to exit. It's still possible for the child to be
killed or reniced, but that would only affect a single upcall instead of
the entire daemon. Also, If the process is killed prematurely, then log
an error to tip off the admin that there was a problem.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e58c341..99537d9 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -67,6 +67,8 @@
 #include <errno.h>
 #include <gssapi/gssapi.h>
 #include <netdb.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 
 #include "gssd.h"
 #include "err_util.h"
@@ -982,6 +984,26 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
 	int			err, downcall_err = -EACCES;
 	gss_cred_id_t		gss_cred;
 	OM_uint32		maj_stat, min_stat, lifetime_rec;
+	pid_t			pid;
+
+	pid = fork();
+	switch(pid) {
+	case 0:
+		/* Child: fall through to rest of function */
+		break;
+	case -1:
+		/* fork() failed! */
+		printerr(0, "WARNING: unable to fork() to handle upcall: %s\n",
+				strerror(errno));
+		return;
+	default:
+		/* Parent: just wait on child to exit and return */
+		wait(&err);
+		if (WIFSIGNALED(err))
+			printerr(0, "WARNING: forked child was killed with signal %d\n",
+					WTERMSIG(err));
+		return;
+	}
 
 	printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
 
@@ -1121,7 +1143,7 @@ out:
 		AUTH_DESTROY(auth);
 	if (rpc_clnt)
 		clnt_destroy(rpc_clnt);
-	return;
+	exit(0);
 
 out_return_error:
 	do_error_downcall(fd, uid, downcall_err);
-- 
1.8.3.1

Re: [PATCH v3 1/2] gssd: have process_krb5_upcall fork before handling upcall

[Steve Dickson]

On 09/10/13 16:21, Jeff Layton wrote:
> Most krb5 installations use credcache locations that contain %{uid},
> which expands to the real UID of the current process. In order for
> GSSAPI to find those properly, we need to be able to switch the real UID
> of the process to the designated one. That however, opens the door to
> allowing gssd to be killed or reniced during the window where we've
> switched credentials.
> 
> To combat this, change gssd to fork before trying to handle each upcall.
> The child will do the work to establish the context and the parent task
> will just wait for it to exit. It's still possible for the child to be
> killed or reniced, but that would only affect a single upcall instead of
> the entire daemon. Also, If the process is killed prematurely, then log
> an error to tip off the admin that there was a problem.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Committed...

steved.

> ---
>  utils/gssd/gssd_proc.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index e58c341..99537d9 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -67,6 +67,8 @@
>  #include <errno.h>
>  #include <gssapi/gssapi.h>
>  #include <netdb.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
>  
>  #include "gssd.h"
>  #include "err_util.h"
> @@ -982,6 +984,26 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  	int			err, downcall_err = -EACCES;
>  	gss_cred_id_t		gss_cred;
>  	OM_uint32		maj_stat, min_stat, lifetime_rec;
> +	pid_t			pid;
> +
> +	pid = fork();
> +	switch(pid) {
> +	case 0:
> +		/* Child: fall through to rest of function */
> +		break;
> +	case -1:
> +		/* fork() failed! */
> +		printerr(0, "WARNING: unable to fork() to handle upcall: %s\n",
> +				strerror(errno));
> +		return;
> +	default:
> +		/* Parent: just wait on child to exit and return */
> +		wait(&err);
> +		if (WIFSIGNALED(err))
> +			printerr(0, "WARNING: forked child was killed with signal %d\n",
> +					WTERMSIG(err));
> +		return;
> +	}
>  
>  	printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
>  
> @@ -1121,7 +1143,7 @@ out:
>  		AUTH_DESTROY(auth);
>  	if (rpc_clnt)
>  		clnt_destroy(rpc_clnt);
> -	return;
> +	exit(0);
>  
>  out_return_error:
>  	do_error_downcall(fd, uid, downcall_err);
> 

[PATCH v3 2/2] gssd: do a more thorough change of identity after forking

[Jeff Layton]

The part of process_krb5_upcall that handles non-machine user creds
first tries to query GSSAPI for credentials. If that fails, it then
falls back to trawling through likely credcache locations to find them
and then points $KRB5CCNAME at it before proceeding. There are a number
of bugs in this code that this patch attempts to address.

The code that queries GSSAPI for credentials does it as root which
almost universally fails to do anything useful unless we happen to be
looking for non-machine root creds. Because of this, gssd almost always
falls back to having to search for credcaches "manually". The code that
handles credential switching is in create_auth_rpc_client, so it's too
late to be of any use here.

Worse yet, for historical reasons the MIT krb5 authors used %{uid} in
the default credcache locations which translates to the real uid. Thus
switching the fsuid or even euid is insufficient. You must switch the
real uid in order to be able to find the proper credcache in most cases.

This patch moves the credential switching to occur much earlier in the
process and has it do a much more thorough job of it. It first drops all
supplimentary groups, then determines a gid to use and switches the gids
and uids to the correct ones. If it can't determine the correct gid to
use, it then tries to look up the one for "nobody" and uses that.

Once the credentials are switched, the forked child now no longer tries
to change them back. It does the downcall with the new credentials and
just exits when it's done.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/gssd/gssd_proc.c | 82 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 16 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 99537d9..b48d163 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -834,7 +834,6 @@ create_auth_rpc_client(struct clnt_info *clp,
 	CLIENT			*rpc_clnt = NULL;
 	struct rpc_gss_sec	sec;
 	AUTH			*auth = NULL;
-	uid_t			save_uid = -1;
 	int			retval = -1;
 	OM_uint32		min_stat;
 	char			rpc_errmsg[1024];
@@ -843,16 +842,6 @@ create_auth_rpc_client(struct clnt_info *clp,
 	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
 	socklen_t		salen;
 
-	/* Create the context as the user (not as root) */
-	save_uid = geteuid();
-	if (setfsuid(uid) != 0) {
-		printerr(0, "WARNING: Failed to setfsuid for "
-			    "user with uid %d\n", uid);
-		goto out_fail;
-	}
-	printerr(2, "creating context using fsuid %d (save_uid %d)\n",
-			uid, save_uid);
-
 	sec.qop = GSS_C_QOP_DEFAULT;
 	sec.svc = RPCSEC_GSS_SVC_NONE;
 	sec.cred = cred;
@@ -951,11 +940,6 @@ create_auth_rpc_client(struct clnt_info *clp,
   out:
 	if (sec.cred != GSS_C_NO_CREDENTIAL)
 		gss_release_cred(&min_stat, &sec.cred);
-	/* Restore euid to original value */
-	if (((int)save_uid != -1) && (setfsuid(save_uid) != (int)uid)) {
-		printerr(0, "WARNING: Failed to restore fsuid"
-			    " to uid %d from %d\n", save_uid, uid);
-	}
 	return retval;
 
   out_fail:
@@ -966,6 +950,64 @@ create_auth_rpc_client(struct clnt_info *clp,
 }
 
 /*
+ * Create the context as the user (not as root).
+ *
+ * Note that we change the *real* uid here, as changing the effective uid is
+ * not sufficient. This is due to an unfortunate historical error in the MIT
+ * krb5 libs, where they used %{uid} in the default_ccache_name. Changing that
+ * now might break some applications so we're sort of stuck with it.
+ *
+ * Unfortunately, doing this leaves the forked child vulnerable to signals and
+ * renicing, but this is the best we can do. In the event that a child is
+ * signalled before downcalling, the kernel will just eventually time out the
+ * upcall attempt.
+ */
+static int
+change_identity(uid_t uid)
+{
+	struct passwd	*pw;
+
+	/* drop list of supplimentary groups first */
+	if (setgroups(0, NULL) != 0) {
+		printerr(0, "WARNING: unable to drop supplimentary groups!");
+		return errno;
+	}
+
+	/* try to get pwent for user */
+	pw = getpwuid(uid);
+	if (!pw) {
+		/* if that doesn't work, try to get one for "nobody" */
+		errno = 0;
+		pw = getpwnam("nobody");
+		if (!pw) {
+			printerr(0, "WARNING: unable to determine gid for uid %u\n", uid);
+			return errno ? errno : ENOENT;
+		}
+	}
+
+	/*
+	 * Switch the GIDs. Note that we leave the saved-set-gid alone in an
+	 * attempt to prevent attacks via ptrace()
+	 */
+	if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
+		printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
+		return errno;
+	}
+
+	/*
+	 * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
+	 * other processes running with this uid.
+	 */
+	if (setresuid(uid, uid, -1) != 0) {
+		printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
+				uid);
+		return errno;
+	}
+
+	return 0;
+}
+
+/*
  * this code uses the userland rpcsec gss library to create a krb5
  * context on behalf of the kernel
  */
@@ -1036,6 +1078,14 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
 		 service ? service : "<null>");
 	if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
 				service == NULL)) {
+
+		err = change_identity(uid);
+		if (err) {
+			printerr(0, "WARNING: failed to change identity: %s",
+				 strerror(err));
+			goto out_return_error;
+		}
+
 		/* Tell krb5 gss which credentials cache to use */
 		/* Try first to acquire credentials directly via GSSAPI */
 		err = gssd_acquire_user_cred(uid, &gss_cred);
-- 
1.8.3.1

Re: [PATCH v3 2/2] gssd: do a more thorough change of identity after forking

[Steve Dickson]

On 09/10/13 16:21, Jeff Layton wrote:
> The part of process_krb5_upcall that handles non-machine user creds
> first tries to query GSSAPI for credentials. If that fails, it then
> falls back to trawling through likely credcache locations to find them
> and then points $KRB5CCNAME at it before proceeding. There are a number
> of bugs in this code that this patch attempts to address.
> 
> The code that queries GSSAPI for credentials does it as root which
> almost universally fails to do anything useful unless we happen to be
> looking for non-machine root creds. Because of this, gssd almost always
> falls back to having to search for credcaches "manually". The code that
> handles credential switching is in create_auth_rpc_client, so it's too
> late to be of any use here.
> 
> Worse yet, for historical reasons the MIT krb5 authors used %{uid} in
> the default credcache locations which translates to the real uid. Thus
> switching the fsuid or even euid is insufficient. You must switch the
> real uid in order to be able to find the proper credcache in most cases.
> 
> This patch moves the credential switching to occur much earlier in the
> process and has it do a much more thorough job of it. It first drops all
> supplimentary groups, then determines a gid to use and switches the gids
> and uids to the correct ones. If it can't determine the correct gid to
> use, it then tries to look up the one for "nobody" and uses that.
> 
> Once the credentials are switched, the forked child now no longer tries
> to change them back. It does the downcall with the new credentials and
> just exits when it's done.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Committed... 

steved.

> ---
>  utils/gssd/gssd_proc.c | 82 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 99537d9..b48d163 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -834,7 +834,6 @@ create_auth_rpc_client(struct clnt_info *clp,
>  	CLIENT			*rpc_clnt = NULL;
>  	struct rpc_gss_sec	sec;
>  	AUTH			*auth = NULL;
> -	uid_t			save_uid = -1;
>  	int			retval = -1;
>  	OM_uint32		min_stat;
>  	char			rpc_errmsg[1024];
> @@ -843,16 +842,6 @@ create_auth_rpc_client(struct clnt_info *clp,
>  	struct sockaddr		*addr = (struct sockaddr *) &clp->addr;
>  	socklen_t		salen;
>  
> -	/* Create the context as the user (not as root) */
> -	save_uid = geteuid();
> -	if (setfsuid(uid) != 0) {
> -		printerr(0, "WARNING: Failed to setfsuid for "
> -			    "user with uid %d\n", uid);
> -		goto out_fail;
> -	}
> -	printerr(2, "creating context using fsuid %d (save_uid %d)\n",
> -			uid, save_uid);
> -
>  	sec.qop = GSS_C_QOP_DEFAULT;
>  	sec.svc = RPCSEC_GSS_SVC_NONE;
>  	sec.cred = cred;
> @@ -951,11 +940,6 @@ create_auth_rpc_client(struct clnt_info *clp,
>    out:
>  	if (sec.cred != GSS_C_NO_CREDENTIAL)
>  		gss_release_cred(&min_stat, &sec.cred);
> -	/* Restore euid to original value */
> -	if (((int)save_uid != -1) && (setfsuid(save_uid) != (int)uid)) {
> -		printerr(0, "WARNING: Failed to restore fsuid"
> -			    " to uid %d from %d\n", save_uid, uid);
> -	}
>  	return retval;
>  
>    out_fail:
> @@ -966,6 +950,64 @@ create_auth_rpc_client(struct clnt_info *clp,
>  }
>  
>  /*
> + * Create the context as the user (not as root).
> + *
> + * Note that we change the *real* uid here, as changing the effective uid is
> + * not sufficient. This is due to an unfortunate historical error in the MIT
> + * krb5 libs, where they used %{uid} in the default_ccache_name. Changing that
> + * now might break some applications so we're sort of stuck with it.
> + *
> + * Unfortunately, doing this leaves the forked child vulnerable to signals and
> + * renicing, but this is the best we can do. In the event that a child is
> + * signalled before downcalling, the kernel will just eventually time out the
> + * upcall attempt.
> + */
> +static int
> +change_identity(uid_t uid)
> +{
> +	struct passwd	*pw;
> +
> +	/* drop list of supplimentary groups first */
> +	if (setgroups(0, NULL) != 0) {
> +		printerr(0, "WARNING: unable to drop supplimentary groups!");
> +		return errno;
> +	}
> +
> +	/* try to get pwent for user */
> +	pw = getpwuid(uid);
> +	if (!pw) {
> +		/* if that doesn't work, try to get one for "nobody" */
> +		errno = 0;
> +		pw = getpwnam("nobody");
> +		if (!pw) {
> +			printerr(0, "WARNING: unable to determine gid for uid %u\n", uid);
> +			return errno ? errno : ENOENT;
> +		}
> +	}
> +
> +	/*
> +	 * Switch the GIDs. Note that we leave the saved-set-gid alone in an
> +	 * attempt to prevent attacks via ptrace()
> +	 */
> +	if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> +		printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
> +		return errno;
> +	}
> +
> +	/*
> +	 * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
> +	 * other processes running with this uid.
> +	 */
> +	if (setresuid(uid, uid, -1) != 0) {
> +		printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
> +				uid);
> +		return errno;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
>   * this code uses the userland rpcsec gss library to create a krb5
>   * context on behalf of the kernel
>   */
> @@ -1036,6 +1078,14 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>  		 service ? service : "<null>");
>  	if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
>  				service == NULL)) {
> +
> +		err = change_identity(uid);
> +		if (err) {
> +			printerr(0, "WARNING: failed to change identity: %s",
> +				 strerror(err));
> +			goto out_return_error;
> +		}
> +
>  		/* Tell krb5 gss which credentials cache to use */
>  		/* Try first to acquire credentials directly via GSSAPI */
>  		err = gssd_acquire_user_cred(uid, &gss_cred);
> 

Re: [PATCH v3 0/2] gssd: allow gssd to work with KEYRING: credcaches

[Jeff Layton]

On Wed,  9 Oct 2013 16:21:54 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> Changes since original set:
> v3:
> - have parent check to see if child was signalled and log a warning if so
> - drop supplimentary groups and change gid before acquiring creds. Keep
>   suid and sgid as well to hamper ptrace.
> 
> v2:
> - fix bisectability. The original set added includes in the wrong
>   place in patch #1 and then fixed it in patch #2. The final result
>   of this set is the same but should bisect cleanly.
> 
> This patchset fixes up gssd to work with KEYRING: style credcaches. At
> the same time, it also fixes gssd not to need to trawl through likely
> credcache locations by allowing GSSAPI to find them in the intended
> fashion.
> 
> The basic idea is to have gssd fork() after reading off the pipe, but
> before handling the upcall and to do a more thorough job of changing
> credentials.
> 
> Jeff Layton (2):
>   gssd: have process_krb5_upcall fork before handling upcall
>   gssd: do a more thorough change of identity after forking
> 
>  utils/gssd/gssd_proc.c | 106 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 89 insertions(+), 17 deletions(-)
> 

I had asked Steve to hold off on applying these until now. While they
work as expected for the most part, they break a the case where gssd is
set up to use gssproxy to get credentials. Older versions of gssproxy
relied on the caller having and euid of 0. Simo now has a set of fixes
queued up for gssproxy to address the problem, so the way should now be
clear to merge this set into nfs-utils.

One thing we will probably want to do for Fedora at least is to add a
"Conflicts:" with older versions of gssproxy. That should ensure that
people relying on gssd + gssproxy won't get any surprises when updating.

Steve, sound reasonable?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 0/2] gssd: allow gssd to work with KEYRING: credcaches

[Steve Dickson]

On 15/10/13 09:34, Jeff Layton wrote:
> On Wed,  9 Oct 2013 16:21:54 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
> 
>> Changes since original set:
>> v3:
>> - have parent check to see if child was signalled and log a warning if so
>> - drop supplimentary groups and change gid before acquiring creds. Keep
>>   suid and sgid as well to hamper ptrace.
>>
>> v2:
>> - fix bisectability. The original set added includes in the wrong
>>   place in patch #1 and then fixed it in patch #2. The final result
>>   of this set is the same but should bisect cleanly.
>>
>> This patchset fixes up gssd to work with KEYRING: style credcaches. At
>> the same time, it also fixes gssd not to need to trawl through likely
>> credcache locations by allowing GSSAPI to find them in the intended
>> fashion.
>>
>> The basic idea is to have gssd fork() after reading off the pipe, but
>> before handling the upcall and to do a more thorough job of changing
>> credentials.
>>
>> Jeff Layton (2):
>>   gssd: have process_krb5_upcall fork before handling upcall
>>   gssd: do a more thorough change of identity after forking
>>
>>  utils/gssd/gssd_proc.c | 106 +++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 89 insertions(+), 17 deletions(-)
>>
> 
> I had asked Steve to hold off on applying these until now. While they
> work as expected for the most part, they break a the case where gssd is
> set up to use gssproxy to get credentials. Older versions of gssproxy
> relied on the caller having and euid of 0. Simo now has a set of fixes
> queued up for gssproxy to address the problem, so the way should now be
> clear to merge this set into nfs-utils.
> 
> One thing we will probably want to do for Fedora at least is to add a
> "Conflicts:" with older versions of gssproxy. That should ensure that
> people relying on gssd + gssproxy won't get any surprises when updating.
> 
> Steve, sound reasonable?
I'll queue them up...

steved.

>