| Message ID | 20210915162326.392808-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: enable genfscon labeling for securityfs | expand |
On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > Add support for genfscon per-file labeling of securityfs files. This allows > for separate labels and therby permissions for different files, e.g. > /sys/kernel/security/integrity/ima/policy. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Hi Christian, It would be nice if you could add some additional notes on how this was tested to the description above. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 6517f221d52c..a18626424731 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > !strcmp(sb->s_type->name, "tracefs") || > !strcmp(sb->s_type->name, "binder") || > !strcmp(sb->s_type->name, "bpf") || > - !strcmp(sb->s_type->name, "pstore")) > + !strcmp(sb->s_type->name, "pstore") || > + !strcmp(sb->s_type->name, "securityfs")) > sbsec->flags |= SE_SBGENFS; > > if (!strcmp(sb->s_type->name, "sysfs") || > -- > 2.33.0
On Wed, 15 Sept 2021 at 20:28, Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > for separate labels and therby permissions for different files, e.g. > > /sys/kernel/security/integrity/ima/policy. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/hooks.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > Hi Christian, > > It would be nice if you could add some additional notes on how this > was tested to the description above. > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 6517f221d52c..a18626424731 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > > !strcmp(sb->s_type->name, "tracefs") || > > !strcmp(sb->s_type->name, "binder") || > > !strcmp(sb->s_type->name, "bpf") || > > - !strcmp(sb->s_type->name, "pstore")) > > + !strcmp(sb->s_type->name, "pstore") || > > + !strcmp(sb->s_type->name, "securityfs")) > > sbsec->flags |= SE_SBGENFS; > > > > if (!strcmp(sb->s_type->name, "sysfs") || > > -- > > 2.33.0 > > -- > paul moore > www.paul-moore.com Something like: Add support for genfscon per-file labeling of securityfs files. This allows for separate labels and thereby access control for different files. For example a genfscon statement genfscon securityfs /integrity/ima/policy system_u:object_r:ima_policy_t:s0 will set a specific label to the IMA policy file and thus allow to control the ability to set the IMA policy. Setting labels directly, e.g. via chcon(1) or setfiles(8), is still not supported. ?
On Thu, Sep 16, 2021 at 1:41 PM Christian Göttsche <cgzones@googlemail.com> wrote: > On Wed, 15 Sept 2021 at 20:28, Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > > for separate labels and therby permissions for different files, e.g. > > > /sys/kernel/security/integrity/ima/policy. > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > --- > > > security/selinux/hooks.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Hi Christian, > > > > It would be nice if you could add some additional notes on how this > > was tested to the description above. > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 6517f221d52c..a18626424731 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > > > !strcmp(sb->s_type->name, "tracefs") || > > > !strcmp(sb->s_type->name, "binder") || > > > !strcmp(sb->s_type->name, "bpf") || > > > - !strcmp(sb->s_type->name, "pstore")) > > > + !strcmp(sb->s_type->name, "pstore") || > > > + !strcmp(sb->s_type->name, "securityfs")) > > > sbsec->flags |= SE_SBGENFS; > > > > > > if (!strcmp(sb->s_type->name, "sysfs") || > > > -- > > > 2.33.0 > > > > -- > > paul moore > > www.paul-moore.com > > Something like: > > Add support for genfscon per-file labeling of securityfs files. This allows > for separate labels and thereby access control for different files. > For example a genfscon statement > genfscon securityfs /integrity/ima/policy > system_u:object_r:ima_policy_t:s0 > will set a specific label to the IMA policy file and thus allow to > control the ability > to set the IMA policy. > Setting labels directly, e.g. via chcon(1) or setfiles(8), is > still not supported. > > ? That's a much better description of the functionality, especially for those who may not be very familiar with SELinux, thank you. However I was hoping to also hear some confirmation that you have tested this and it worked without problem?
On Fri, 17 Sept 2021 at 04:07, Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Sep 16, 2021 at 1:41 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > On Wed, 15 Sept 2021 at 20:28, Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche > > > <cgzones@googlemail.com> wrote: > > > > > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > > > for separate labels and therby permissions for different files, e.g. > > > > /sys/kernel/security/integrity/ima/policy. > > > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > --- > > > > security/selinux/hooks.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Hi Christian, > > > > > > It would be nice if you could add some additional notes on how this > > > was tested to the description above. > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 6517f221d52c..a18626424731 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > > > > !strcmp(sb->s_type->name, "tracefs") || > > > > !strcmp(sb->s_type->name, "binder") || > > > > !strcmp(sb->s_type->name, "bpf") || > > > > - !strcmp(sb->s_type->name, "pstore")) > > > > + !strcmp(sb->s_type->name, "pstore") || > > > > + !strcmp(sb->s_type->name, "securityfs")) > > > > sbsec->flags |= SE_SBGENFS; > > > > > > > > if (!strcmp(sb->s_type->name, "sysfs") || > > > > -- > > > > 2.33.0 > > > > > > -- > > > paul moore > > > www.paul-moore.com > > > > Something like: > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > for separate labels and thereby access control for different files. > > For example a genfscon statement > > genfscon securityfs /integrity/ima/policy > > system_u:object_r:ima_policy_t:s0 > > will set a specific label to the IMA policy file and thus allow to > > control the ability > > to set the IMA policy. > > Setting labels directly, e.g. via chcon(1) or setfiles(8), is > > still not supported. > > > > ? > > That's a much better description of the functionality, especially for > those who may not be very familiar with SELinux, thank you. However I > was hoping to also hear some confirmation that you have tested this > and it worked without problem? > > -- > paul moore > www.paul-moore.com With the following genfscon statements genfscon securityfs / system_u:object_r:securityfs_t:s0 genfscon securityfs /integrity/ima system_u:object_r:ima_t:s0 genfscon securityfs /integrity/ima/policy system_u:object_r:ima_policy_t:s0 genfscon securityfs /lockdown system_u:object_r:lockdown_t:s0 the labels are $ ls -laZ /sys/kernel/security/ /sys/kernel/security/integrity/ima/ /sys/kernel/security/: total 0 drwxr-xr-x. 3 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 . drwxr-xr-x. 15 root root system_u:object_r:sysfs_t:s0 0 Sep 17 09:38 .. lr--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 evm -> integrity/evm/evm lr--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 ima -> integrity/ima drwxr-xr-x. 4 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 integrity -rw-r--r--. 1 root root system_u:object_r:lockdown_t:s0 0 Sep 17 09:38 lockdown -r--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 lsm /sys/kernel/security/integrity/ima/: total 0 drwxr-xr-x. 2 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 . rwxr-xr-x. 4 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 .. -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 ascii_runtime_measurements -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 binary_runtime_measurements --w-------. 1 root root system_u:object_r:ima_policy_t:s0 0 Sep 17 09:38 policy -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 runtime_measurements_count -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 violations and audit logs are generated like Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { read } for pid=331 comm="systemd-tmpfile" name="ima" dev="securityfs" ino=167 scontext=system_u:system_r:systemd_tmpfiles_t:s0 tcontext=system_u:object_r:ima_t:s0 tclass=dir permissive=1 Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { open } for pid=331 comm="systemd-tmpfile" path="/sys/kernel/security/integrity/ima" dev="securityfs" ino=167 scontext=system_u:system_r:systemd_tmpfiles_t:s0 tcontext=system_u:object_r:ima_t:s0 tclass=dir permissive=1 Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { getattr } for pid=331 comm="systemd-tmpfile" path="/sys/kernel/security/integrity/ima/policy" dev="securityfs" ino=173 scontext=system_u:system_r:systemd_tmpfiles_t:s0 tcontext=system_u:object_r:ima_policy_t:s0 tclass=file permissive=1 chcon(1) fails like $ chcon -v -t securityfs_t /sys/kernel/security/integrity/ima/policy changing security context of '/sys/kernel/security/integrity/ima/policy' chcon: failed to change context of '/sys/kernel/security/integrity/ima/policy' to 'system_u:object_r:securityfs_t:s0': Operation not supported A file context definition of /sys/kernel/security/integrity/ima/policy -- gen_context(system_u:object_r:securityfs_t,s0) results in $ matchpathcon -V /sys/kernel/security/integrity/ima/policy Deprecated, use selabel_lookup /sys/kernel/security/integrity/ima/policy has context system_u:object_r:ima_policy_t:s0, should be system_u:object_r:securityfs_t:s0 $ restorecon -vRF /sys/kernel/security/ # nothing # Are there any other use cases or edge cases I need to test. One thing I noticed is that a genfscon statements take affect only after a reboot, not after remounting the securityfs via mount -v -o remount -t securityfs /sys/kernel/security Is this expected?
On Fri, Sep 17, 2021 at 9:07 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Fri, 17 Sept 2021 at 04:07, Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Sep 16, 2021 at 1:41 PM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > On Wed, 15 Sept 2021 at 20:28, Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche > > > > <cgzones@googlemail.com> wrote: > > > > > > > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > > > > for separate labels and therby permissions for different files, e.g. > > > > > /sys/kernel/security/integrity/ima/policy. > > > > > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > > --- > > > > > security/selinux/hooks.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > Hi Christian, > > > > > > > > It would be nice if you could add some additional notes on how this > > > > was tested to the description above. > > > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > > index 6517f221d52c..a18626424731 100644 > > > > > --- a/security/selinux/hooks.c > > > > > +++ b/security/selinux/hooks.c > > > > > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > > > > > !strcmp(sb->s_type->name, "tracefs") || > > > > > !strcmp(sb->s_type->name, "binder") || > > > > > !strcmp(sb->s_type->name, "bpf") || > > > > > - !strcmp(sb->s_type->name, "pstore")) > > > > > + !strcmp(sb->s_type->name, "pstore") || > > > > > + !strcmp(sb->s_type->name, "securityfs")) > > > > > sbsec->flags |= SE_SBGENFS; > > > > > > > > > > if (!strcmp(sb->s_type->name, "sysfs") || > > > > > -- > > > > > 2.33.0 > > > > > > > > -- > > > > paul moore > > > > www.paul-moore.com > > > > > > Something like: > > > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > > for separate labels and thereby access control for different files. > > > For example a genfscon statement > > > genfscon securityfs /integrity/ima/policy > > > system_u:object_r:ima_policy_t:s0 > > > will set a specific label to the IMA policy file and thus allow to > > > control the ability > > > to set the IMA policy. > > > Setting labels directly, e.g. via chcon(1) or setfiles(8), is > > > still not supported. > > > > > > ? > > > > That's a much better description of the functionality, especially for > > those who may not be very familiar with SELinux, thank you. However I > > was hoping to also hear some confirmation that you have tested this > > and it worked without problem? > > > > -- > > paul moore > > www.paul-moore.com > > With the following genfscon statements > > genfscon securityfs / system_u:object_r:securityfs_t:s0 > genfscon securityfs /integrity/ima system_u:object_r:ima_t:s0 > genfscon securityfs /integrity/ima/policy system_u:object_r:ima_policy_t:s0 > genfscon securityfs /lockdown system_u:object_r:lockdown_t:s0 > > the labels are > > $ ls -laZ /sys/kernel/security/ /sys/kernel/security/integrity/ima/ > /sys/kernel/security/: > total 0 > drwxr-xr-x. 3 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 . > drwxr-xr-x. 15 root root system_u:object_r:sysfs_t:s0 0 Sep 17 09:38 .. > lr--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep > 17 09:38 evm -> integrity/evm/evm > lr--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep > 17 09:38 ima -> integrity/ima > drwxr-xr-x. 4 root root system_u:object_r:securityfs_t:s0 0 Sep > 17 09:38 integrity > -rw-r--r--. 1 root root system_u:object_r:lockdown_t:s0 0 Sep > 17 09:38 lockdown > -r--r--r--. 1 root root system_u:object_r:securityfs_t:s0 0 Sep > 17 09:38 lsm > > /sys/kernel/security/integrity/ima/: > total 0 > drwxr-xr-x. 2 root root system_u:object_r:ima_t:s0 0 Sep 17 09:38 . > rwxr-xr-x. 4 root root system_u:object_r:securityfs_t:s0 0 Sep 17 09:38 .. > -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 > 09:38 ascii_runtime_measurements > -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 > 09:38 binary_runtime_measurements > --w-------. 1 root root system_u:object_r:ima_policy_t:s0 0 Sep 17 > 09:38 policy > -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 > 09:38 runtime_measurements_count > -r--r-----. 1 root root system_u:object_r:ima_t:s0 0 Sep 17 > 09:38 violations > > and audit logs are generated like > > Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { > read } for pid=331 comm="systemd-tmpfile" name="ima" dev="securityfs" > ino=167 scontext=system_u:system_r:systemd_tmpfiles_t:s0 > tcontext=system_u:object_r:ima_t:s0 tclass=dir permissive=1 > Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { > open } for pid=331 comm="systemd-tmpfile" > path="/sys/kernel/security/integrity/ima" dev="securityfs" ino=167 > scontext=system_u:system_r:systemd_tmpfiles_t:s0 > tcontext=system_u:object_r:ima_t:s0 tclass=dir permissive=1 > Sep 17 09:38:28 debianBullseye audit[331]: AVC avc: denied { > getattr } for pid=331 comm="systemd-tmpfile" > path="/sys/kernel/security/integrity/ima/policy" dev="securityfs" > ino=173 scontext=system_u:system_r:systemd_tmpfiles_t:s0 > tcontext=system_u:object_r:ima_policy_t:s0 tclass=file permissive=1 > > > chcon(1) fails like > > $ chcon -v -t securityfs_t /sys/kernel/security/integrity/ima/policy > changing security context of '/sys/kernel/security/integrity/ima/policy' > chcon: failed to change context of > '/sys/kernel/security/integrity/ima/policy' to > 'system_u:object_r:securityfs_t:s0': Operation not supported > > A file context definition of > > /sys/kernel/security/integrity/ima/policy -- > gen_context(system_u:object_r:securityfs_t,s0) > > results in > > $ matchpathcon -V /sys/kernel/security/integrity/ima/policy > Deprecated, use selabel_lookup > /sys/kernel/security/integrity/ima/policy has context > system_u:object_r:ima_policy_t:s0, should be > system_u:object_r:securityfs_t:s0 > $ restorecon -vRF /sys/kernel/security/ > # nothing # > > Are there any other use cases or edge cases I need to test. > > One thing I noticed is that a genfscon statements take affect only > after a reboot, not after remounting the securityfs via > > mount -v -o remount -t securityfs /sys/kernel/security > > Is this expected? genfscon is only consulted when a dentry is first instantiated for an inode, typically on first lookup or creation. So you'd have to unmount all securityfs mounts to force eviction of the existing dentries/inodes and get them re-created. And likely can't be done if the kernel does an internal mount for its own usage (but doesn't look like that is the case for securityfs, just selinuxfs).
On Thu, Sep 16, 2021 at 10:07 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Sep 16, 2021 at 1:41 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > On Wed, 15 Sept 2021 at 20:28, Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Sep 15, 2021 at 12:24 PM Christian Göttsche > > > <cgzones@googlemail.com> wrote: > > > > > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > > > for separate labels and therby permissions for different files, e.g. > > > > /sys/kernel/security/integrity/ima/policy. > > > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > --- > > > > security/selinux/hooks.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Hi Christian, > > > > > > It would be nice if you could add some additional notes on how this > > > was tested to the description above. > > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > > index 6517f221d52c..a18626424731 100644 > > > > --- a/security/selinux/hooks.c > > > > +++ b/security/selinux/hooks.c > > > > @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, > > > > !strcmp(sb->s_type->name, "tracefs") || > > > > !strcmp(sb->s_type->name, "binder") || > > > > !strcmp(sb->s_type->name, "bpf") || > > > > - !strcmp(sb->s_type->name, "pstore")) > > > > + !strcmp(sb->s_type->name, "pstore") || > > > > + !strcmp(sb->s_type->name, "securityfs")) > > > > sbsec->flags |= SE_SBGENFS; > > > > > > > > if (!strcmp(sb->s_type->name, "sysfs") || > > > > -- > > > > 2.33.0 > > > > > > -- > > > paul moore > > > www.paul-moore.com > > > > Something like: > > > > Add support for genfscon per-file labeling of securityfs files. This allows > > for separate labels and thereby access control for different files. > > For example a genfscon statement > > genfscon securityfs /integrity/ima/policy > > system_u:object_r:ima_policy_t:s0 > > will set a specific label to the IMA policy file and thus allow to > > control the ability > > to set the IMA policy. > > Setting labels directly, e.g. via chcon(1) or setfiles(8), is > > still not supported. > > > > ? > > That's a much better description of the functionality, especially for > those who may not be very familiar with SELinux, thank you. However I > was hoping to also hear some confirmation that you have tested this > and it worked without problem? Hi Christian, my apologies on the delay, I was distracted by a few other SELinux issues. Thank you for sending out your testing notes in the meantime. Are you okay if I replace the original commit description with your more verbose version? If not, could you resend the patch with the update commit description? Thanks.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6517f221d52c..a18626424731 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -760,7 +760,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, !strcmp(sb->s_type->name, "tracefs") || !strcmp(sb->s_type->name, "binder") || !strcmp(sb->s_type->name, "bpf") || - !strcmp(sb->s_type->name, "pstore")) + !strcmp(sb->s_type->name, "pstore") || + !strcmp(sb->s_type->name, "securityfs")) sbsec->flags |= SE_SBGENFS; if (!strcmp(sb->s_type->name, "sysfs") ||
Add support for genfscon per-file labeling of securityfs files. This allows for separate labels and therby permissions for different files, e.g. /sys/kernel/security/integrity/ima/policy. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/hooks.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)