diff mbox series

[3/9] ima: Move xattr hooks into LSM

Message ID 20221013223654.659758-3-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series integrity: Move hooks into LSM | expand

Commit Message

Kees Cook Oct. 13, 2022, 10:36 p.m. UTC 
Move the xattr IMA hooks into normal LSM layer. As with SELinux and
Smack, handle calling cap_inode_setxattr() internally.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jonathan McDowell <noodles@fb.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Petr Vorel <pvorel@suse.cz>
Cc: linux-integrity@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/ima.h                   | 16 ----------------
 security/integrity/ima/ima.h          | 10 ++++++++++
 security/integrity/ima/ima_appraise.c | 19 ++++++++++++++++---
 security/integrity/ima/ima_main.c     |  4 ++++
 security/security.c                   | 10 ++--------
 5 files changed, 32 insertions(+), 27 deletions(-)

Comments

Christian Brauner Oct. 18, 2022, 3:07 p.m. UTC | #1 
On Thu, Oct 13, 2022 at 03:36:48PM -0700, Kees Cook wrote:
> Move the xattr IMA hooks into normal LSM layer. As with SELinux and
> Smack, handle calling cap_inode_setxattr() internally.
> 
> Cc: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Jonathan McDowell <noodles@fb.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Petr Vorel <pvorel@suse.cz>
> Cc: linux-integrity@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

I like that changes obviously but in general, does IMA depend on being
called _after_ all other LSMs or is this just a historical artifact?
Mimi Zohar Oct. 19, 2022, 1:24 p.m. UTC | #2 
On Tue, 2022-10-18 at 17:07 +0200, Christian Brauner wrote:
> On Thu, Oct 13, 2022 at 03:36:48PM -0700, Kees Cook wrote:
> > Move the xattr IMA hooks into normal LSM layer. As with SELinux and
> > Smack, handle calling cap_inode_setxattr() internally.
> > 
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Jonathan McDowell <noodles@fb.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Petr Vorel <pvorel@suse.cz>
> > Cc: linux-integrity@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> I like that changes obviously but in general, does IMA depend on being
> called _after_ all other LSMs or is this just a historical artifact?

Calculating the EVM HMAC must be last, after the other security xattrs
have been updated.
diff mbox series

Patch

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3c641cc65270..6dc5143f89f2 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -135,9 +135,6 @@  static inline void ima_post_key_create_or_update(struct key *keyring,
 extern bool is_ima_appraise_enabled(void);
 extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 				   struct dentry *dentry);
-extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
-		       const void *xattr_value, size_t xattr_value_len);
-extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
 #else
 static inline bool is_ima_appraise_enabled(void)
 {
@@ -150,19 +147,6 @@  static inline void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 	return;
 }
 
-static inline int ima_inode_setxattr(struct dentry *dentry,
-				     const char *xattr_name,
-				     const void *xattr_value,
-				     size_t xattr_value_len)
-{
-	return 0;
-}
-
-static inline int ima_inode_removexattr(struct dentry *dentry,
-					const char *xattr_name)
-{
-	return 0;
-}
 #endif /* CONFIG_IMA_APPRAISE */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index be965a8715e4..15a369df4c00 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -168,6 +168,16 @@  int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
 
+/* LSM hooks */
+#ifdef CONFIG_IMA_APPRAISE
+int ima_inode_setxattr(struct user_namespace *mnt_userns,
+		       struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len,
+		       int flags);
+int ima_inode_removexattr(struct user_namespace *mnt_userns,
+			  struct dentry *dentry, const char *xattr_name);
+#endif
+
 /*
  * used to protect h_table and sha_table
  */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index bde74fcecee3..ddd9df6b7dac 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -744,8 +744,10 @@  static int validate_hash_algo(struct dentry *dentry,
 	return -EACCES;
 }
 
-int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
-		       const void *xattr_value, size_t xattr_value_len)
+int ima_inode_setxattr(struct user_namespace *mnt_userns,
+		       struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len,
+		       int flags)
 {
 	const struct evm_ima_xattr_data *xvalue = xattr_value;
 	int digsig = 0;
@@ -754,6 +756,11 @@  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
 				   xattr_value_len);
 	if (result == 1) {
+		result = cap_inode_setxattr(dentry, xattr_name, xattr_value,
+					    xattr_value_len, flags);
+		if (result)
+			return result;
+
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
 		digsig = (xvalue->type == EVM_IMA_XATTR_DIGSIG);
@@ -770,11 +777,17 @@  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	return result;
 }
 
-int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+int ima_inode_removexattr(struct user_namespace *mnt_userns,
+			  struct dentry *dentry, const char *xattr_name)
 {
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
+	if (result == 1) {
+		result = cap_inode_removexattr(mnt_userns, dentry, xattr_name);
+		if (result)
+			return result;
+	}
 	if (result == 1 || evm_revalidate_status(xattr_name)) {
 		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
 		if (result == 1)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2cff001b02e4..b3b79d030a67 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1089,6 +1089,10 @@  static struct security_hook_list ima_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
 	LSM_HOOK_INIT(kernel_load_data, ima_load_data),
 	LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
+#ifdef CONFIG_IMA_APPRAISE
+	LSM_HOOK_INIT(inode_setxattr, ima_inode_setxattr),
+	LSM_HOOK_INIT(inode_removexattr, ima_inode_removexattr),
+#endif
 };
 
 void __init integrity_lsm_ima_init(void)
diff --git a/security/security.c b/security/security.c
index 8f7c1b5fa5fa..ca731132a0e9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1349,7 +1349,7 @@  int security_inode_setxattr(struct user_namespace *mnt_userns,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 	/*
-	 * SELinux and Smack integrate the cap call,
+	 * SELinux, Smack, and IMA integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
 	ret = call_int_hook(inode_setxattr, 1, mnt_userns, dentry, name, value,
@@ -1357,9 +1357,6 @@  int security_inode_setxattr(struct user_namespace *mnt_userns,
 
 	if (ret == 1)
 		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
-	ret = ima_inode_setxattr(dentry, name, value, size);
 	if (ret)
 		return ret;
 	return evm_inode_setxattr(mnt_userns, dentry, name, value, size);
@@ -1396,15 +1393,12 @@  int security_inode_removexattr(struct user_namespace *mnt_userns,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 	/*
-	 * SELinux and Smack integrate the cap call,
+	 * SELinux, Smack, and IMA integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
 	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name);
 	if (ret == 1)
 		ret = cap_inode_removexattr(mnt_userns, dentry, name);
-	if (ret)
-		return ret;
-	ret = ima_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
 	return evm_inode_removexattr(mnt_userns, dentry, name);