| Message ID | 20231018100815.26278-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | RFC: selinux: don't filter copy-up xattrs while uninitialized | expand |
On Wed, Oct 18, 2023 at 6:08 AM David Disseldorp <ddiss@suse.de> wrote: > > Extended attribute copy-up functionality added via 19472b69d639d > ("selinux: Implementation for inode_copy_up_xattr() hook") sees > "security.selinux" contexts dropped, instead relying on contexts > applied via the inode_copy_up() hook. > > When copy-up takes place during early boot, prior to selinux > initialization / policy load, the context stripping can be unwanted > and unexpected. Make filtering dependent on selinux_initialized(). > > RFC: This changes user behaviour so is likely unacceptable. Still, > I'd be interested in hearing other suggestions for how this could be > addressed. IMHO, this is fixing a bug, only affects early userspace (pre policy load), and is likely acceptable. But Paul will make the final call. We can't introduce and use a new policy capability here because this is before policy has been loaded. > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 2aa0e219d7217..fb3e53bb7e90c 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3527,7 +3527,7 @@ static int selinux_inode_copy_up_xattr(const char *name) > * don't then want to overwrite it by blindly copying all the lower > * xattrs up. Instead, we have to filter out SELinux-related xattrs. > */ > - if (strcmp(name, XATTR_NAME_SELINUX) == 0) > + if (selinux_initialized() && strcmp(name, XATTR_NAME_SELINUX) == 0) > return 1; /* Discard */ > /* > * Any other attribute apart from SELINUX is not claimed, supported > -- > 2.35.3
On Fri, Oct 20, 2023 at 8:21 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Wed, Oct 18, 2023 at 6:08 AM David Disseldorp <ddiss@suse.de> wrote: > > > > Extended attribute copy-up functionality added via 19472b69d639d > > ("selinux: Implementation for inode_copy_up_xattr() hook") sees > > "security.selinux" contexts dropped, instead relying on contexts > > applied via the inode_copy_up() hook. > > > > When copy-up takes place during early boot, prior to selinux > > initialization / policy load, the context stripping can be unwanted > > and unexpected. Make filtering dependent on selinux_initialized(). > > > > RFC: This changes user behaviour so is likely unacceptable. Still, > > I'd be interested in hearing other suggestions for how this could be > > addressed. > > IMHO, this is fixing a bug, only affects early userspace (pre policy > load), and is likely acceptable. > But Paul will make the final call. We can't introduce and use a new > policy capability here because this is before policy has been loaded. I agree with Stephen, this is a bug fix so I wouldn't worry too much about user visible behavior. For better or worse, the SELinux-enabled-but-no-policy-loaded case has always been a bit awkward and has required multiple patches over the years to correct unwanted behaviors. I'm open to comments on this, but I don't believe this is something we want to see backported to the stable kernels, and considering we are currently at v6.6-rc6, this isn't really a candidate for the upcoming merge window. This means we have a few more weeks to comment, test, etc. and one of the things I would like to see is a better description of before-and-after labeling in the commit description. This helps people who trip over this change, identify what changed, and helps them resolve the problem on their systems. Does that sound good?
Hi Paul and Stephen, On Fri, 20 Oct 2023 11:55:31 -0400, Paul Moore wrote: > On Fri, Oct 20, 2023 at 8:21 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Wed, Oct 18, 2023 at 6:08 AM David Disseldorp <ddiss@suse.de> wrote: > > > > > > Extended attribute copy-up functionality added via 19472b69d639d > > > ("selinux: Implementation for inode_copy_up_xattr() hook") sees > > > "security.selinux" contexts dropped, instead relying on contexts > > > applied via the inode_copy_up() hook. > > > > > > When copy-up takes place during early boot, prior to selinux > > > initialization / policy load, the context stripping can be unwanted > > > and unexpected. Make filtering dependent on selinux_initialized(). > > > > > > RFC: This changes user behaviour so is likely unacceptable. Still, > > > I'd be interested in hearing other suggestions for how this could be > > > addressed. > > > > IMHO, this is fixing a bug, only affects early userspace (pre policy > > load), and is likely acceptable. > > But Paul will make the final call. We can't introduce and use a new > > policy capability here because this is before policy has been loaded. > > I agree with Stephen, this is a bug fix so I wouldn't worry too much > about user visible behavior. For better or worse, the > SELinux-enabled-but-no-policy-loaded case has always been a bit > awkward and has required multiple patches over the years to correct > unwanted behaviors. Understood. > I'm open to comments on this, but I don't believe this is something we > want to see backported to the stable kernels, and considering we are > currently at v6.6-rc6, this isn't really a candidate for the upcoming > merge window. This means we have a few more weeks to comment, test, > etc. and one of the things I would like to see is a better description > of before-and-after labeling in the commit description. This helps > people who trip over this change, identify what changed, and helps > them resolve the problem on their systems. > > Does that sound good? That sounds good to me. I'll rework the commit description (and comment above this change), do some further testing and then submit a v2. Thanks for your feedback, David
On Fri, Oct 20, 2023 at 4:33 PM David Disseldorp <ddiss@suse.de> wrote: > > Hi Paul and Stephen, > > On Fri, 20 Oct 2023 11:55:31 -0400, Paul Moore wrote: > > > On Fri, Oct 20, 2023 at 8:21 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > On Wed, Oct 18, 2023 at 6:08 AM David Disseldorp <ddiss@suse.de> wrote: > > > > > > > > Extended attribute copy-up functionality added via 19472b69d639d > > > > ("selinux: Implementation for inode_copy_up_xattr() hook") sees > > > > "security.selinux" contexts dropped, instead relying on contexts > > > > applied via the inode_copy_up() hook. > > > > > > > > When copy-up takes place during early boot, prior to selinux > > > > initialization / policy load, the context stripping can be unwanted > > > > and unexpected. Make filtering dependent on selinux_initialized(). > > > > > > > > RFC: This changes user behaviour so is likely unacceptable. Still, > > > > I'd be interested in hearing other suggestions for how this could be > > > > addressed. > > > > > > IMHO, this is fixing a bug, only affects early userspace (pre policy > > > load), and is likely acceptable. > > > But Paul will make the final call. We can't introduce and use a new > > > policy capability here because this is before policy has been loaded. > > > > I agree with Stephen, this is a bug fix so I wouldn't worry too much > > about user visible behavior. For better or worse, the > > SELinux-enabled-but-no-policy-loaded case has always been a bit > > awkward and has required multiple patches over the years to correct > > unwanted behaviors. > > Understood. > > > I'm open to comments on this, but I don't believe this is something we > > want to see backported to the stable kernels, and considering we are > > currently at v6.6-rc6, this isn't really a candidate for the upcoming > > merge window. This means we have a few more weeks to comment, test, > > etc. and one of the things I would like to see is a better description > > of before-and-after labeling in the commit description. This helps > > people who trip over this change, identify what changed, and helps > > them resolve the problem on their systems. > > > > Does that sound good? > > That sounds good to me. I'll rework the commit description (and comment > above this change), do some further testing and then submit a v2. Hi David, No rush, I just wanted to check in on this and see how things were going?
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 2aa0e219d7217..fb3e53bb7e90c 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3527,7 +3527,7 @@ static int selinux_inode_copy_up_xattr(const char *name) * don't then want to overwrite it by blindly copying all the lower * xattrs up. Instead, we have to filter out SELinux-related xattrs. */ - if (strcmp(name, XATTR_NAME_SELINUX) == 0) + if (selinux_initialized() && strcmp(name, XATTR_NAME_SELINUX) == 0) return 1; /* Discard */ /* * Any other attribute apart from SELINUX is not claimed, supported
Extended attribute copy-up functionality added via 19472b69d639d ("selinux: Implementation for inode_copy_up_xattr() hook") sees "security.selinux" contexts dropped, instead relying on contexts applied via the inode_copy_up() hook. When copy-up takes place during early boot, prior to selinux initialization / policy load, the context stripping can be unwanted and unexpected. Make filtering dependent on selinux_initialized(). RFC: This changes user behaviour so is likely unacceptable. Still, I'd be interested in hearing other suggestions for how this could be addressed. --- security/selinux/hooks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)