Changes since original set:
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 set is comprised of a couple of patches that fix gssd so that it
works with KEYRING: style credcaches. It turns out that gssd already
tries to query GSSAPI to find the best credcache to use and only falls
back to trawling through likely locations for credcaches if that fails.
The problem is that the initial call into GSSAPI for this almost always
fails, so it ends up falling back to trawling in the common case. This
patch corrects this by making a number of changes:
1) credentials are switched sooner during the upcall and don't switch back
2) credentials are switched using setuid() instead of setfsuid(). The
GSSAPI libs depend on the *real* uid being correct.
3) the daemon now forks before doing any credential switching to ensure
that unprivileged users can't do anything nefarious to it while it's
running under a different uid.
With this set of changes, and a bleeding-edge version of the krb5 and
keyutils libs, I can now successfully use KEYRING: style credcaches.
Jeff Layton (2):
gssd: have process_krb5_upcall fork before handling upcall
gssd: switch real uid instead of just fsuid when looking for user
creds
utils/gssd/gssd_proc.c | 47 ++++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
--
1.8.3.1
In order to handle KEYRING: caches, we need to be able to switch the
real UID of the process to the designated one, but that opens the door
to allowing gssd to be killed or reniced during the window where we've
switched credentials.
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.
Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd_proc.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e58c341..fd258f7 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,23 @@ 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);
+ return;
+ }
printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
@@ -1121,7 +1140,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
----- Original Message -----
> 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 and that
> 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" and then set
> $KRB5CCNAME if and when they are found. 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, the GSSAPI code that handles finding credcaches does it based
> on the return from getuid(), so just switching the fsuid or even euid is
> insufficient. You must switch the real uid.
In what case does it do this ?
If it is unconditional this is a bug in GSSAPI and we should fix it there.
You shouldn't really change the real uid of gssd because then the user can
send a kill to the gssd process, only euid should be changed IMO.
Simo.
> This code moves the credential switching into process_krb5_upcall and
> makes it use setuid() instead of setfsuid(). That's of course
> irreversible so we can't switch back to root after doing so. No matter
> though since it's probably safer to do all of this as an unprivileged
> user anyway.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> utils/gssd/gssd_proc.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index fd258f7..4856d08 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:
> @@ -1033,6 +1017,16 @@ 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)) {
> +
> + /* Create the context as the user (not as root) */
> + if (setuid(uid) != 0) {
> + printerr(0, "WARNING: Failed to setuid for "
> + "user with uid %d\n", uid);
> + goto out_return_error;
> + }
> +
> + printerr(2, "creating context using real uid %d\n", uid);
> +
> /* 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Simo Sorce * Red Hat, Inc. * New York
On Fri, 4 Oct 2013 15:46:42 -0400 (EDT)
Simo Sorce <[email protected]> wrote:
>
>
> ----- Original Message -----
> > 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 and that
> > 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" and then set
> > $KRB5CCNAME if and when they are found. 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, the GSSAPI code that handles finding credcaches does it based
> > on the return from getuid(), so just switching the fsuid or even euid is
> > insufficient. You must switch the real uid.
>
> In what case does it do this ?
> If it is unconditional this is a bug in GSSAPI and we should fix it there.
>
As far as I can tell (primarily from stracing and experimenting) it
always does this. Additionally it looks like KEYRING: support might
also depend on this.
> You shouldn't really change the real uid of gssd because then the user can
> send a kill to the gssd process, only euid should be changed IMO.
>
Perhaps you missed the first patch in the series that makes gssd fork()
before doing this? That prevents the main daemon from being affected.
Only the forked thread handling the upcall should be vulnerable. That's
unfortunate, but seems like a reasonable way to deal with the needs of
GSSAPI.
> > This code moves the credential switching into process_krb5_upcall and
> > makes it use setuid() instead of setfsuid(). That's of course
> > irreversible so we can't switch back to root after doing so. No matter
> > though since it's probably safer to do all of this as an unprivileged
> > user anyway.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > utils/gssd/gssd_proc.c | 26 ++++++++++----------------
> > 1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index fd258f7..4856d08 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:
> > @@ -1033,6 +1017,16 @@ 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)) {
> > +
> > + /* Create the context as the user (not as root) */
> > + if (setuid(uid) != 0) {
> > + printerr(0, "WARNING: Failed to setuid for "
> > + "user with uid %d\n", uid);
> > + goto out_return_error;
> > + }
> > +
> > + printerr(2, "creating context using real uid %d\n", uid);
> > +
> > /* 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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
Jeff Layton <[email protected]>
On Sat, 5 Oct 2013 18:04:03 -0400
Jeff Layton <[email protected]> wrote:
> On Fri, 4 Oct 2013 15:46:42 -0400 (EDT)
> Simo Sorce <[email protected]> wrote:
>
> >
> >
> > ----- Original Message -----
> > > 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 and that
> > > 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" and then set
> > > $KRB5CCNAME if and when they are found. 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, the GSSAPI code that handles finding credcaches does it based
> > > on the return from getuid(), so just switching the fsuid or even euid is
> > > insufficient. You must switch the real uid.
> >
> > In what case does it do this ?
> > If it is unconditional this is a bug in GSSAPI and we should fix it there.
> >
>
> As far as I can tell (primarily from stracing and experimenting) it
> always does this. Additionally it looks like KEYRING: support might
> also depend on this.
>
I took a closer look at this this morning and I think I see the
problem. The issue is in libkrb5 and the default definitions of the
credcache locations. For instance:
default_ccache_name = KEYRING:persistent:%{uid}
%{uid} expands to the real uid, not the effective one. If you change
that to:
default_ccache_name = KEYRING:persistent:%{euid}
...then it all works just by swapping the euid.
So, not a bug in libkrb5 or libgssapi per-se, but I think we're sort of
stuck with switching the real uid due to this.
The fact that the first patch in this series makes the daemon fork
before doing this should help mitigate any damage that an unprivileged
user can do.
--
Jeff Layton <[email protected]>
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 and that
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" and then set
$KRB5CCNAME if and when they are found. 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, the GSSAPI code that handles finding credcaches does it based
on the return from getuid(), so just switching the fsuid or even euid is
insufficient. You must switch the real uid.
This code moves the credential switching into process_krb5_upcall and
makes it use setuid() instead of setfsuid(). That's of course
irreversible so we can't switch back to root after doing so. No matter
though since it's probably safer to do all of this as an unprivileged
user anyway.
Signed-off-by: Jeff Layton <[email protected]>
---
utils/gssd/gssd_proc.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index fd258f7..4856d08 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:
@@ -1033,6 +1017,16 @@ 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)) {
+
+ /* Create the context as the user (not as root) */
+ if (setuid(uid) != 0) {
+ printerr(0, "WARNING: Failed to setuid for "
+ "user with uid %d\n", uid);
+ goto out_return_error;
+ }
+
+ printerr(2, "creating context using real uid %d\n", uid);
+
/* 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