| Message ID | alpine.LRH.2.00.1803101900250.24419@sjc-ads-6991.cisco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/10/2018 10:08 PM, Victor Kamensky wrote: > > > On Tue, 20 Feb 2018, Stephen Smalley wrote: > >> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote: >>> From: Victor Kamensky <kamensky@cisco.com> >>> >>> With initramfs cpio format that supports extended attributes >>> we need to skip sid population on sys_lsetxattr call from >>> initramfs for rootfs if security server is not initialized yet. >>> >>> Otherwise callback in selinux_inode_post_setxattr will try to >>> translate give security.selinux label into sid context and since >>> security server is not available yet inode will receive default >>> sid (typically kernel_t). Note that in the same time proper >>> label will be stored in inode xattrs. Later, since inode sid >>> would be already populated system will never look back at >>> actual xattrs. But if we skip sid population for rootfs and >>> we have policy that direct use of xattrs for rootfs, proper >>> sid will be filled in from extended attributes one node is >>> accessed and server is initialized. >>> >>> Note new DELAYAFTERINIT_MNT super block flag is introduced >>> to only mark rootfs for such behavior. For other types of >>> tmpfs original logic is still used. >> >> (cc selinux maintainers) >> >> Wondering if we shouldn't just do this always, for all filesystem >> types. > > Ok, I think it makes sense. The one that do not support xattrs > will not reach selinux_inode_post_setxattr anyway. And try > to cache sid while !ss_initialized is not good idea for any > filesystem types. > >> Also, I think this should likely also be done in >> selinux_inode_setsecurity() for consistency. > > I am not sure that I follow selinux_inode_setsecurity suggestion. > selinux_inode_setsecurity is about permission check. And > selinux_inode_post_setxattr deals with processing and setting > side effects if xattr was "security.selinux", it does not > matter what happens in selinux_inode_setsecurity if it > returns access_ok, LSM will still call selinux_inode_post_setxattr > and we would need to check and not produce any sid caching > side effects if !ss_initialized. selinux_inode_setsecurity is the vfs fallback for setting security attributes when the filesystem/inode does not support setxattr itself, and is also used by kernfs. So you need to update both selinux_inode_post_setxattr and selinux_inode_setsecurity in the same way. > > Sitll keeping logic in selinux_inode_post_setxattr, checked > that the following with much simple code works too: > >> From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001 > From: Victor Kamensky <kamensky@cisco.com> > Date: Fri, 9 Mar 2018 23:06:08 -0800 > Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy > loaded > > With initramfs cpio format that supports extended attributes > we need to skip sid population on sys_lsetxattr call from > initramfs for rootfs if security server is not initialized yet. > > Otherwise callback in selinux_inode_post_setxattr will try to > translate give security.selinux label into sid context and since > security server is not available yet inode will receive default > sid (typically kernel_t). Note that in the same time proper > label will be stored in inode xattrs. Later, since inode sid > would be already populated system will never look back at > actual xattrs. But if we skip sid population for rootfs and > we have policy that direct use of xattrs for rootfs, proper > sid will be filled in from extended attributes one node is > accessed and server is initialized. > > Signed-off-by: Victor Kamensky <kamensky@cisco.com> > --- > security/selinux/hooks.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 31303ed..4c13759 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, > return; > } > > + if (!ss_initialized) { > + return; > + } > + > rc = security_context_to_sid_force(value, size, &newsid); > if (rc) { > printk(KERN_ERR "SELinux: unable to map context to SID" -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 31303ed..4c13759 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, return; } + if (!ss_initialized) { + return; + } + rc = security_context_to_sid_force(value, size, &newsid); if (rc) { printk(KERN_ERR "SELinux: unable to map context to SID"