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

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,

Steve