On Thu, Jan 13, 2011 at 9:48 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Thu, 13 Jan 2011 09:26:59 -0600 > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > >> On Tue, Jan 4, 2011 at 2:09 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >> > 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 permission checking has to be done by the client. >> What cifs/smb command can a client send to the server to verify >> permissions? >> > > There is no such call. How would you communicate to the server who > the user actually *is* anyway? The only way to do this is to map user > accesses to the appropriate credentials (aka multiuser mounts). > > I think it would be a good idea to step back and state exactly what > you want to achieve with client-side permissions enforcement. The > server is already going to enforce permissions according to the > credentials used to access it. > > What exactly do you intend to protect with client-side enforcement? Jeff, with client side permission checking/enforcment, as I understand, we are checking for accesses (read, write, etc.). Is that correct or is there more to that? I think it easy when a user wants to either change owner or group or mode of an object, client just needs to prepare a DACL and let server decide the action. With permission checking, since we can't forward such a request to the server, a client has to act on that request. The uid and gid of an object should be based on the DACL of the file at the server, irrespective of the credentials of the session. That way each session has a uniform view of the share. If owner and group based in the DACL can't be mapped to a uid and gid on the client, it should default to something like nobody and nogroup. If the owner can be mapped, then user permissions are calculated as per ACE(s) in the DACL but if owner can't be mapped, they should default to no permissions Same applies to group permissions. Other permissions should be calculated as per ACE(s) in the DACL. Every uid/fsuid on the client is treated as an authenticated user (SID) as far as that file object is considered. So the permission checking for accesses for that uid/fsuid is as per SID of the authenticated/credential'ed user of that SMB session. If authenticated user's SID happens to be owner of the file object, user permissions are checked, if it happens to be group (owner) of the file object, group permissions are checked and if not, other permissions are checked. With multisession mounts, as per I understand, we would be checking with respective session's authenticated user's SID instead of a single SMB session's credential'ed SID (all the time). > > -- > 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
