Patchwork cifs: send real uid of initiating process to upcall instead of mount uid

login
register
mail settings
Submitter Jeff Layton
Date Aug. 3, 2009, 6:34 p.m.
Message ID <1249324456-8194-1-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/30645/
State New
Headers show

Comments

Jeff Layton - Aug. 3, 2009, 6:34 p.m.
The ownership of files on the mount has no direct relationship to the
credentials used to do the mount. Instead of sending the uid
corresponding to the owner of files on the mount, instead send the real
uid of the process that initiated the upcall.

Usually this will be the real uid of the process running /bin/mount.
Eventually however, we want to be able to establish new sessions for
users that walk into a cifs mount. For that we need the real uid of
those users.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifs_spnego.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Steve French - Aug. 3, 2009, 6:41 p.m.
Wouldn't it give more flexibility to the upcall program if we sent both or
changed the name (and be less confusing) e.g. to "mount_uid" or something
else distinct

On Mon, Aug 3, 2009 at 1:34 PM, Jeff Layton <jlayton@redhat.com> wrote:

> The ownership of files on the mount has no direct relationship to the
> credentials used to do the mount. Instead of sending the uid
> corresponding to the owner of files on the mount, instead send the real
> uid of the process that initiated the upcall.
>
> Usually this will be the real uid of the process running /bin/mount.
> Eventually however, we want to be able to establish new sessions for
> users that walk into a cifs mount. For that we need the real uid of
> those users.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifs_spnego.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
> index 8ec7736..1d0d8fc 100644
> --- a/fs/cifs/cifs_spnego.c
> +++ b/fs/cifs/cifs_spnego.c
> @@ -140,7 +140,7 @@ cifs_get_spnego_key(struct cifsSesInfo *sesInfo)
>                goto out;
>
>        dp = description + strlen(description);
> -       sprintf(dp, ";uid=0x%x", sesInfo->linux_uid);
> +       sprintf(dp, ";uid=0x%x", current_uid());
>
>        dp = description + strlen(description);
>        sprintf(dp, ";user=%s", sesInfo->userName);
> --
> 1.6.0.6
>
>
Jeff Layton - Aug. 3, 2009, 6:52 p.m.
On Mon, 3 Aug 2009 13:41:03 -0500
Steve French <smfrench@gmail.com> wrote:

> Wouldn't it give more flexibility to the upcall program if we sent both or
> changed the name (and be less confusing) e.g. to "mount_uid" or something
> else distinct
> 

We already send a bunch of fields that we don't actually use...

...and TBH, I'm a little leery of the existing situation. Might someone
be able to trick the kernel into using a credcache to which the user
doesn't have access by setting the right uid= option on a mount?

One of the main problems we have with "uid=" is that it means too much.
I'd like to remove it from having any meaning at all to the krb5 auth
subsys. Seems saner and more secure to me.
Steve French - Aug. 3, 2009, 7:40 p.m.
On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote:
> be able to trick the kernel into using a credcache to which the user
doesn't
> have access by setting the right uid= option on a mount?
...
>  Seems saner and more secure to me.

Are you saying "saner" to rename "uid=" for the purposes of the upcall...
not
just the change to pass the "uid of the process that initiated the upcall"
(not
the "owner of the files" uid) (If so, I agree)
Jeff Layton - Aug. 3, 2009, 8:20 p.m.
On Mon, 3 Aug 2009 14:40:19 -0500
Steve French <smfrench@gmail.com> wrote:

> On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > be able to trick the kernel into using a credcache to which the user
> doesn't
> > have access by setting the right uid= option on a mount?
> ...
> >  Seems saner and more secure to me.
> 
> Are you saying "saner" to rename "uid=" for the purposes of the upcall...
> not
> just the change to pass the "uid of the process that initiated the upcall"
> (not
> the "owner of the files" uid) (If so, I agree)
> 
> 

I'm not sure I follow what you're saying...

I'm saying that we should pass the real uid of the process that
initiated the upcall to the upcall. There's no point in passing the uid
of the process that owns the files on the mount to the upcall.

I don't see the point of renaming the uid= "key" here. Can you
elaborate as to why you think it would be good to do so?
Steve French - Aug. 3, 2009, 8:45 p.m.
On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 3 Aug 2009 14:40:19 -0500
> Steve French <smfrench@gmail.com> wrote:
>
> > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > > be able to trick the kernel into using a credcache to which the user
> > doesn't
> > > have access by setting the right uid= option on a mount?
> > ...
> > >  Seems saner and more secure to me.
> >
> > Are you saying "saner" to rename "uid=" for the purposes of the upcall...
> > not
> > just the change to pass the "uid of the process that initiated the
> upcall"
> > (not
> > the "owner of the files" uid) (If so, I agree)
> >
> >
>
> I'm not sure I follow what you're saying...
>
> I'm saying that we should pass the real uid of the process that
> initiated the upcall to the upcall. There's no point in passing the uid
> of the process that owns the files on the mount to the upcall.
>

Yes - I was agreeing on this :)


>
> I don't see the point of renaming the uid= "key" here. Can you
> elaborate as to why you think it would be good to do so?
>

I thought that you had also noted that using "uid" to mean more than
one thing on mount was confusing, and thus could be confusing
to also keep the same name in cifs.upcall.  If cifs.upcall does
not care about the "default file owner" but does care about
the process that initiated the upcall - seems like it would make
sense to call it something distinct to eliminate any
possible confusion.




Thanks,

Steve
Jeff Layton - Aug. 3, 2009, 10:37 p.m.
On Mon, 3 Aug 2009 15:45:51 -0500
Steve French <smfrench@gmail.com> wrote:

> On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Mon, 3 Aug 2009 14:40:19 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >
> > > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > > > be able to trick the kernel into using a credcache to which the user
> > > doesn't
> > > > have access by setting the right uid= option on a mount?
> > > ...
> > > >  Seems saner and more secure to me.
> > >
> > > Are you saying "saner" to rename "uid=" for the purposes of the upcall...
> > > not
> > > just the change to pass the "uid of the process that initiated the
> > upcall"
> > > (not
> > > the "owner of the files" uid) (If so, I agree)
> > >
> > >
> >
> > I'm not sure I follow what you're saying...
> >
> > I'm saying that we should pass the real uid of the process that
> > initiated the upcall to the upcall. There's no point in passing the uid
> > of the process that owns the files on the mount to the upcall.
> >
> 
> Yes - I was agreeing on this :)
> 

Ok good. Though I mis-wrote that above. That should have read "There's
no point in passing the uid that owns the files on the mount..."

...but you get the idea.

> 
> >
> > I don't see the point of renaming the uid= "key" here. Can you
> > elaborate as to why you think it would be good to do so?
> >
> 
> I thought that you had also noted that using "uid" to mean more than
> one thing on mount was confusing, and thus could be confusing
> to also keep the same name in cifs.upcall.  If cifs.upcall does
> not care about the "default file owner" but does care about
> the process that initiated the upcall - seems like it would make
> sense to call it something distinct to eliminate any
> possible confusion.
> 

Ahh ok. The problem there is that would break older cifs.upcall
programs.

cifs.upcall currently looks for an option called "uid=". I'm just
proposing that we change how we populate that option. If we call it
something new, then older cifs.upcall programs won't know about the
change and wouldn't setuid to a user before trying to access the
credcache.

The catch here is that this change might be problematic for people that
expect that setting uid= to a value will point cifs.upcall at that
uid's credcache. If we're concerned about that, it might be better to
make this change more gradually.

Thoughts?
Jeff Layton - Aug. 7, 2009, 2:36 p.m.
On Mon, 3 Aug 2009 18:37:20 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 3 Aug 2009 15:45:51 -0500
> Steve French <smfrench@gmail.com> wrote:
> 
> > On Mon, Aug 3, 2009 at 3:20 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > > On Mon, 3 Aug 2009 14:40:19 -0500
> > > Steve French <smfrench@gmail.com> wrote:
> > >
> > > > On Mon, Aug 3, 2009 at 1:52 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > > > > be able to trick the kernel into using a credcache to which the user
> > > > doesn't
> > > > > have access by setting the right uid= option on a mount?
> > > > ...
> > > > >  Seems saner and more secure to me.
> > > >
> > > > Are you saying "saner" to rename "uid=" for the purposes of the upcall...
> > > > not
> > > > just the change to pass the "uid of the process that initiated the
> > > upcall"
> > > > (not
> > > > the "owner of the files" uid) (If so, I agree)
> > > >
> > > >
> > >
> > > I'm not sure I follow what you're saying...
> > >
> > > I'm saying that we should pass the real uid of the process that
> > > initiated the upcall to the upcall. There's no point in passing the uid
> > > of the process that owns the files on the mount to the upcall.
> > >
> > 
> > Yes - I was agreeing on this :)
> > 
> 
> Ok good. Though I mis-wrote that above. That should have read "There's
> no point in passing the uid that owns the files on the mount..."
> 
> ...but you get the idea.
> 
> > 
> > >
> > > I don't see the point of renaming the uid= "key" here. Can you
> > > elaborate as to why you think it would be good to do so?
> > >
> > 
> > I thought that you had also noted that using "uid" to mean more than
> > one thing on mount was confusing, and thus could be confusing
> > to also keep the same name in cifs.upcall.  If cifs.upcall does
> > not care about the "default file owner" but does care about
> > the process that initiated the upcall - seems like it would make
> > sense to call it something distinct to eliminate any
> > possible confusion.
> > 
> 
> Ahh ok. The problem there is that would break older cifs.upcall
> programs.
> 
> cifs.upcall currently looks for an option called "uid=". I'm just
> proposing that we change how we populate that option. If we call it
> something new, then older cifs.upcall programs won't know about the
> change and wouldn't setuid to a user before trying to access the
> credcache.
> 
> The catch here is that this change might be problematic for people that
> expect that setting uid= to a value will point cifs.upcall at that
> uid's credcache. If we're concerned about that, it might be better to
> make this change more gradually.
> 
> Thoughts?
> 

Actually...I think I'm going to withdraw this patch. Sending the real
uid still isn't quite what we need here. The problem is that there's no
guarantee that the "correct" uid will be the one that initiates the
upcall during a reconnect.

I need to rethink this (and will plan to respin and resend in the
future).

Patch

diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
index 8ec7736..1d0d8fc 100644
--- a/fs/cifs/cifs_spnego.c
+++ b/fs/cifs/cifs_spnego.c
@@ -140,7 +140,7 @@  cifs_get_spnego_key(struct cifsSesInfo *sesInfo)
 		goto out;
 
 	dp = description + strlen(description);
-	sprintf(dp, ";uid=0x%x", sesInfo->linux_uid);
+	sprintf(dp, ";uid=0x%x", current_uid());
 
 	dp = description + strlen(description);
 	sprintf(dp, ";user=%s", sesInfo->userName);