| Message ID | 1243205117-3351-2-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 24 May 2009 18:45:15 -0400 Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com> > --- > 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) { > vol->override_gid = 1; > - } > } else if (strnicmp(data, "file_mode", 4) == 0) { > if (value && *value) { > vol->file_mode = > -- > 1.6.0.6 > 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. Thanks,
On Thu, Jun 4, 2009 at 12:15 PM, Jeff Layton<jlayton@redhat.com> wrote: > On Sun, 24 May 2009 18:45:15 -0400 > Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com> >> --- >> Â 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?)
On Sat, 6 Jun 2009 16:12:56 -0500 Steve French <smfrench@gmail.com> wrote: > On Thu, Jun 4, 2009 at 12:15 PM, Jeff Layton<jlayton@redhat.com> wrote: > > On Sun, 24 May 2009 18:45:15 -0400 > > Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com> > >> --- > >> Â 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.
On Sat, Jun 6, 2009 at 6:25 PM, Jeff Layton<jlayton@redhat.com> wrote: > On Sat, 6 Jun 2009 16:12:56 -0500 > Steve French <smfrench@gmail.com> wrote: >>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. OK ... that logic makes sense.
On Sat, 6 Jun 2009 19:15:40 -0500 Steve French <smfrench@gmail.com> wrote: > On Sat, Jun 6, 2009 at 6:25 PM, Jeff Layton<jlayton@redhat.com> wrote: > > On Sat, 6 Jun 2009 16:12:56 -0500 > > Steve French <smfrench@gmail.com> wrote: > >>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. > > OK ... that logic makes sense. > I'm working on a manpage update for this, and a thought strikes me. Would it be better to replace these new options (forceuid/forcegid) with a single option "forceperm" or something that not only forces the uid/gid but also forces the mode to the file_mode/dir_mode ?
On Sun, Jun 7, 2009 at 6:40 AM, Jeff Layton<jlayton@redhat.com> wrote: > On Sat, 6 Jun 2009 19:15:40 -0500 > Steve French <smfrench@gmail.com> wrote: > >> On Sat, Jun 6, 2009 at 6:25 PM, Jeff Layton<jlayton@redhat.com> wrote: >> > 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. >> >> OK ... that logic makes sense. >> > > I'm working on a manpage update for this, and a thought strikes me. > > Would it be better to replace these new options (forceuid/forcegid) > with a single option "forceperm" or something that not only forces the > uid/gid but also forces the mode to the file_mode/dir_mode ? I don't think so. I have heard of a few use cases where users want 'to override the owner (the "home directory mount" use case) returned by the server but keep the permissions on files (because in this use case the files are mostly owned by the same user but the server and client don't use the same uid for that user. Once the uid is corrected by force uid, the actual perms make sense)
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) { vol->override_gid = 1; - } } else if (strnicmp(data, "file_mode", 4) == 0) { if (value && *value) { vol->file_mode =
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 <jlayton@redhat.com> --- fs/cifs/connect.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)