Stephen Smalley <sds@xxxxxxxxxxxxx> writes: > On Thu, 2017-09-28 at 17:34 -0500, Eric W. Biederman wrote: >> It looks like once upon a time a long time ago selinux copied code >> from cap_inode_removexattr and cap_inode_setxattr into >> selinux_inode_setotherxattr. However the code has now diverged and >> selinux is implementing a policy that is quite different than >> cap_inode_setxattr and cap_inode_removexattr especially when it comes >> to the security.capable xattr. >> >> To keep things working and to make the comments in >> security/security.c >> correct when the xattr is securit.capable, call cap_inode_setxattr >> or cap_inode_removexattr as appropriate. >> >> I suspect there is a larger conversation to be had here but this >> is enough to keep selinux from implementing a non-sense hard coded >> policy that breaks other parts of the kernel. > > Originally SELinux called the cap functions directly since there was no > stacking support in the infrastructure and one had to manually stack a > secondary module internally. inode_setxattr and inode_removexattr > however were special cases because the cap functions would check > CAP_SYS_ADMIN for any non-capability attributes in the security.* > namespace, and we don't want to impose that requirement on setting > security.selinux. Thus, we inlined the capabilities logic into the > selinux hook functions and adapted it appropriately. When the stacking > support was introduced, it had to also special case these hooks so that > only the primary module's hook is used for the same reason; otherwise, > the kernel would end up applying a CAP_SYS_ADMIN check on setting > security.selinux. Your change below is almost but not quite right > since it only calls the cap functions when setting the capability > attribute; the residual problem is that it will then skip the SELinux > FILE__SETATTR (file setattr) permission check when setting those > attributes, which we want to retain. So you need to only return early > if cap_inode_setxattr()/removexattr() return an error; otherwise, you > need to proceed to the SELinux check, and you can then delete the > duplicated logic from selinux_inode_setotherxattr(). At which point it > just becomes a call to dentry_has_perm() and you can just inline that > into selinux_inode_setxattr() and selinux_inode_removexattr(). I will look at that. Thank you, Eric _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
