| Message ID | 20170223215835.18490-1-jlayton@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-02-23 at 16:58 -0500, Jeff Layton wrote: > Setuid programs triggering upcalls could trick the program here. Also, > the d_automount method is done with credentials overridden so if you > can end up with mismatched creds and env vars due to that as well. > > It's a hack, but the only recourse I can see is to avoid doing this > when the uid is 0. That means we can rely on this for getting to root's > credcache, but hopefully that's not a huge issue. > > Reported-by: Chad William Seys <cwseys@physics.wisc.edu> > Signed-off-by: Jeff Layton <jlayton@samba.org> > --- > cifs.upcall.8.in | 5 ++++- > cifs.upcall.c | 10 +++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/cifs.upcall.8.in b/cifs.upcall.8.in > index e1f3956e176a..81481a482fb4 100644 > --- a/cifs.upcall.8.in > +++ b/cifs.upcall.8.in > @@ -44,7 +44,10 @@ Normally, cifs.upcall will probe the environment variable space of the process > that initiated the upcall in order to fetch the value of $KRB5CCNAME. This can > assist the program with finding credential caches in non-default locations. If > this option is set, then the program won't do this and will rely on finding > -credcaches in the default locations specified in krb5.conf. > +credcaches in the default locations specified in krb5.conf. Note that this is > +never performed when the uid is 0. The default credcache location is always > +used when the uid is 0, regardless of the environment variable setting in the > +process. > .RE > .PP > \--krb5conf=/path/to/krb5.conf|-k /path/to/krb5.conf > diff --git a/cifs.upcall.c b/cifs.upcall.c > index f766a8b5799e..8f6bb761a27e 100644 > --- a/cifs.upcall.c > +++ b/cifs.upcall.c > @@ -1018,11 +1018,19 @@ int main(const int argc, char *const argv[]) > } > > /* > + * We can't reasonably do this for root. Mounting a DFS share, for > + * instance we can end up with creds being overridden, but the env > + * variable left intact. > + */ > + if (uid == 0) > + env_probe = false; > + > + /* > * Must do this before setuid, as we need elevated capabilities to > * look at the environ file. > */ > env_cachename = > - get_cachename_from_process_env(env_probe ? arg.pid : 0); > + get_cachename_from_process_env(env_probe ? arg.pid : 0); > > rc = setuid(uid); > if (rc == -1) { Actually...what may be better here is to just unset KRB5CCNAME after we try to find an existing credcache, but before we try to set one up from the keytab. I see no need to use the environment var at all when we expect to instantiate a new ccache. Now that I think about it though, we might still want to restrict root like this too. In particular, we could end up performing session setup for root's session from the wrong credcache. I don't think that's quite what we want there. So I think we'll want to keep this patch, and have it unset the environment var too. There's probably some way to do all of this more cleanly from a namespace standpoint, but I'm not sure how...
diff --git a/cifs.upcall.8.in b/cifs.upcall.8.in index e1f3956e176a..81481a482fb4 100644 --- a/cifs.upcall.8.in +++ b/cifs.upcall.8.in @@ -44,7 +44,10 @@ Normally, cifs.upcall will probe the environment variable space of the process that initiated the upcall in order to fetch the value of $KRB5CCNAME. This can assist the program with finding credential caches in non-default locations. If this option is set, then the program won't do this and will rely on finding -credcaches in the default locations specified in krb5.conf. +credcaches in the default locations specified in krb5.conf. Note that this is +never performed when the uid is 0. The default credcache location is always +used when the uid is 0, regardless of the environment variable setting in the +process. .RE .PP \--krb5conf=/path/to/krb5.conf|-k /path/to/krb5.conf diff --git a/cifs.upcall.c b/cifs.upcall.c index f766a8b5799e..8f6bb761a27e 100644 --- a/cifs.upcall.c +++ b/cifs.upcall.c @@ -1018,11 +1018,19 @@ int main(const int argc, char *const argv[]) } /* + * We can't reasonably do this for root. Mounting a DFS share, for + * instance we can end up with creds being overridden, but the env + * variable left intact. + */ + if (uid == 0) + env_probe = false; + + /* * Must do this before setuid, as we need elevated capabilities to * look at the environ file. */ env_cachename = - get_cachename_from_process_env(env_probe ? arg.pid : 0); + get_cachename_from_process_env(env_probe ? arg.pid : 0); rc = setuid(uid); if (rc == -1) {
Setuid programs triggering upcalls could trick the program here. Also, the d_automount method is done with credentials overridden so if you can end up with mismatched creds and env vars due to that as well. It's a hack, but the only recourse I can see is to avoid doing this when the uid is 0. That means we can rely on this for getting to root's credcache, but hopefully that's not a huge issue. Reported-by: Chad William Seys <cwseys@physics.wisc.edu> Signed-off-by: Jeff Layton <jlayton@samba.org> --- cifs.upcall.8.in | 5 ++++- cifs.upcall.c | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-)