Re: [RFC][cifs-utils PATCH] cifs.upcall: allow scraping of KRB5CCNAME out of initiating task's /proc/<pid>/environ file

[Simo Sorce <simo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Mon, 2017-02-13 at 07:52 -0500, Jeff Layton wrote:
> On Mon, 2017-02-13 at 05:02 -0500, Simo Sorce wrote:
> > On Sat, 2017-02-11 at 10:16 -0500, Jeff Layton wrote:
> > > On Sat, 2017-02-11 at 08:41 -0500, Jeff Layton wrote:
> > > > Chad reported that he was seeing a regression in cifs-utils-
> > > > 6.6.
> > > > Prior
> > > > to that, cifs.upcall was able to find credcaches in non-default
> > > > FILE:
> > > > locations, but with the rework of that code, that ability was
> > > > lost.
> > > > 
> > > > Unfortunately, the krb5 library design doesn't really take into
> > > > account
> > > > the fact that we might need to find a credcache in a process
> > > > that
> > > > isn't
> > > > descended from the session.
> > > > 
> > > > When the kernel does an upcall, it passes several bits of info
> > > > about the
> > > > task that initiated the upcall. One of those things is the PID
> > > > (the
> > > > tgid, in particular). We can use that info to reach into the
> > > > /proc/<pid>/environ file for the process, and grab whatever
> > > > value
> > > > of
> > > > KRB5CCNAME is there.  This patch adds this ability to
> > > > cifs.upcall.
> > > > 
> > > > I'm not 100% convinced that this is a good idea however, so for
> > > > now,
> > > > this is disabled unless the command line has a '-e' switch.
> > > > Anyone
> > > > wishing to play with this should edit their /etc/request-
> > > > key.conf
> > > > files
> > > > accordingly.
> > > > 
> > > 
> > > Meant to put a Reported-by: here for Chad. I'll do that before
> > > merging.
> > > 
> > > > > > > Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > > > 
> > > > ---
> > > >  cifs.upcall.8.in |  10 ++++
> > > >  cifs.upcall.c    | 148
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  2 files changed, 152 insertions(+), 6 deletions(-)
> > > > 
> > > 
> > > FWIW, this patch works as expected in cursory, light testing.
> > > This
> > > should be superior to the earlier method where we scanned in /tmp
> > > as
> > > well -- more efficient, and also able to find non-FILE:
> > > credcaches,
> > > as
> > > well as ones that lie in other places besides /tmp.
> > > 
> > > My main concern is that we have to do the environ file scraping
> > > while
> > > privileged, and that we end up trusting some info from a userland
> > > process that triggered the upcall in KRB5CCNAME. Does this open
> > > any
> > > potential attack vectors that I'm not considering?
> > 
> > Well you are operating as the user however I have not seen a setgid
> > operation, does this mean cifs.upcall is running with group id 0 ?
> > 
> > This may give access to files the original user should not be able
> > to
> > access or cause files to be created with improper permission.
> > 
> 
> Yeah, ick. The kernel doesn't send down the gid, so we can't just add
> that in directly...
> 
> We could do a getpwuid(uid), and then setgid(pw_gid). That might be
> problematic for applications that have done a setgid(), but those
> ought to be very rare.

For the CIFS/NFS use case I think this is good enough, if you did
setgid and the credential file can only be accessed with that gid...
well tough luck, I mean it must be *exceedingly* rare.
For tht matter we do not have either all the secondary gids, and the
ccache may be (only) readable by one of them too ...

> > Another thing people may need to pay attention to is SELinux
> > context, I
> > am not sure what selinux context cifs.upcall runs into the
> > difference
> > may matter in some cases, but in those I think proper SELinux
> > policy
> > should be devised, I see no need to change how cifs.upcall
> > operates.
> > 
> 
> Yeah, that's a different matter. It would be nice to confine
> cifs.upcall, but we'd need an SELinux policy for that. Honestly, I'd
> rather see us move to gssproxy before spending time on that.

Yes,
I think the above approach is good enough.

Simo.