On Tue, 4 Jan 2011 13:51:41 -0600 Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > On Tue, Jan 4, 2011 at 1:31 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Tue, 4 Jan 2011 13:20:38 -0600 > > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > > > >> On Tue, Jan 4, 2011 at 1:11 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > >> > On Mon, 6 Dec 2010 14:56:46 -0600 > >> > shirishpargaonkar@xxxxxxxxx wrote: > >> > > >> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> > >> >> > >> >> > >> >> If a DACL has entries for ACEs for SID Everyone and Authenticated Users, > >> >> factor in mask in respective entries during calculation of permissions > >> >> for all three, user, group, and other. > >> >> > >> >> http://technet.microsoft.com/en-us/library/bb463216.aspx > >> >> > >> >> > >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> > >> >> --- > >> >> fs/cifs/cifsacl.c | 13 +++++++++++-- > >> >> 1 files changed, 11 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > >> >> index c6ebea0..a520091 100644 > >> >> --- a/fs/cifs/cifsacl.c > >> >> +++ b/fs/cifs/cifsacl.c > >> >> @@ -43,9 +43,12 @@ static struct cifs_wksid wksidarr[NUM_WK_SIDS] = { > >> >> ; > >> >> > >> >> > >> >> -/* security id for everyone */ > >> >> +/* security id for everyone/world system group */ > >> >> static const struct cifs_sid sid_everyone = { > >> >> 1, 1, {0, 0, 0, 0, 0, 1}, {0} }; > >> >> +/* security id for Authenticated Users system group */ > >> >> +static const struct cifs_sid sid_authusers = { > >> >> + 1, 1, {0, 0, 0, 0, 0, 5}, {11} }; > >> >> /* group users */ > >> >> static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} }; > >> >> > >> >> @@ -367,7 +370,7 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl, > >> >> if (num_aces > 0) { > >> >> umode_t user_mask = S_IRWXU; > >> >> umode_t group_mask = S_IRWXG; > >> >> - umode_t other_mask = S_IRWXO; > >> >> + umode_t other_mask = S_IRWXU | S_IRWXG | S_IRWXO; > >> >> > >> >> ppace = kmalloc(num_aces * sizeof(struct cifs_ace *), > >> >> GFP_KERNEL); > >> >> @@ -392,6 +395,12 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl, > >> >> ppace[i]->type, > >> >> &fattr->cf_mode, > >> >> &other_mask); > >> >> + if (compare_sids(&(ppace[i]->sid), &sid_authusers)) > >> >> + access_flags_to_mode(ppace[i]->access_req, > >> >> + ppace[i]->type, > >> >> + &fattr->cf_mode, > >> >> + &other_mask); > >> >> + > >> >> > >> >> /* memcpy((void *)(&(cifscred->aces[i])), > >> >> (void *)ppace[i], > >> > > >> > This looks like a harmless change... > >> > > >> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx> > >> > > >> > >> Jeff, Thanks. > >> > >> > I have real doubts however about the owner/group parts of the cifsacl > >> > scheme. It sets the user and group mode bits without setting the user > >> > and group owners to meaningful values. So, those permissions have no > >> > basis in reality. > >> > >> With the sid to uid and gid mapping code using winbind in the works, > >> would that address these concerns? > >> > > > > Yes, that would help some. > > > > That would give you the ability to present permissions that have some > > basis in reality. You'll still have to figure out what to do when you > > get a SID that can't be mapped for some reason. > > If we are using winbind, I do not see how this can happen. > A SID perhaps could not be looked up but winbind, when asked, > can/will always map a Owner SID to an uid and Group SID to a gid. > Ahh ok. Good to know. Still, we need a meaningful owner to set it to when the upcall fails. For instance, if the upcall isn't set up at all... > > > > The real solution is to get rid of client-side permissions enforcement > > altogether. The server is the only place that they can be reliably > > enforced. > > I can see when changing mode or group or owner for an object, > client should just be a conduit for that action (i.e. dispatch a > fabricated DACL) and server is the one that should decide to enforce > the change. Agreed. > But cifs client nonetheless needs to calculate and set mode and fill in > uid and gid for an inode since none of these values (mode, uid, and gid) > exist at the equivalent object on the server. > Well, you need to have something to return in a stat() call. Those values do not necessarily have to be used to enforce permissions. Note the way that CIFS_MOUNT_NO_PERM is used. > I think we had this conversation before so I will dig it up, but I am > trying to figure out how and where client enforces permissions > (which it should not) other than when mode or user or group ids are > being changed. > See cifs_permission which eventually calls generic_permission, which uses the i_mode/i_uid/i_gid to determine whether access is allowed. The problem though is that cifs mounts use a single set of credentials by default. Unless we make multiuser mounts the default, we're sort of stuck with this broken model for legacy reasons. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
