Re: [PATCH 1/3] cifs: make overriding of ownership conditional on new mount options

On Sat, 6 Jun 2009 16:12:56 -0500
Steve French <[email protected]> wrote:

> On Thu, Jun 4, 2009 at 12:15 PM, Jeff Layton<[email protected]> wrote:
> > On Sun, 24 May 2009 18:45:15 -0400
> > Jeff Layton <[email protected]> wrote:
> >
> >> We have a bit of a problem with the uid= option. The basic issue is that
> >> it means too many things and has too many side-effects.
> >>
> >> It's possible to allow an unprivileged user to mount a filesystem if the
> >> user owns the mountpoint, /bin/mount is setuid root, and the mount is
> >> set up in /etc/fstab with the "user" option.
> >>
> >> When doing this though, /bin/mount automatically adds the "uid=" and
> >> "gid=" options to the share. This is fortunate since the correct uid=
> >> option is needed in order to tell the upcall what user's credcache to
> >> use when generating the SPNEGO blob.
> >>
> >> On a mount without unix extensions this is fine -- you generally will
> >> want the files to be owned by the "owner" of the mount. The problem
> >> comes in on a mount with unix extensions. With those enabled, the
> >> uid/gid options cause the ownership of files to be overriden even though
> >> the server is sending along the ownership info.
> >>
> >> This means that it's not possible to have a mount by an unprivileged
> >> user that shows the server's file ownership info. The result is also
> >> inode permissions that have no reflection at all on the server. You
> >> simply cannot separate ownership from the mode in this fashion.
> >>
> >> This behavior also makes MultiuserMount option less usable. Once you
> >> pass in the uid= option for a mount, then you can't use unix ownership
> >> info and allow someone to share the mount.
> >>
> >> While I'm not thrilled with it, the only solution I can see is to stop
> >> making uid=/gid= force the overriding of ownership on mounts, and to add
> >> new mount options that turn this behavior on.
> >>
> >> Signed-off-by: Jeff Layton <[email protected]>
> >> ---
> >>  fs/cifs/connect.c |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 4aa81a5..4f5a03c 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -1092,17 +1092,17 @@ cifs_parse_mount_options(char *options, const char *devname,
> >>                               return 1;
> >>                       }
> >>               } else if (strnicmp(data, "uid", 3) == 0) {
> >> -                     if (value && *value) {
> >> +                     if (value && *value)
> >>                               vol->linux_uid =
> >>                                       simple_strtoul(value, &value, 0);
> >> +             } else if (strnicmp(data, "forceuid", 8) == 0) {
> >>                               vol->override_uid = 1;
> >> -                     }
> >>               } else if (strnicmp(data, "gid", 3) == 0) {
> >> -                     if (value && *value) {
> >> +                     if (value && *value)
> >>                               vol->linux_gid =
> >>                                       simple_strtoul(value, &value, 0);
> >> +             } else if (strnicmp(data, "forcegid", 8) == 0) {
> >
> > Steve,
> >   Any word on this patch? I have a customer who is being impacted by
> > this bug. I need to bring it to some sort of resolution soon.
> 
> Merged.  Also added minor patch to update the fs/cifs/CHANGES and
> README to mention the change.  The wording I used is not great
> but when the man page is updated, perhaps we can make it less confusing
> (not sure how ... but I agree that the change would be useful).  Perhaps
> we should add a debug message to note when the user specified
> "forceuid" and Unix extensions were not negotiated? or perhaps that is
> unimportant ... and the more important case of "they specified a uid
> or gid, but the server's uids don't seem to match the client uids" and
> should have specified forceuid?)
> 
> 

Thanks,

I don't see any point in warning when unix extensions aren't
negotiated. The way I look at it is this -- when the owner can't be
determined then we use the default uid/gid for the share. The new
options just force this even when the uid/gid can be determined. When
they're specified on a share where we can't determine the ownership,
then they're just ignored.

-- 
Jeff Layton <[email protected]>