| Message ID | 20220217143457.75229-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: log anon inode class name | expand |
On Thu, Feb 17, 2022 at 9:35 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Log the anonymous inode class name in the security hook > inode_init_security_anon. This name is the key for name based type > transitions on the anon_inode security class on creation. Example: > > type=AVC msg=audit(02/16/22 22:02:50.585:216) : avc: granted { create } for pid=2136 comm=mariadbd anonclass="[io_uring]" dev="anon_inodefs" ino=6871 scontext=system_u:system_r:mysqld_t:s0 tcontext=system_u:system_r:mysqld_iouring_t:s0 tclass=anon_inode > > Add a new LSM audit data type holding the inode and the class name. > > Also warn if the security hook gets called with no name set; currently > the only caller fs/anon_inodes.c:anon_inode_make_secure_inode() passes > one. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > include/linux/lsm_audit.h | 5 +++++ > security/lsm_audit.c | 21 +++++++++++++++++++++ > security/selinux/hooks.c | 7 +++++-- > 3 files changed, 31 insertions(+), 2 deletions(-) ... > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > index 17d02eda9538..8135a88d6d82 100644 > --- a/include/linux/lsm_audit.h > +++ b/include/linux/lsm_audit.h > @@ -76,6 +76,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_IBENDPORT 14 > #define LSM_AUDIT_DATA_LOCKDOWN 15 > #define LSM_AUDIT_DATA_NOTIFICATION 16 > +#define LSM_AUDIT_DATA_ANONINODE 17 > union { > struct path path; > struct dentry *dentry; > @@ -96,6 +97,10 @@ struct common_audit_data { > struct lsm_ibpkey_audit *ibpkey; > struct lsm_ibendport_audit *ibendport; > int reason; > + struct { > + struct inode *inode; > + const char *anonclass; > + } anoninode_struct; > } u; > /* this union contains LSM specific data */ > union { > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > index 1897cbf6fc69..5545fed35539 100644 > --- a/security/lsm_audit.c > +++ b/security/lsm_audit.c > @@ -433,6 +433,27 @@ static void dump_common_audit_data(struct audit_buffer *ab, > audit_log_format(ab, " lockdown_reason=\"%s\"", > lockdown_reasons[a->u.reason]); > break; > + case LSM_AUDIT_DATA_ANONINODE: { > + struct dentry *dentry; > + struct inode *inode; > + > + rcu_read_lock(); > + inode = a->u.anoninode_struct.inode; > + dentry = d_find_alias_rcu(inode); > + if (dentry) { > + audit_log_format(ab, " name="); > + spin_lock(&dentry->d_lock); > + audit_log_untrustedstring(ab, dentry->d_name.name); > + spin_unlock(&dentry->d_lock); > + } I'm not sure we are ever going to get a useful dentry name for anonymous inodes, I think we can probably drop this. The "anonclass=" field will likely be much more interesting and helpful. > + audit_log_format(ab, " anonclass="); > + audit_log_untrustedstring(ab, a->u.anoninode_struct.anonclass); > + audit_log_format(ab, " dev="); > + audit_log_untrustedstring(ab, inode->i_sb->s_id); I'm pretty sure this is always going to end up being "anon_inodefs" and thus not very useful. > + audit_log_format(ab, " ino=%lu", inode->i_ino); Similarly, I'm not sure how useful the inode number is in practice. I've never tried, but can a user lookup an anonymous inode via the inode number? > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index dafabb4dcc64..19c831d94d9b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2932,6 +2932,8 @@ static int selinux_inode_init_security_anon(struct inode *inode, > if (unlikely(!selinux_initialized(&selinux_state))) > return 0; > > + WARN_ON(!name); > + > isec = selinux_inode(inode); > > /* > @@ -2965,8 +2967,9 @@ static int selinux_inode_init_security_anon(struct inode *inode, > * allowed to actually create this type of anonymous inode. > */ > > - ad.type = LSM_AUDIT_DATA_INODE; > - ad.u.inode = inode; > + ad.type = LSM_AUDIT_DATA_ANONINODE; > + ad.u.anoninode_struct.inode = inode; > + ad.u.anoninode_struct.anonclass = name ? (const char *)name->name : "unknown(null)"; This doesn't seem to match well with the newly added WARN_ON() assertion above. I would suggest dropping the WARN_ON() assertion as security_transition_sid() can already handle that safely, and leaving the tertiary statement above; however I think we should probably change the anonclass string to "?" as that is the common unset field value used by audit.
On Fri, 25 Feb 2022 at 01:25, Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:35 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Log the anonymous inode class name in the security hook > > inode_init_security_anon. This name is the key for name based type > > transitions on the anon_inode security class on creation. Example: > > > > type=AVC msg=audit(02/16/22 22:02:50.585:216) : avc: granted { create } for pid=2136 comm=mariadbd anonclass="[io_uring]" dev="anon_inodefs" ino=6871 scontext=system_u:system_r:mysqld_t:s0 tcontext=system_u:system_r:mysqld_iouring_t:s0 tclass=anon_inode > > > > Add a new LSM audit data type holding the inode and the class name. > > > > Also warn if the security hook gets called with no name set; currently > > the only caller fs/anon_inodes.c:anon_inode_make_secure_inode() passes > > one. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > include/linux/lsm_audit.h | 5 +++++ > > security/lsm_audit.c | 21 +++++++++++++++++++++ > > security/selinux/hooks.c | 7 +++++-- > > 3 files changed, 31 insertions(+), 2 deletions(-) > > ... > > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > > index 17d02eda9538..8135a88d6d82 100644 > > --- a/include/linux/lsm_audit.h > > +++ b/include/linux/lsm_audit.h > > @@ -76,6 +76,7 @@ struct common_audit_data { > > #define LSM_AUDIT_DATA_IBENDPORT 14 > > #define LSM_AUDIT_DATA_LOCKDOWN 15 > > #define LSM_AUDIT_DATA_NOTIFICATION 16 > > +#define LSM_AUDIT_DATA_ANONINODE 17 > > union { > > struct path path; > > struct dentry *dentry; > > @@ -96,6 +97,10 @@ struct common_audit_data { > > struct lsm_ibpkey_audit *ibpkey; > > struct lsm_ibendport_audit *ibendport; > > int reason; > > + struct { > > + struct inode *inode; > > + const char *anonclass; > > + } anoninode_struct; > > } u; > > /* this union contains LSM specific data */ > > union { > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > > index 1897cbf6fc69..5545fed35539 100644 > > --- a/security/lsm_audit.c > > +++ b/security/lsm_audit.c > > @@ -433,6 +433,27 @@ static void dump_common_audit_data(struct audit_buffer *ab, > > audit_log_format(ab, " lockdown_reason=\"%s\"", > > lockdown_reasons[a->u.reason]); > > break; > > + case LSM_AUDIT_DATA_ANONINODE: { > > + struct dentry *dentry; > > + struct inode *inode; > > + > > + rcu_read_lock(); > > + inode = a->u.anoninode_struct.inode; > > + dentry = d_find_alias_rcu(inode); > > + if (dentry) { > > + audit_log_format(ab, " name="); > > + spin_lock(&dentry->d_lock); > > + audit_log_untrustedstring(ab, dentry->d_name.name); > > + spin_unlock(&dentry->d_lock); > > + } > > I'm not sure we are ever going to get a useful dentry name for > anonymous inodes, I think we can probably drop this. The "anonclass=" > field will likely be much more interesting and helpful. > > > + audit_log_format(ab, " anonclass="); > > + audit_log_untrustedstring(ab, a->u.anoninode_struct.anonclass); > > + audit_log_format(ab, " dev="); > > + audit_log_untrustedstring(ab, inode->i_sb->s_id); > > I'm pretty sure this is always going to end up being "anon_inodefs" > and thus not very useful. > > > + audit_log_format(ab, " ino=%lu", inode->i_ino); > > Similarly, I'm not sure how useful the inode number is in practice. > I've never tried, but can a user lookup an anonymous inode via the > inode number? > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index dafabb4dcc64..19c831d94d9b 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2932,6 +2932,8 @@ static int selinux_inode_init_security_anon(struct inode *inode, > > if (unlikely(!selinux_initialized(&selinux_state))) > > return 0; > > > > + WARN_ON(!name); > > + > > isec = selinux_inode(inode); > > > > /* > > @@ -2965,8 +2967,9 @@ static int selinux_inode_init_security_anon(struct inode *inode, > > * allowed to actually create this type of anonymous inode. > > */ > > > > - ad.type = LSM_AUDIT_DATA_INODE; > > - ad.u.inode = inode; > > + ad.type = LSM_AUDIT_DATA_ANONINODE; > > + ad.u.anoninode_struct.inode = inode; > > + ad.u.anoninode_struct.anonclass = name ? (const char *)name->name : "unknown(null)"; > > This doesn't seem to match well with the newly added WARN_ON() > assertion above. I would suggest dropping the WARN_ON() assertion as > security_transition_sid() can already handle that safely, and leaving > the tertiary statement above; however I think we should probably > change the anonclass string to "?" as that is the common unset field > value used by audit. Is the hook inode_init_security_anon expected to be called with an empty name though? The condition was just a fallback to not crash the kernel. (Dropped in v2.) > > -- > paul-moore.com
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h index 17d02eda9538..8135a88d6d82 100644 --- a/include/linux/lsm_audit.h +++ b/include/linux/lsm_audit.h @@ -76,6 +76,7 @@ struct common_audit_data { #define LSM_AUDIT_DATA_IBENDPORT 14 #define LSM_AUDIT_DATA_LOCKDOWN 15 #define LSM_AUDIT_DATA_NOTIFICATION 16 +#define LSM_AUDIT_DATA_ANONINODE 17 union { struct path path; struct dentry *dentry; @@ -96,6 +97,10 @@ struct common_audit_data { struct lsm_ibpkey_audit *ibpkey; struct lsm_ibendport_audit *ibendport; int reason; + struct { + struct inode *inode; + const char *anonclass; + } anoninode_struct; } u; /* this union contains LSM specific data */ union { diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 1897cbf6fc69..5545fed35539 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -433,6 +433,27 @@ static void dump_common_audit_data(struct audit_buffer *ab, audit_log_format(ab, " lockdown_reason=\"%s\"", lockdown_reasons[a->u.reason]); break; + case LSM_AUDIT_DATA_ANONINODE: { + struct dentry *dentry; + struct inode *inode; + + rcu_read_lock(); + inode = a->u.anoninode_struct.inode; + dentry = d_find_alias_rcu(inode); + if (dentry) { + audit_log_format(ab, " name="); + spin_lock(&dentry->d_lock); + audit_log_untrustedstring(ab, dentry->d_name.name); + spin_unlock(&dentry->d_lock); + } + audit_log_format(ab, " anonclass="); + audit_log_untrustedstring(ab, a->u.anoninode_struct.anonclass); + audit_log_format(ab, " dev="); + audit_log_untrustedstring(ab, inode->i_sb->s_id); + audit_log_format(ab, " ino=%lu", inode->i_ino); + rcu_read_unlock(); + break; + } } /* switch (a->type) */ } diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index dafabb4dcc64..19c831d94d9b 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2932,6 +2932,8 @@ static int selinux_inode_init_security_anon(struct inode *inode, if (unlikely(!selinux_initialized(&selinux_state))) return 0; + WARN_ON(!name); + isec = selinux_inode(inode); /* @@ -2965,8 +2967,9 @@ static int selinux_inode_init_security_anon(struct inode *inode, * allowed to actually create this type of anonymous inode. */ - ad.type = LSM_AUDIT_DATA_INODE; - ad.u.inode = inode; + ad.type = LSM_AUDIT_DATA_ANONINODE; + ad.u.anoninode_struct.inode = inode; + ad.u.anoninode_struct.anonclass = name ? (const char *)name->name : "unknown(null)"; return avc_has_perm(&selinux_state, tsec->sid,
Log the anonymous inode class name in the security hook inode_init_security_anon. This name is the key for name based type transitions on the anon_inode security class on creation. Example: type=AVC msg=audit(02/16/22 22:02:50.585:216) : avc: granted { create } for pid=2136 comm=mariadbd anonclass="[io_uring]" dev="anon_inodefs" ino=6871 scontext=system_u:system_r:mysqld_t:s0 tcontext=system_u:system_r:mysqld_iouring_t:s0 tclass=anon_inode Add a new LSM audit data type holding the inode and the class name. Also warn if the security hook gets called with no name set; currently the only caller fs/anon_inodes.c:anon_inode_make_secure_inode() passes one. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- include/linux/lsm_audit.h | 5 +++++ security/lsm_audit.c | 21 +++++++++++++++++++++ security/selinux/hooks.c | 7 +++++-- 3 files changed, 31 insertions(+), 2 deletions(-)