On Wed, Jan 5, 2011 at 6:19 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Tue, 4 Jan 2011 16:19:26 -0600 > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: > >> On Tue, Jan 4, 2011 at 4:10 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: >> > On Tue, 4 Jan 2011 14:27:45 -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... >> >> > >> >> >> >> I think at least two things can go against, either upcall is not setup at all >> >> so an upcall fails or we run out of idmap range as specified in smb.conf >> >> file for either uid or gid or both and so upcall fails. >> >> >> > >> > Yep. The reason for the failure doesn't matter much. What matters is >> > what you do if it does. You'll need to set the uid or gid to something. >> > >> > What will you set it to? >> > >> >> I think it should default to 0 (superuser). >> > > That sounds a little dangerous. What if the mode bits indicate setuid? > > rpc.idmapd generally maps users to "nobody" if no mapping can be found. > Something like that might be a safer default. It should probably be a > tunable setting too. > > -- > Jeff Layton <jlayton@xxxxxxxxxx> > It really should not matter what the owner and group name/id of an object within such mapped share look like since permission checking is going to happen as per server. -- If a process is checking permissions, an attempt is made to map the uid of the process to a SID. If it does not map, then -EACCESS error is returned. If it maps to a SID, verify whether that SID exists as one of the ACEs in the fetched DACL, if it does not, return -EACCESS error and if it does, map the mask value in that ACE to the permissions requested/being_ascertained and return the result like generic_permissions() would. -- If a user mounts a share using uid and gid mount options, should all the objects within such share appear with uname and gname respectively, winbind mapping should not be even attempted when listing/stat'ing a file and winbind should be employed only during permission_checking/chmod/chroup/chmod? -- 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
