diff mbox series

[v2,15/30] evm: add post set acl hook

Message ID 20220926140827.142806-16-brauner@kernel.org (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series acl: add vfs posix acl api | expand

Commit Message

Christian Brauner Sept. 26, 2022, 2:08 p.m. UTC
The security_inode_post_setxattr() hook is used by security modules to
update their own security.* xattrs. Consequently none of the security
modules operate on posix acls. So we don't need an additional security
hook when post setting posix acls.

However, the integrity subsystem wants to be informed about posix acl
changes and specifically evm to update their hashes when the xattrs
change. The callchain for evm_inode_post_setxattr() is:

-> evm_inode_post_setxattr()
   -> evm_update_evmxattr()
      -> evm_calc_hmac()
         -> evm_calc_hmac_or_hash()

and evm_cacl_hmac_or_hash() walks the global list of protected xattr
names evm_config_xattrnames. This global list can be modified via
/sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is
restricted to security.* xattrs and the default xattrs in
evm_config_xattrnames only contains security.* xattrs as well.

So the actual value for posix acls is currently completely irrelevant
for evm during evm_inode_post_setxattr() and frankly it should stay that
way in the future to not cause the vfs any more headaches. But if the
actual posix acl values matter then evm shouldn't operate on the binary
void blob and try to hack around in the uapi struct anyway. Instead it
should then in the future add a dedicated hook which takes a struct
posix_acl argument passing the posix acls in the proper vfs format.

For now it is sufficient to make evm_inode_post_set_acl() a wrapper
around evm_inode_post_setxattr() not passing any actual values down.
This will still cause the hashes to be updated as before.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Notes:
    /* v2 */
    unchanged

 fs/posix_acl.c      |  5 ++++-
 include/linux/evm.h | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Paul Moore Sept. 27, 2022, 10:56 p.m. UTC | #1
On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The security_inode_post_setxattr() hook is used by security modules to
> update their own security.* xattrs. Consequently none of the security
> modules operate on posix acls. So we don't need an additional security
> hook when post setting posix acls.
>
> However, the integrity subsystem wants to be informed about posix acl
> changes and specifically evm to update their hashes when the xattrs
> change. The callchain for evm_inode_post_setxattr() is:
>
> -> evm_inode_post_setxattr()
>    -> evm_update_evmxattr()
>       -> evm_calc_hmac()
>          -> evm_calc_hmac_or_hash()
>
> and evm_cacl_hmac_or_hash() walks the global list of protected xattr
> names evm_config_xattrnames. This global list can be modified via
> /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is
> restricted to security.* xattrs and the default xattrs in
> evm_config_xattrnames only contains security.* xattrs as well.
>
> So the actual value for posix acls is currently completely irrelevant
> for evm during evm_inode_post_setxattr() and frankly it should stay that
> way in the future to not cause the vfs any more headaches. But if the
> actual posix acl values matter then evm shouldn't operate on the binary
> void blob and try to hack around in the uapi struct anyway. Instead it
> should then in the future add a dedicated hook which takes a struct
> posix_acl argument passing the posix acls in the proper vfs format.
>
> For now it is sufficient to make evm_inode_post_set_acl() a wrapper
> around evm_inode_post_setxattr() not passing any actual values down.
> This will still cause the hashes to be updated as before.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>
> Notes:
>     /* v2 */
>     unchanged
>
>  fs/posix_acl.c      |  5 ++++-
>  include/linux/evm.h | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Paul Moore <paul@paul-moore.com>

--
paul-moore.com
diff mbox series

Patch

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 471d17fa1611..ef0908a4bc46 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -25,6 +25,7 @@ 
 #include <linux/namei.h>
 #include <linux/mnt_idmapping.h>
 #include <linux/security.h>
+#include <linux/evm.h>
 #include <linux/fsnotify.h>
 
 static struct posix_acl **acl_by_type(struct inode *inode, int type)
@@ -1351,8 +1352,10 @@  int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
 		error = -EIO;
 	else
 		error = -EOPNOTSUPP;
-	if (!error)
+	if (!error) {
 		fsnotify_xattr(dentry);
+		evm_inode_post_set_acl(dentry, acl_name, kacl);
+	}
 
 out_inode_unlock:
 	inode_unlock(inode);
diff --git a/include/linux/evm.h b/include/linux/evm.h
index aebcfd47d496..7811ce56e02f 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -38,6 +38,12 @@  extern void evm_inode_post_removexattr(struct dentry *dentry,
 extern int evm_inode_set_acl(struct user_namespace *mnt_userns,
 			     struct dentry *dentry, const char *acl_name,
 			     struct posix_acl *kacl);
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name,
+					  struct posix_acl *kacl)
+{
+	return evm_inode_post_setxattr(dentry, acl_name, NULL, 0);
+}
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -118,6 +124,13 @@  static inline int evm_inode_set_acl(struct user_namespace *mnt_userns,
 	return 0;
 }
 
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name,
+					  struct posix_acl *kacl)
+{
+	return;
+}
+
 static inline int evm_inode_init_security(struct inode *inode,
 					  const struct xattr *xattr_array,
 					  struct xattr *evm)