Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
@ 2020-04-29  7:39 Roberto Sassu
  2020-04-29  7:39 ` [RFC][PATCH 2/3] evm: Extend API of post hooks to pass the result of pre hooks Roberto Sassu
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Roberto Sassu @ 2020-04-29  7:39 UTC (permalink / raw)
  To: zohar, david.safford, viro, jmorris
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, Roberto Sassu

EVM is a module for the protection of the integrity of file metadata. It
protects security-relevant extended attributes, and some file attributes
such as the UID and the GID. It protects their integrity with an HMAC or
with a signature.

What makes EVM different from other LSMs is that it makes a security
decision depending on multiple pieces of information, which cannot be
managed atomically by the system.

Example: cp -a file.orig file.dest

If security.selinux, security.ima and security.evm must be preserved, cp
will invoke setxattr() for each xattr, and EVM performs a verification
during each operation. The problem is that copying security.evm from
file.orig to file.dest will likely break the following EVM verifications if
some metadata still have to be copied. EVM has no visibility on the
metadata of the source file, so it cannot determine when the copy can be
considered complete.

On the other hand, EVM has to check metadata during every operation to
ensure that there is no transition from corrupted metadata, e.g. after an
offline attack, to valid ones after the operation. An HMAC update would
prevent the corruption to be detected, as the HMAC on the new values would
be correct. Thus, to avoid this issue, EVM has to return an error to the
system call so that its execution will be interrupted.

A solution that would satisfy both requirements, not breaking user space
applications and detecting corrupted metadata is to let metadata operations
be completed successfully and to pass the result of the EVM verification
from the pre hooks to the post hooks. In this way, the HMAC update can be
avoided if the verification wasn't successful.

This approach will bring another important benefit: it is no longer
required that every file has a valid HMAC or signature. Instead of always
enforcing metadata integrity, even when it is not relevant for IMA, EVM
will let IMA decide for files selected with the appraisal policy,
depending on the result of the requested verification.

The main problem is that the result of the verification currently cannot be
passed from the pre hooks to the post hooks, due to how the LSM API is
defined. A possible solution would be to use integrity_iint_cache for this
purpose, but it will increase the memory pressure, as new structures will
be allocated also for metadata operations, not only for measurement,
appraisal and audit. Another solution would be to extend the LSM API, but
it seems not worthwhile as EVM would be the only module getting a benefit
from this change.

Given that pre and post hooks are called from the same system call, a more
efficient solution seems to move the hooks outside the LSM infrastructure,
so that the return value of the pre hooks can be passed to the post hooks.
A predefined error (-EAGAIN) will be used to signal to the system call to
continue the execution. Otherwise, if the pre hooks return -EPERM, the
system calls will behave as before and will immediately return before
metadata are changed.

Overview of the changes:

evm_inode_init_security()	LSM (no change)
evm_inode_setxattr()		LSM -> vfs_setxattr()
evm_inode_post_setxattr()	LSM -> vfs_setxattr()
evm_inode_removexattr()		LSM -> vfs_removexattr()
evm_inode_post_removexattr()	vfs_removexattr() (no change)
evm_inode_setattr()		LSM -> vfs_setattr()
evm_inode_post_setattr()	vfs_setattr() (no change)
evm_verifyxattr()		outside LSM (no change)

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/attr.c           |  5 ++++-
 fs/xattr.c          | 17 +++++++++++++++--
 security/security.c | 18 +++---------------
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index b4bbdbd4c8ca..8f26d7d2e3b4 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 {
 	struct inode *inode = dentry->d_inode;
 	umode_t mode = inode->i_mode;
-	int error;
+	int error, evm_error;
 	struct timespec64 now;
 	unsigned int ia_valid = attr->ia_valid;
 
@@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	error = security_inode_setattr(dentry, attr);
 	if (error)
 		return error;
+	evm_error = evm_inode_setattr(dentry, attr);
+	if (evm_error)
+		return evm_error;
 	error = try_break_deleg(inode, delegated_inode);
 	if (error)
 		return error;
diff --git a/fs/xattr.c b/fs/xattr.c
index e13265e65871..3b323b75b741 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 			fsnotify_xattr(dentry);
 			security_inode_post_setxattr(dentry, name, value,
 						     size, flags);
+			evm_inode_post_setxattr(dentry, name, value, size);
 		}
 	} else {
 		if (unlikely(is_bad_inode(inode)))
@@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
+	int error, evm_error;
 
 	error = xattr_permission(inode, name, MAY_WRITE);
 	if (error)
@@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 	if (error)
 		goto out;
 
+	evm_error = evm_inode_setxattr(dentry, name, value, size);
+	if (evm_error) {
+		error = evm_error;
+		goto out;
+	}
+
 	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
 
 out:
@@ -382,7 +389,7 @@ int
 vfs_removexattr(struct dentry *dentry, const char *name)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
+	int error, evm_error;
 
 	error = xattr_permission(inode, name, MAY_WRITE);
 	if (error)
@@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char *name)
 	if (error)
 		goto out;
 
+	evm_error = evm_inode_removexattr(dentry, name);
+	if (evm_error) {
+		error = evm_error;
+		goto out;
+	}
+
 	error = __vfs_removexattr(dentry, name);
 
 	if (!error) {
diff --git a/security/security.c b/security/security.c
index 7fed24b9d57e..e1368ab34cee 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode *inode, int mask)
 
 int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
 {
-	int ret;
-
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	ret = call_int_hook(inode_setattr, 0, dentry, attr);
-	if (ret)
-		return ret;
-	return evm_inode_setattr(dentry, attr);
+	return call_int_hook(inode_setattr, 0, dentry, attr);
 }
 EXPORT_SYMBOL_GPL(security_inode_setattr);
 
@@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry, const char *name,
 		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(dentry, name, value, size);
+	return ima_inode_setxattr(dentry, name, value, size);
 }
 
 void security_inode_post_setxattr(struct dentry *dentry, const char *name,
@@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return;
 	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
-	evm_inode_post_setxattr(dentry, name, value, size);
 }
 
 int security_inode_getxattr(struct dentry *dentry, const char *name)
@@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
 		ret = cap_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
-	ret = ima_inode_removexattr(dentry, name);
-	if (ret)
-		return ret;
-	return evm_inode_removexattr(dentry, name);
+	return ima_inode_removexattr(dentry, name);
 }
 
 int security_inode_need_killpriv(struct dentry *dentry)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC][PATCH 2/3] evm: Extend API of post hooks to pass the result of pre hooks
  2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
@ 2020-04-29  7:39 ` Roberto Sassu
  2020-04-29  7:39 ` [RFC][PATCH 3/3] evm: Return -EAGAIN to ignore verification failures Roberto Sassu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Roberto Sassu @ 2020-04-29  7:39 UTC (permalink / raw)
  To: zohar, david.safford, viro, jmorris
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, Roberto Sassu

This patch extends the API of post hooks to pass the result returned by the
pre hooks. Given that now this information is available, post hooks can
stop before updating the HMAC if the result of the pre hook is not zero.

They still reset the result of the last verification, stored in the
integrity_iint_cache, to ensure that file metadata are re-evaluated after
update.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/attr.c                         |  2 +-
 fs/xattr.c                        | 49 ++++++++++++++++++-------------
 include/linux/evm.h               | 18 ++++++++----
 security/integrity/evm/evm_main.c | 21 +++++++++++--
 4 files changed, 59 insertions(+), 31 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 8f26d7d2e3b4..6ce60e1eba34 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -343,7 +343,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	if (!error) {
 		fsnotify_change(dentry, ia_valid);
 		ima_inode_post_setattr(dentry);
-		evm_inode_post_setattr(dentry, ia_valid);
+		evm_inode_post_setattr(dentry, ia_valid, evm_error);
 	}
 
 	return error;
diff --git a/fs/xattr.c b/fs/xattr.c
index 3b323b75b741..b1fd2aa481a8 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -151,24 +151,8 @@ __vfs_setxattr(struct dentry *dentry, struct inode *inode, const char *name,
 }
 EXPORT_SYMBOL(__vfs_setxattr);
 
-/**
- *  __vfs_setxattr_noperm - perform setxattr operation without performing
- *  permission checks.
- *
- *  @dentry - object to perform setxattr on
- *  @name - xattr name to set
- *  @value - value to set @name to
- *  @size - size of @value
- *  @flags - flags to pass into filesystem operations
- *
- *  returns the result of the internal setxattr or setsecurity operations.
- *
- *  This function requires the caller to lock the inode's i_mutex before it
- *  is executed. It also assumes that the caller will make the appropriate
- *  permission checks.
- */
-int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
-		const void *value, size_t size, int flags)
+static int __vfs_setxattr_noperm_evm(struct dentry *dentry, const char *name,
+		const void *value, size_t size, int flags, int evm_error)
 {
 	struct inode *inode = dentry->d_inode;
 	int error = -EAGAIN;
@@ -183,7 +167,8 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 			fsnotify_xattr(dentry);
 			security_inode_post_setxattr(dentry, name, value,
 						     size, flags);
-			evm_inode_post_setxattr(dentry, name, value, size);
+			evm_inode_post_setxattr(dentry, name, value, size,
+						evm_error);
 		}
 	} else {
 		if (unlikely(is_bad_inode(inode)))
@@ -205,6 +190,27 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 	return error;
 }
 
+/**
+ *  __vfs_setxattr_noperm - perform setxattr operation without performing
+ *  permission checks.
+ *
+ *  @dentry - object to perform setxattr on
+ *  @name - xattr name to set
+ *  @value - value to set @name to
+ *  @size - size of @value
+ *  @flags - flags to pass into filesystem operations
+ *
+ *  returns the result of the internal setxattr or setsecurity operations.
+ *
+ *  This function requires the caller to lock the inode's i_mutex before it
+ *  is executed. It also assumes that the caller will make the appropriate
+ *  permission checks.
+ */
+int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __vfs_setxattr_noperm_evm(dentry, name, value, size, flags, 0);
+}
 
 int
 vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
@@ -228,7 +234,8 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		goto out;
 	}
 
-	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
+	error = __vfs_setxattr_noperm_evm(dentry, name, value, size, flags,
+					  evm_error);
 
 out:
 	inode_unlock(inode);
@@ -410,7 +417,7 @@ vfs_removexattr(struct dentry *dentry, const char *name)
 
 	if (!error) {
 		fsnotify_xattr(dentry);
-		evm_inode_post_removexattr(dentry, name);
+		evm_inode_post_removexattr(dentry, name, evm_error);
 	}
 
 out:
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8302bc29bb35..cc23d2248d3e 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -22,16 +22,19 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     size_t xattr_value_len,
 					     struct integrity_iint_cache *iint);
 extern int evm_inode_setattr(struct dentry *dentry, struct iattr *attr);
-extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
+extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid,
+				   int evm_pre_error);
 extern int evm_inode_setxattr(struct dentry *dentry, const char *name,
 			      const void *value, size_t size);
 extern void evm_inode_post_setxattr(struct dentry *dentry,
 				    const char *xattr_name,
 				    const void *xattr_value,
-				    size_t xattr_value_len);
+				    size_t xattr_value_len,
+				    int evm_pre_error);
 extern int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
-				       const char *xattr_name);
+				       const char *xattr_name,
+				       int evm_pre_error);
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -66,7 +69,8 @@ static inline int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	return 0;
 }
 
-static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid,
+					  int evm_pre_error)
 {
 	return;
 }
@@ -80,7 +84,8 @@ static inline int evm_inode_setxattr(struct dentry *dentry, const char *name,
 static inline void evm_inode_post_setxattr(struct dentry *dentry,
 					   const char *xattr_name,
 					   const void *xattr_value,
-					   size_t xattr_value_len)
+					   size_t xattr_value_len,
+					   int evm_pre_error)
 {
 	return;
 }
@@ -92,7 +97,8 @@ static inline int evm_inode_removexattr(struct dentry *dentry,
 }
 
 static inline void evm_inode_post_removexattr(struct dentry *dentry,
-					      const char *xattr_name)
+					      const char *xattr_name,
+					      int evm_pre_error)
 {
 	return;
 }
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index d361d7fdafc4..ca9eaef7058b 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -422,6 +422,7 @@ static void evm_reset_status(struct inode *inode)
  * @xattr_name: pointer to the affected extended attribute name
  * @xattr_value: pointer to the new extended attribute value
  * @xattr_value_len: pointer to the new extended attribute value length
+ * @evm_pre_error: error returned by evm_inode_setxattr()
  *
  * Update the HMAC stored in 'security.evm' to reflect the change.
  *
@@ -430,7 +431,8 @@ static void evm_reset_status(struct inode *inode)
  * i_mutex lock.
  */
 void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
-			     const void *xattr_value, size_t xattr_value_len)
+			     const void *xattr_value, size_t xattr_value_len,
+			     int evm_pre_error)
 {
 	if (!evm_key_loaded() || (!evm_protected_xattr(xattr_name)
 				  && !posix_xattr_acl(xattr_name)))
@@ -438,6 +440,9 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
 
 	evm_reset_status(dentry->d_inode);
 
+	if (evm_pre_error)
+		return;
+
 	evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
 }
 
@@ -445,19 +450,24 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
  * evm_inode_post_removexattr - update 'security.evm' after removing the xattr
  * @dentry: pointer to the affected dentry
  * @xattr_name: pointer to the affected extended attribute name
+ * @evm_pre_error: error returned by evm_inode_removexattr()
  *
  * Update the HMAC stored in 'security.evm' to reflect removal of the xattr.
  *
  * No need to take the i_mutex lock here, as this function is called from
  * vfs_removexattr() which takes the i_mutex.
  */
-void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
+void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name,
+				int evm_pre_error)
 {
 	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
 		return;
 
 	evm_reset_status(dentry->d_inode);
 
+	if (evm_pre_error)
+		return;
+
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
 
@@ -495,6 +505,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
  * evm_inode_post_setattr - update 'security.evm' after modifying metadata
  * @dentry: pointer to the affected dentry
  * @ia_valid: for the UID and GID status
+ * @evm_pre_error: error returned by evm_inode_setattr()
  *
  * For now, update the HMAC stored in 'security.evm' to reflect UID/GID
  * changes.
@@ -502,11 +513,15 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
  * This function is called from notify_change(), which expects the caller
  * to lock the inode's i_mutex.
  */
-void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
+void evm_inode_post_setattr(struct dentry *dentry, int ia_valid,
+			    int evm_pre_error)
 {
 	if (!evm_key_loaded())
 		return;
 
+	if (evm_pre_error)
+		return;
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC][PATCH 3/3] evm: Return -EAGAIN to ignore verification failures
  2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
  2020-04-29  7:39 ` [RFC][PATCH 2/3] evm: Extend API of post hooks to pass the result of pre hooks Roberto Sassu
@ 2020-04-29  7:39 ` Roberto Sassu
  2020-04-30 16:11 ` [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Lev R. Oshvang .
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Roberto Sassu @ 2020-04-29  7:39 UTC (permalink / raw)
  To: zohar, david.safford, viro, jmorris
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, Roberto Sassu

By default, EVM maintains the same behavior as before hooks were moved
outside the LSM infrastructure. When EVM returns -EPERM, callers stop their
execution and return the error to user space.

This patch introduces a new mode, called ignore, that changes the return
value of the pre hooks from -EPERM to -EAGAIN. It also modifies the callers
of pre and post hooks to continue the execution if -EAGAIN is returned. The
error is then handled by the post hooks.

The only error that is not ignored is when user space is trying to modify a
portable signature. Once that signature has been validated with the current
values of metadata, there is no valid reason to change them.

From user space perspective, operations on corrupted metadata are
successfully performed but post hooks didn't update the HMAC. At the next
IMA verification, when evm_verifyxattr() is called, corruption will be
detected and access will be denied.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/attr.c                         |  2 +-
 fs/xattr.c                        |  4 ++--
 security/integrity/evm/evm_main.c | 23 +++++++++++++++++------
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 6ce60e1eba34..6370e2f3704d 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -329,7 +329,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	if (error)
 		return error;
 	evm_error = evm_inode_setattr(dentry, attr);
-	if (evm_error)
+	if (evm_error && evm_error != -EAGAIN)
 		return evm_error;
 	error = try_break_deleg(inode, delegated_inode);
 	if (error)
diff --git a/fs/xattr.c b/fs/xattr.c
index b1fd2aa481a8..73f0f3cd6c45 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -229,7 +229,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		goto out;
 
 	evm_error = evm_inode_setxattr(dentry, name, value, size);
-	if (evm_error) {
+	if (evm_error && evm_error != -EAGAIN) {
 		error = evm_error;
 		goto out;
 	}
@@ -408,7 +408,7 @@ vfs_removexattr(struct dentry *dentry, const char *name)
 		goto out;
 
 	evm_error = evm_inode_removexattr(dentry, name);
-	if (evm_error) {
+	if (evm_error && evm_error != -EAGAIN) {
 		error = evm_error;
 		goto out;
 	}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index ca9eaef7058b..ef09caa3bbcf 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -54,11 +54,13 @@ static struct xattr_list evm_config_default_xattrnames[] = {
 
 LIST_HEAD(evm_config_xattrnames);
 
-static int evm_fixmode;
+static int evm_fixmode, evm_ignoremode __ro_after_init;
 static int __init evm_set_fixmode(char *str)
 {
 	if (strncmp(str, "fix", 3) == 0)
 		evm_fixmode = 1;
+	if (strncmp(str, "ignore", 6) == 0)
+		evm_ignoremode = 1;
 	return 0;
 }
 __setup("evm=", evm_set_fixmode);
@@ -311,6 +313,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 			     const void *xattr_value, size_t xattr_value_len)
 {
 	enum integrity_status evm_status;
+	int rc = -EPERM;
 
 	if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
 		if (!capable(CAP_SYS_ADMIN))
@@ -345,12 +348,17 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 				    -EPERM, 0);
 	}
 out:
-	if (evm_status != INTEGRITY_PASS)
+	if (evm_status != INTEGRITY_PASS) {
+		if (evm_ignoremode && evm_status != INTEGRITY_PASS_IMMUTABLE)
+			rc = -EAGAIN;
+
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
 				    integrity_status_msg[evm_status],
-				    -EPERM, 0);
-	return evm_status == INTEGRITY_PASS ? 0 : -EPERM;
+				    rc, 0);
+	}
+
+	return evm_status == INTEGRITY_PASS ? 0 : rc;
 }
 
 /**
@@ -482,6 +490,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	unsigned int ia_valid = attr->ia_valid;
 	enum integrity_status evm_status;
+	int rc = -EPERM;
 
 	/* Policy permits modification of the protected attrs even though
 	 * there's no HMAC key loaded
@@ -495,10 +504,12 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	if ((evm_status == INTEGRITY_PASS) ||
 	    (evm_status == INTEGRITY_NOXATTRS))
 		return 0;
+	if (evm_ignoremode && evm_status != INTEGRITY_PASS_IMMUTABLE)
+		rc = -EAGAIN;
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
-			    integrity_status_msg[evm_status], -EPERM, 0);
-	return -EPERM;
+			    integrity_status_msg[evm_status], rc, 0);
+	return rc;
 }
 
 /**
-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
  2020-04-29  7:39 ` [RFC][PATCH 2/3] evm: Extend API of post hooks to pass the result of pre hooks Roberto Sassu
  2020-04-29  7:39 ` [RFC][PATCH 3/3] evm: Return -EAGAIN to ignore verification failures Roberto Sassu
@ 2020-04-30 16:11 ` Lev R. Oshvang .
  2020-05-01  6:56   ` Roberto Sassu
  2020-05-06 16:11 ` Roberto Sassu
  2020-05-06 19:44 ` Mimi Zohar
  4 siblings, 1 reply; 24+ messages in thread
From: Lev R. Oshvang . @ 2020-04-30 16:11 UTC (permalink / raw)
  To: Roberto Sassu, linux-integrity

On Wed, Apr 29, 2020 at 10:45 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> EVM is a module for the protection of the integrity of file metadata. It
> protects security-relevant extended attributes, and some file attributes
> such as the UID and the GID. It protects their integrity with an HMAC or
> with a signature.
>
> What makes EVM different from other LSMs is that it makes a security
> decision depending on multiple pieces of information, which cannot be
> managed atomically by the system.
>
> Example: cp -a file.orig file.dest
>
> If security.selinux, security.ima and security.evm must be preserved, cp
> will invoke setxattr() for each xattr, and EVM performs a verification
> during each operation. The problem is that copying security.evm from
> file.orig to file.dest will likely break the following EVM verifications if
> some metadata still have to be copied. EVM has no visibility on the
> metadata of the source file, so it cannot determine when the copy can be
> considered complete.
>
> On the other hand, EVM has to check metadata during every operation to
> ensure that there is no transition from corrupted metadata, e.g. after an
> offline attack, to valid ones after the operation. An HMAC update would
> prevent the corruption to be detected, as the HMAC on the new values would
> be correct. Thus, to avoid this issue, EVM has to return an error to the
> system call so that its execution will be interrupted.
>
> A solution that would satisfy both requirements, not breaking user space
> applications and detecting corrupted metadata is to let metadata operations
> be completed successfully and to pass the result of the EVM verification
> from the pre hooks to the post hooks. In this way, the HMAC update can be
> avoided if the verification wasn't successful.
>
> This approach will bring another important benefit: it is no longer
> required that every file has a valid HMAC or signature. Instead of always
> enforcing metadata integrity, even when it is not relevant for IMA, EVM
> will let IMA decide for files selected with the appraisal policy,
> depending on the result of the requested verification.
>
> The main problem is that the result of the verification currently cannot be
> passed from the pre hooks to the post hooks, due to how the LSM API is
> defined. A possible solution would be to use integrity_iint_cache for this
> purpose, but it will increase the memory pressure, as new structures will
> be allocated also for metadata operations, not only for measurement,
> appraisal and audit. Another solution would be to extend the LSM API, but
> it seems not worthwhile as EVM would be the only module getting a benefit
> from this change.
>
> Given that pre and post hooks are called from the same system call, a more
> efficient solution seems to move the hooks outside the LSM infrastructure,
> so that the return value of the pre hooks can be passed to the post hooks.
> A predefined error (-EAGAIN) will be used to signal to the system call to
> continue the execution. Otherwise, if the pre hooks return -EPERM, the
> system calls will behave as before and will immediately return before
> metadata are changed.
>
> Overview of the changes:
>
> evm_inode_init_security()       LSM (no change)
> evm_inode_setxattr()            LSM -> vfs_setxattr()
> evm_inode_post_setxattr()       LSM -> vfs_setxattr()
> evm_inode_removexattr()         LSM -> vfs_removexattr()
> evm_inode_post_removexattr()    vfs_removexattr() (no change)
> evm_inode_setattr()             LSM -> vfs_setattr()
> evm_inode_post_setattr()        vfs_setattr() (no change)
> evm_verifyxattr()               outside LSM (no change)
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/attr.c           |  5 ++++-
>  fs/xattr.c          | 17 +++++++++++++++--
>  security/security.c | 18 +++---------------
>  3 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index b4bbdbd4c8ca..8f26d7d2e3b4 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
>  {
>         struct inode *inode = dentry->d_inode;
>         umode_t mode = inode->i_mode;
> -       int error;
> +       int error, evm_error;
>         struct timespec64 now;
>         unsigned int ia_valid = attr->ia_valid;
>
> @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
>         error = security_inode_setattr(dentry, attr);
>         if (error)
>                 return error;
> +       evm_error = evm_inode_setattr(dentry, attr);
> +       if (evm_error)
> +               return evm_error;
>         error = try_break_deleg(inode, delegated_inode);
>         if (error)
>                 return error;
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e13265e65871..3b323b75b741 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>                         fsnotify_xattr(dentry);
>                         security_inode_post_setxattr(dentry, name, value,
>                                                      size, flags);
> +                       evm_inode_post_setxattr(dentry, name, value, size);
>                 }
>         } else {
>                 if (unlikely(is_bad_inode(inode)))
> @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>                 size_t size, int flags)
>  {
>         struct inode *inode = dentry->d_inode;
> -       int error;
> +       int error, evm_error;
>
>         error = xattr_permission(inode, name, MAY_WRITE);
>         if (error)
> @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
>         if (error)
>                 goto out;
>
> +       evm_error = evm_inode_setxattr(dentry, name, value, size);
> +       if (evm_error) {
> +               error = evm_error;
> +               goto out;
> +       }
> +
>         error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
>
>  out:
> @@ -382,7 +389,7 @@ int
>  vfs_removexattr(struct dentry *dentry, const char *name)
>  {
>         struct inode *inode = dentry->d_inode;
> -       int error;
> +       int error, evm_error;
>
>         error = xattr_permission(inode, name, MAY_WRITE);
>         if (error)
> @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char *name)
>         if (error)
>                 goto out;
>
> +       evm_error = evm_inode_removexattr(dentry, name);
> +       if (evm_error) {
> +               error = evm_error;
> +               goto out;
> +       }
> +
>         error = __vfs_removexattr(dentry, name);
>
>         if (!error) {
> diff --git a/security/security.c b/security/security.c
> index 7fed24b9d57e..e1368ab34cee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode *inode, int mask)
>
>  int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
>  {
> -       int ret;
> -
>         if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>                 return 0;
> -       ret = call_int_hook(inode_setattr, 0, dentry, attr);
> -       if (ret)
> -               return ret;
> -       return evm_inode_setattr(dentry, attr);
> +       return call_int_hook(inode_setattr, 0, dentry, attr);
>  }
>  EXPORT_SYMBOL_GPL(security_inode_setattr);
>
> @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry, const char *name,
>                 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(dentry, name, value, size);
> +       return ima_inode_setxattr(dentry, name, value, size);
>  }
>
>  void security_inode_post_setxattr(struct dentry *dentry, const char *name,
> @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
>         if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>                 return;
>         call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> -       evm_inode_post_setxattr(dentry, name, value, size);
>  }
>
>  int security_inode_getxattr(struct dentry *dentry, const char *name)
> @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
>                 ret = cap_inode_removexattr(dentry, name);
>         if (ret)
>                 return ret;
> -       ret = ima_inode_removexattr(dentry, name);
> -       if (ret)
> -               return ret;
> -       return evm_inode_removexattr(dentry, name);
> +       return ima_inode_removexattr(dentry, name);
>  }
>
>  int security_inode_need_killpriv(struct dentry *dentry)
> --
> 2.17.1
>

Hi Roberto,

I apologize that due to my relatively small experience I may not
understand completely your patch.
Please be patient.
To my understanding, EVM knows(configuration) which security
attributes must be taken into HMAC, security.selinux, security.smack,
system.acl, security.ima, etc.
So until HMAC calculation should fail until cp command will not finish
copying all security attributes.
Is it the desired behavior of HMAC?
If the number of security attributes varies between files under EVM,
can we just remove
evm_inode_setxattr() from secutity_inode_setxattr() because it is the
job of evmctl utility to compute security.hmac value?

Actually I do not see the use case why HMAC should be computed inline.
I come from embedded word and will never modify extended attributes
because the signing key will never be placed on the target device. I
think this argument stands also for production servers and for package
management as well.

Regards,
Lev

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-04-30 16:11 ` [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Lev R. Oshvang .
@ 2020-05-01  6:56   ` Roberto Sassu
  0 siblings, 0 replies; 24+ messages in thread
From: Roberto Sassu @ 2020-05-01  6:56 UTC (permalink / raw)
  To: Lev R. Oshvang ., linux-integrity, Mimi Zohar, Silviu Vlasceanu

> -----Original Message-----
> From: Lev R. Oshvang . [mailto:levonshe@gmail.com]
> Sent: Thursday, April 30, 2020 6:12 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>; linux-
> integrity@vger.kernel.org
> Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
> 
> On Wed, Apr 29, 2020 at 10:45 AM Roberto Sassu
> <roberto.sassu@huawei.com> wrote:
> >
> > EVM is a module for the protection of the integrity of file metadata. It
> > protects security-relevant extended attributes, and some file attributes
> > such as the UID and the GID. It protects their integrity with an HMAC or
> > with a signature.
> >
> > What makes EVM different from other LSMs is that it makes a security
> > decision depending on multiple pieces of information, which cannot be
> > managed atomically by the system.
> >
> > Example: cp -a file.orig file.dest
> >
> > If security.selinux, security.ima and security.evm must be preserved, cp
> > will invoke setxattr() for each xattr, and EVM performs a verification
> > during each operation. The problem is that copying security.evm from
> > file.orig to file.dest will likely break the following EVM verifications if
> > some metadata still have to be copied. EVM has no visibility on the
> > metadata of the source file, so it cannot determine when the copy can be
> > considered complete.
> >
> > On the other hand, EVM has to check metadata during every operation to
> > ensure that there is no transition from corrupted metadata, e.g. after an
> > offline attack, to valid ones after the operation. An HMAC update would
> > prevent the corruption to be detected, as the HMAC on the new values
> would
> > be correct. Thus, to avoid this issue, EVM has to return an error to the
> > system call so that its execution will be interrupted.
> >
> > A solution that would satisfy both requirements, not breaking user space
> > applications and detecting corrupted metadata is to let metadata
> operations
> > be completed successfully and to pass the result of the EVM verification
> > from the pre hooks to the post hooks. In this way, the HMAC update can
> be
> > avoided if the verification wasn't successful.
> >
> > This approach will bring another important benefit: it is no longer
> > required that every file has a valid HMAC or signature. Instead of always
> > enforcing metadata integrity, even when it is not relevant for IMA, EVM
> > will let IMA decide for files selected with the appraisal policy,
> > depending on the result of the requested verification.
> >
> > The main problem is that the result of the verification currently cannot be
> > passed from the pre hooks to the post hooks, due to how the LSM API is
> > defined. A possible solution would be to use integrity_iint_cache for this
> > purpose, but it will increase the memory pressure, as new structures will
> > be allocated also for metadata operations, not only for measurement,
> > appraisal and audit. Another solution would be to extend the LSM API, but
> > it seems not worthwhile as EVM would be the only module getting a
> benefit
> > from this change.
> >
> > Given that pre and post hooks are called from the same system call, a
> more
> > efficient solution seems to move the hooks outside the LSM infrastructure,
> > so that the return value of the pre hooks can be passed to the post hooks.
> > A predefined error (-EAGAIN) will be used to signal to the system call to
> > continue the execution. Otherwise, if the pre hooks return -EPERM, the
> > system calls will behave as before and will immediately return before
> > metadata are changed.
> >
> > Overview of the changes:
> >
> > evm_inode_init_security()       LSM (no change)
> > evm_inode_setxattr()            LSM -> vfs_setxattr()
> > evm_inode_post_setxattr()       LSM -> vfs_setxattr()
> > evm_inode_removexattr()         LSM -> vfs_removexattr()
> > evm_inode_post_removexattr()    vfs_removexattr() (no change)
> > evm_inode_setattr()             LSM -> vfs_setattr()
> > evm_inode_post_setattr()        vfs_setattr() (no change)
> > evm_verifyxattr()               outside LSM (no change)
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  fs/attr.c           |  5 ++++-
> >  fs/xattr.c          | 17 +++++++++++++++--
> >  security/security.c | 18 +++---------------
> >  3 files changed, 22 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index b4bbdbd4c8ca..8f26d7d2e3b4 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct
> iattr * attr, struct inode **de
> >  {
> >         struct inode *inode = dentry->d_inode;
> >         umode_t mode = inode->i_mode;
> > -       int error;
> > +       int error, evm_error;
> >         struct timespec64 now;
> >         unsigned int ia_valid = attr->ia_valid;
> >
> > @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct
> iattr * attr, struct inode **de
> >         error = security_inode_setattr(dentry, attr);
> >         if (error)
> >                 return error;
> > +       evm_error = evm_inode_setattr(dentry, attr);
> > +       if (evm_error)
> > +               return evm_error;
> >         error = try_break_deleg(inode, delegated_inode);
> >         if (error)
> >                 return error;
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index e13265e65871..3b323b75b741 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry,
> const char *name,
> >                         fsnotify_xattr(dentry);
> >                         security_inode_post_setxattr(dentry, name, value,
> >                                                      size, flags);
> > +                       evm_inode_post_setxattr(dentry, name, value, size);
> >                 }
> >         } else {
> >                 if (unlikely(is_bad_inode(inode)))
> > @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
> >                 size_t size, int flags)
> >  {
> >         struct inode *inode = dentry->d_inode;
> > -       int error;
> > +       int error, evm_error;
> >
> >         error = xattr_permission(inode, name, MAY_WRITE);
> >         if (error)
> > @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
> >         if (error)
> >                 goto out;
> >
> > +       evm_error = evm_inode_setxattr(dentry, name, value, size);
> > +       if (evm_error) {
> > +               error = evm_error;
> > +               goto out;
> > +       }
> > +
> >         error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
> >
> >  out:
> > @@ -382,7 +389,7 @@ int
> >  vfs_removexattr(struct dentry *dentry, const char *name)
> >  {
> >         struct inode *inode = dentry->d_inode;
> > -       int error;
> > +       int error, evm_error;
> >
> >         error = xattr_permission(inode, name, MAY_WRITE);
> >         if (error)
> > @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const
> char *name)
> >         if (error)
> >                 goto out;
> >
> > +       evm_error = evm_inode_removexattr(dentry, name);
> > +       if (evm_error) {
> > +               error = evm_error;
> > +               goto out;
> > +       }
> > +
> >         error = __vfs_removexattr(dentry, name);
> >
> >         if (!error) {
> > diff --git a/security/security.c b/security/security.c
> > index 7fed24b9d57e..e1368ab34cee 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode
> *inode, int mask)
> >
> >  int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
> >  {
> > -       int ret;
> > -
> >         if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> >                 return 0;
> > -       ret = call_int_hook(inode_setattr, 0, dentry, attr);
> > -       if (ret)
> > -               return ret;
> > -       return evm_inode_setattr(dentry, attr);
> > +       return call_int_hook(inode_setattr, 0, dentry, attr);
> >  }
> >  EXPORT_SYMBOL_GPL(security_inode_setattr);
> >
> > @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry
> *dentry, const char *name,
> >                 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(dentry, name, value, size);
> > +       return ima_inode_setxattr(dentry, name, value, size);
> >  }
> >
> >  void security_inode_post_setxattr(struct dentry *dentry, const char
> *name,
> > @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry
> *dentry, const char *name,
> >         if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> >                 return;
> >         call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> > -       evm_inode_post_setxattr(dentry, name, value, size);
> >  }
> >
> >  int security_inode_getxattr(struct dentry *dentry, const char *name)
> > @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry
> *dentry, const char *name)
> >                 ret = cap_inode_removexattr(dentry, name);
> >         if (ret)
> >                 return ret;
> > -       ret = ima_inode_removexattr(dentry, name);
> > -       if (ret)
> > -               return ret;
> > -       return evm_inode_removexattr(dentry, name);
> > +       return ima_inode_removexattr(dentry, name);
> >  }
> >
> >  int security_inode_need_killpriv(struct dentry *dentry)
> > --
> > 2.17.1
> >
> 
> Hi Roberto,
> 
> I apologize that due to my relatively small experience I may not
> understand completely your patch.
> Please be patient.
> To my understanding, EVM knows(configuration) which security
> attributes must be taken into HMAC, security.selinux, security.smack,
> system.acl, security.ima, etc.
> So until HMAC calculation should fail until cp command will not finish
> copying all security attributes.
> Is it the desired behavior of HMAC?

Hi Lev

HMAC is not the main focus of this patch. In /etc/xattr.conf there is
a line to ignore security.evm. so cp will copy one xattr at time and the
HMAC will never be invalid.

The problem arises with signatures. In this case you cannot ignore
security.evm and you have to copy it to the destination. But when you
copy it, verification of the destination is not yet possible because other
xattrs are still not copied.

My idea is that it is not necessary to block xattr operations to guarantee
xattr integrity. The same can be achieved by letting the operations be
performed and not updating the HMAC if xattrs are corrupted.

This will ensure much better compatibility with user space.

> If the number of security attributes varies between files under EVM,
> can we just remove
> evm_inode_setxattr() from secutity_inode_setxattr() because it is the
> job of evmctl utility to compute security.hmac value?
> 
> Actually I do not see the use case why HMAC should be computed inline.
> I come from embedded word and will never modify extended attributes
> because the signing key will never be placed on the target device. I
> think this argument stands also for production servers and for package
> management as well.

HMAC is meant to be used to protect mutable files. Since security.ima
changes after every modification, as it includes the digest of the file,
signatures cannot be used.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
                   ` (2 preceding siblings ...)
  2020-04-30 16:11 ` [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Lev R. Oshvang .
@ 2020-05-06 16:11 ` Roberto Sassu
  2020-05-06 19:44 ` Mimi Zohar
  4 siblings, 0 replies; 24+ messages in thread
From: Roberto Sassu @ 2020-05-06 16:11 UTC (permalink / raw)
  To: zohar, david.safford, viro, jmorris
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: Roberto Sassu
> Sent: Wednesday, April 29, 2020 9:40 AM
> To: zohar@linux.ibm.com; david.safford@gmail.com;
> viro@zeniv.linux.org.uk; jmorris@namei.org
> Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux-
> security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu
> Vlasceanu <Silviu.Vlasceanu@huawei.com>; Roberto Sassu
> <roberto.sassu@huawei.com>
> Subject: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure

Any thought on this? The implementation can be discussed later.

I just wanted a feedback on the approach, if this is the right direction
to solve the problem.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


> EVM is a module for the protection of the integrity of file metadata. It
> protects security-relevant extended attributes, and some file attributes
> such as the UID and the GID. It protects their integrity with an HMAC or
> with a signature.
> 
> What makes EVM different from other LSMs is that it makes a security
> decision depending on multiple pieces of information, which cannot be
> managed atomically by the system.
> 
> Example: cp -a file.orig file.dest
> 
> If security.selinux, security.ima and security.evm must be preserved, cp
> will invoke setxattr() for each xattr, and EVM performs a verification
> during each operation. The problem is that copying security.evm from
> file.orig to file.dest will likely break the following EVM verifications if
> some metadata still have to be copied. EVM has no visibility on the
> metadata of the source file, so it cannot determine when the copy can be
> considered complete.
> 
> On the other hand, EVM has to check metadata during every operation to
> ensure that there is no transition from corrupted metadata, e.g. after an
> offline attack, to valid ones after the operation. An HMAC update would
> prevent the corruption to be detected, as the HMAC on the new values
> would
> be correct. Thus, to avoid this issue, EVM has to return an error to the
> system call so that its execution will be interrupted.
> 
> A solution that would satisfy both requirements, not breaking user space
> applications and detecting corrupted metadata is to let metadata operations
> be completed successfully and to pass the result of the EVM verification
> from the pre hooks to the post hooks. In this way, the HMAC update can be
> avoided if the verification wasn't successful.
> 
> This approach will bring another important benefit: it is no longer
> required that every file has a valid HMAC or signature. Instead of always
> enforcing metadata integrity, even when it is not relevant for IMA, EVM
> will let IMA decide for files selected with the appraisal policy,
> depending on the result of the requested verification.
> 
> The main problem is that the result of the verification currently cannot be
> passed from the pre hooks to the post hooks, due to how the LSM API is
> defined. A possible solution would be to use integrity_iint_cache for this
> purpose, but it will increase the memory pressure, as new structures will
> be allocated also for metadata operations, not only for measurement,
> appraisal and audit. Another solution would be to extend the LSM API, but
> it seems not worthwhile as EVM would be the only module getting a benefit
> from this change.
> 
> Given that pre and post hooks are called from the same system call, a more
> efficient solution seems to move the hooks outside the LSM infrastructure,
> so that the return value of the pre hooks can be passed to the post hooks.
> A predefined error (-EAGAIN) will be used to signal to the system call to
> continue the execution. Otherwise, if the pre hooks return -EPERM, the
> system calls will behave as before and will immediately return before
> metadata are changed.
> 
> Overview of the changes:
> 
> evm_inode_init_security()	LSM (no change)
> evm_inode_setxattr()		LSM -> vfs_setxattr()
> evm_inode_post_setxattr()	LSM -> vfs_setxattr()
> evm_inode_removexattr()		LSM -> vfs_removexattr()
> evm_inode_post_removexattr()	vfs_removexattr() (no change)
> evm_inode_setattr()		LSM -> vfs_setattr()
> evm_inode_post_setattr()	vfs_setattr() (no change)
> evm_verifyxattr()		outside LSM (no change)
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/attr.c           |  5 ++++-
>  fs/xattr.c          | 17 +++++++++++++++--
>  security/security.c | 18 +++---------------
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index b4bbdbd4c8ca..8f26d7d2e3b4 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr
> * attr, struct inode **de
>  {
>  	struct inode *inode = dentry->d_inode;
>  	umode_t mode = inode->i_mode;
> -	int error;
> +	int error, evm_error;
>  	struct timespec64 now;
>  	unsigned int ia_valid = attr->ia_valid;
> 
> @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr
> * attr, struct inode **de
>  	error = security_inode_setattr(dentry, attr);
>  	if (error)
>  		return error;
> +	evm_error = evm_inode_setattr(dentry, attr);
> +	if (evm_error)
> +		return evm_error;
>  	error = try_break_deleg(inode, delegated_inode);
>  	if (error)
>  		return error;
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e13265e65871..3b323b75b741 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry,
> const char *name,
>  			fsnotify_xattr(dentry);
>  			security_inode_post_setxattr(dentry, name, value,
>  						     size, flags);
> +			evm_inode_post_setxattr(dentry, name, value,
> size);
>  		}
>  	} else {
>  		if (unlikely(is_bad_inode(inode)))
> @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
>  		size_t size, int flags)
>  {
>  	struct inode *inode = dentry->d_inode;
> -	int error;
> +	int error, evm_error;
> 
>  	error = xattr_permission(inode, name, MAY_WRITE);
>  	if (error)
> @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
>  	if (error)
>  		goto out;
> 
> +	evm_error = evm_inode_setxattr(dentry, name, value, size);
> +	if (evm_error) {
> +		error = evm_error;
> +		goto out;
> +	}
> +
>  	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
> 
>  out:
> @@ -382,7 +389,7 @@ int
>  vfs_removexattr(struct dentry *dentry, const char *name)
>  {
>  	struct inode *inode = dentry->d_inode;
> -	int error;
> +	int error, evm_error;
> 
>  	error = xattr_permission(inode, name, MAY_WRITE);
>  	if (error)
> @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char
> *name)
>  	if (error)
>  		goto out;
> 
> +	evm_error = evm_inode_removexattr(dentry, name);
> +	if (evm_error) {
> +		error = evm_error;
> +		goto out;
> +	}
> +
>  	error = __vfs_removexattr(dentry, name);
> 
>  	if (!error) {
> diff --git a/security/security.c b/security/security.c
> index 7fed24b9d57e..e1368ab34cee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode
> *inode, int mask)
> 
>  int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
>  {
> -	int ret;
> -
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return 0;
> -	ret = call_int_hook(inode_setattr, 0, dentry, attr);
> -	if (ret)
> -		return ret;
> -	return evm_inode_setattr(dentry, attr);
> +	return call_int_hook(inode_setattr, 0, dentry, attr);
>  }
>  EXPORT_SYMBOL_GPL(security_inode_setattr);
> 
> @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry,
> const char *name,
>  		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(dentry, name, value, size);
> +	return ima_inode_setxattr(dentry, name, value, size);
>  }
> 
>  void security_inode_post_setxattr(struct dentry *dentry, const char
> *name,
> @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry
> *dentry, const char *name,
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;
>  	call_void_hook(inode_post_setxattr, dentry, name, value, size,
> flags);
> -	evm_inode_post_setxattr(dentry, name, value, size);
>  }
> 
>  int security_inode_getxattr(struct dentry *dentry, const char *name)
> @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry
> *dentry, const char *name)
>  		ret = cap_inode_removexattr(dentry, name);
>  	if (ret)
>  		return ret;
> -	ret = ima_inode_removexattr(dentry, name);
> -	if (ret)
> -		return ret;
> -	return evm_inode_removexattr(dentry, name);
> +	return ima_inode_removexattr(dentry, name);
>  }
> 
>  int security_inode_need_killpriv(struct dentry *dentry)
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
                   ` (3 preceding siblings ...)
  2020-05-06 16:11 ` Roberto Sassu
@ 2020-05-06 19:44 ` Mimi Zohar
  2020-05-06 21:10   ` Mimi Zohar
  4 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-06 19:44 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu

[Cc: John Johansen] 

On Wed, 2020-04-29 at 09:39 +0200, Roberto Sassu wrote:
> EVM is a module for the protection of the integrity of file metadata. It
> protects security-relevant extended attributes, and some file attributes
> such as the UID and the GID. It protects their integrity with an HMAC or
> with a signature.
> 
> What makes EVM different from other LSMs is that it makes a security
> decision depending on multiple pieces of information, which cannot be
> managed atomically by the system.
> 
> Example: cp -a file.orig file.dest
> 
> If security.selinux, security.ima and security.evm must be preserved, cp
> will invoke setxattr() for each xattr, and EVM performs a verification
> during each operation. The problem is that copying security.evm from
> file.orig to file.dest will likely break the following EVM verifications if
> some metadata still have to be copied. EVM has no visibility on the
> metadata of the source file, so it cannot determine when the copy can be
> considered complete.

I remember having a similar discussion in the past.  At the time,
there wasn't EVM portable and immutable signature support, just the
HMAC and original signature types.  Neither of these EVM xattrs types
should be copied.

Calling evm_verifyxattr() is not limited to IMA, but may be called by
other LSMs/subsystems as well.  At some point, there was some
discussion about AppArmor calling it directly.  Not sure if that is
still being discussed.

Since copying the EVM HMAC or original signature isn't applicable, I
would prefer exploring an EVM portable and immutable signature only
solution.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-06 19:44 ` Mimi Zohar
@ 2020-05-06 21:10   ` Mimi Zohar
  2020-05-07  7:53     ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-06 21:10 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu

On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> Since copying the EVM HMAC or original signature isn't applicable, I
> would prefer exploring an EVM portable and immutable signature only
> solution.

To prevent copying the EVM xattr, we added "security.evm" to
/etc/xattr.conf.  To support copying just the EVM portable and
immutable signatures will require a different solution.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-06 21:10   ` Mimi Zohar
@ 2020-05-07  7:53     ` Roberto Sassu
  2020-05-07 15:17       ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-07  7:53 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, May 6, 2020 11:10 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>; david.safford@gmail.com;
> viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen
> <john.johansen@canonical.com>
> Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux-
> security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu
> Vlasceanu <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
> 
> On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > Since copying the EVM HMAC or original signature isn't applicable, I
> > would prefer exploring an EVM portable and immutable signature only
> > solution.
> 
> To prevent copying the EVM xattr, we added "security.evm" to
> /etc/xattr.conf.  To support copying just the EVM portable and
> immutable signatures will require a different solution.

This patch set removes the need for ignoring security.evm. It can be always
copied, even if it is an HMAC. EVM will update it only when verification in
the pre hook is successful. Combined with the ability of protecting a subset
of files without introducing an EVM policy, these advantages seem to
outweigh the effort necessary to make the switch.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-07  7:53     ` Roberto Sassu
@ 2020-05-07 15:17       ` Mimi Zohar
  2020-05-07 16:47         ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-07 15:17 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Thu, 2020-05-07 at 07:53 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Wednesday, May 6, 2020 11:10 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>; david.safford@gmail.com;
> > viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen
> > <john.johansen@canonical.com>
> > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org; linux-
> > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu
> > Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure

Roberto, please fix your mailer or at least manually remove this sort
of info from the email.

> > 
> > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > > Since copying the EVM HMAC or original signature isn't applicable, I
> > > would prefer exploring an EVM portable and immutable signature only
> > > solution.
> > 
> > To prevent copying the EVM xattr, we added "security.evm" to
> > /etc/xattr.conf.  To support copying just the EVM portable and
> > immutable signatures will require a different solution.
> 
> This patch set removes the need for ignoring security.evm. It can be always
> copied, even if it is an HMAC. EVM will update it only when verification in
> the pre hook is successful. Combined with the ability of protecting a subset
> of files without introducing an EVM policy, these advantages seem to
> outweigh the effort necessary to make the switch.

As the EVM file HMAC and original signature contain inode specific
information (eg. i_version, i_generation), these xattrs cannot ever be
copied.  The proposed change is in order to support just the new EVM
signatures.

At least IMA file hashes should always be used in conjunction with
EVM.  EVM xattrs should always require a security.ima xattr to bind
the file metadata to the file data.  The IMA and EVM policies really
need to be in sync.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-07 15:17       ` Mimi Zohar
@ 2020-05-07 16:47         ` Roberto Sassu
  2020-05-07 20:45           ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-07 16:47 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> On Thu, 2020-05-07 at 07:53 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Wednesday, May 6, 2020 11:10 PM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> david.safford@gmail.com;
> > > viro@zeniv.linux.org.uk; jmorris@namei.org; John Johansen
> > > <john.johansen@canonical.com>
> > > Cc: linux-fsdevel@vger.kernel.org; linux-integrity@vger.kernel.org;
> linux-
> > > security-module@vger.kernel.org; linux-kernel@vger.kernel.org; Silviu
> > > Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM
> infrastructure
> 
> Roberto, please fix your mailer or at least manually remove this sort
> of info from the email.
> 
> > >
> > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > > > Since copying the EVM HMAC or original signature isn't applicable, I
> > > > would prefer exploring an EVM portable and immutable signature only
> > > > solution.
> > >
> > > To prevent copying the EVM xattr, we added "security.evm" to
> > > /etc/xattr.conf.  To support copying just the EVM portable and
> > > immutable signatures will require a different solution.
> >
> > This patch set removes the need for ignoring security.evm. It can be
> always
> > copied, even if it is an HMAC. EVM will update it only when verification in
> > the pre hook is successful. Combined with the ability of protecting a
> subset
> > of files without introducing an EVM policy, these advantages seem to
> > outweigh the effort necessary to make the switch.
> 
> As the EVM file HMAC and original signature contain inode specific
> information (eg. i_version, i_generation), these xattrs cannot ever be
> copied.  The proposed change is in order to support just the new EVM
> signatures.

Right, I didn't consider it.

Would it make sense instead to introduce an alias like security.evm_immutable
so that this xattr can be copied?

> At least IMA file hashes should always be used in conjunction with
> EVM.  EVM xattrs should always require a security.ima xattr to bind

I proposed to enforce this restriction some time ago:

https://patchwork.kernel.org/patch/10979351/

Is it ok to enforce it globally?

> the file metadata to the file data.  The IMA and EVM policies really
> need to be in sync.

It would be nice, but at the moment EVM considers also files that are
not selected by the IMA policy. An example of why this is a problem is
the audit service that fails to start when it tries to adjust the permissions
of the log files. Those files don't have security.evm because they are
not appraised by IMA, but EVM denies the operation.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-07 16:47         ` Roberto Sassu
@ 2020-05-07 20:45           ` Mimi Zohar
  2020-05-08 10:20             ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-07 20:45 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > > > > Since copying the EVM HMAC or original signature isn't applicable, I
> > > > > would prefer exploring an EVM portable and immutable signature only
> > > > > solution.
> > > >
> > > > To prevent copying the EVM xattr, we added "security.evm" to
> > > > /etc/xattr.conf.  To support copying just the EVM portable and
> > > > immutable signatures will require a different solution.
> > >
> > > This patch set removes the need for ignoring security.evm. It can be
> > always
> > > copied, even if it is an HMAC. EVM will update it only when verification in
> > > the pre hook is successful. Combined with the ability of protecting a
> > subset
> > > of files without introducing an EVM policy, these advantages seem to
> > > outweigh the effort necessary to make the switch.
> > 
> > As the EVM file HMAC and original signature contain inode specific
> > information (eg. i_version, i_generation), these xattrs cannot ever be
> > copied.  The proposed change is in order to support just the new EVM
> > signatures.
> 
> Right, I didn't consider it.
> 
> Would it make sense instead to introduce an alias like security.evm_immutable
> so that this xattr can be copied?

Being portable, not the attribute of being immutable, allows copying
the EVM xattr.  Your original problem - the order in which the xattrs
are copied - might still be an issue.  We need to look at "cp" closer
to understand what it is doing.  For example, are the xattrs written
while the target file is tagged as a new file?

There have been similar problems in the past.  For example, tar calls
mknodat to create the file, but doesn't write the file data.  The
solution there was to tag the file as a new file.

We need to understand the problem better, before deciding how to
resolve it.

> 
> > At least IMA file hashes should always be used in conjunction with
> > EVM.  EVM xattrs should always require a security.ima xattr to bind
> 
> I proposed to enforce this restriction some time ago:
> 
> https://patchwork.kernel.org/patch/10979351/
> 
> Is it ok to enforce it globally?

Doing this would then be dependent on upstreaming the initramfs xattr
patches first, wouldn't it?  :)

> 
> > the file metadata to the file data.  The IMA and EVM policies really
> > need to be in sync.
> 
> It would be nice, but at the moment EVM considers also files that are
> not selected by the IMA policy. An example of why this is a problem is
> the audit service that fails to start when it tries to adjust the permissions
> of the log files. Those files don't have security.evm because they are
> not appraised by IMA, but EVM denies the operation.

No, this is a timing issue as to whether or not the builtin policy or
a custom policy has been loaded.  A custom policy could exclude the
log files based on LSM labels, but they are included in the builtin
policy.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-07 20:45           ` Mimi Zohar
@ 2020-05-08 10:20             ` Roberto Sassu
  2020-05-08 17:08               ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-08 10:20 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> > > > > On Wed, 2020-05-06 at 15:44 -0400, Mimi Zohar wrote:
> > > > > > Since copying the EVM HMAC or original signature isn't applicable, I
> > > > > > would prefer exploring an EVM portable and immutable signature
> only
> > > > > > solution.
> > > > >
> > > > > To prevent copying the EVM xattr, we added "security.evm" to
> > > > > /etc/xattr.conf.  To support copying just the EVM portable and
> > > > > immutable signatures will require a different solution.
> > > >
> > > > This patch set removes the need for ignoring security.evm. It can be
> > > always
> > > > copied, even if it is an HMAC. EVM will update it only when verification
> in
> > > > the pre hook is successful. Combined with the ability of protecting a
> > > subset
> > > > of files without introducing an EVM policy, these advantages seem to
> > > > outweigh the effort necessary to make the switch.
> > >
> > > As the EVM file HMAC and original signature contain inode specific
> > > information (eg. i_version, i_generation), these xattrs cannot ever be
> > > copied.  The proposed change is in order to support just the new EVM
> > > signatures.
> >
> > Right, I didn't consider it.
> >
> > Would it make sense instead to introduce an alias like
> security.evm_immutable
> > so that this xattr can be copied?
> 
> Being portable, not the attribute of being immutable, allows copying
> the EVM xattr.  Your original problem - the order in which the xattrs
> are copied - might still be an issue.  We need to look at "cp" closer
> to understand what it is doing.  For example, are the xattrs written
> while the target file is tagged as a new file?

Yes:

openat(AT_FDCWD, "/bin/cat", O_RDONLY|O_NOFOLLOW) = 3
openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_EXCL, 0700) = 4
read(3, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 131072) = 85520
write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 85520) = 85520
read(3, "", 131072)                     = 0
flistxattr(3, NULL, 0)                  = 43
flistxattr(3, "security.selinux\0security.evm\0se"..., 43) = 43
fgetxattr(3, "security.evm", NULL, 0)   = 521
fgetxattr(3, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521) = 521
fsetxattr(4, "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0
fgetxattr(3, "security.ima", NULL, 0)   = 34
fgetxattr(3, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34) = 34
fsetxattr(4, "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0
fgetxattr(3, "system.posix_acl_access", 0x7ffdf6929310, 132) = -1 ENODATA (No data available)
fstat(3, {st_mode=S_IFREG|0755, st_size=85520, ...}) = 0
fsetxattr(4, "system.posix_acl_access", "\2\0\0\0\1\0\7\0\377\377\377\377\4\0\5\0\377\377\377\377 \0\5\0\377\377\377\377", 28, 0) = 0
close(4)                                = 0
close(3)                                = 0

This will fail. After security.evm is added to the target, the status when
security.ima is added is INTEGRITY_FAIL.

openat(AT_FDCWD, "test-archive.tar", O_RDONLY) = 3
mknodat(AT_FDCWD, "cat", 0755)          = 0
setxattr("cat", "security.selinux", "system_u:object_r:bin_t:s0", 27, 0) = 0
setxattr("cat", "security.evm", "\5\2\4\256\316\302\206\2\09\226L52=\233^n\376:\363+\231\272\213\t\247\312\324\263\222N"..., 521, 0) = 0
setxattr("cat", "security.ima", "\4\4\323\327\215\202I1~\325\0V\354}\4\3328$\210\363ja'\364\351\26\27\222\331\177\23\341"..., 34, 0) = 0
openat(AT_FDCWD, "cat", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK|O_CLOEXEC, 0700) = 4
write(4, "\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\3601\0\0\0\0\0\0"..., 8192) = 8192
read(3, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240
write(4, "\363\17\36\372H\203\354\10H\213\5\301\217\0\0H\205\300t\2\377\320H\203\304\10\303\0\0\0\0\0"..., 10240) = 10240
read(3, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240
write(4, "\300E\204\311t\16M9\346v\5C\306\4'\\I\203\304\1H\203\303\1H9\313\17\203M\1\0"..., 10240) = 10240
read(3, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240
write(4, "\1\0\2\0write error\0cat\0[\0test invoc"..., 10240) = 10240
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA*\6\22\0\0\0\21\0\f\0\21\0\0\0\0\0\0\0\0\1\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240
write(4, "\0\0\0\0\27\0\0\0\0\0\0\0\0\1\0\0GA$\5gcc 9.0.1 20"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA+stack_clash\0\0\23\0\0\0\0\0\0\0"..., 10240) = 10240
read(3, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 10240) = 10240
write(4, "\0\0\0\0\0\1\0\0GA+GLIBCXX_ASSERTIONS\0\0\0"..., 5648) = 5648
fchown(4, 0, 0)                         = 0
fchmod(4, 0755)                         = 0
close(4)                                = 0
close(3)                                = 0

Also this will fail, but for a different reason. After security.selinux is
written, the status when security.evm is added is INTEGRITY_NOLABEL
(unless the HMAC key is loaded, otherwise it will fail when tar sets
security.ima).

Even with my patches to ignore verification errors, there is still another
problem. The signature becomes valid when all xattrs are set, but tar
tries anyway to adjust the owner even if it is already correct. Since
metadata are immutable, fchown() fails (this status is not ignored).

After we solve the problem of copying xattrs, I will send a patch to
determine whether setxattr()/setattr() change metadata. If not, the
operation is allowed even if metadata are immutable.

> There have been similar problems in the past.  For example, tar calls
> mknodat to create the file, but doesn't write the file data.  The
> solution there was to tag the file as a new file.
> 
> We need to understand the problem better, before deciding how to
> resolve it.
> 
> >
> > > At least IMA file hashes should always be used in conjunction with
> > > EVM.  EVM xattrs should always require a security.ima xattr to bind
> >
> > I proposed to enforce this restriction some time ago:
> >
> > https://patchwork.kernel.org/patch/10979351/
> >
> > Is it ok to enforce it globally?
> 
> Doing this would then be dependent on upstreaming the initramfs xattr
> patches first, wouldn't it?  :)

Well, this patch set and the metadata change detection are a dependency
of the initramfs xattr patch set. Without them, we cannot create a ram disk
with files with portable signatures, if enforcement is enabled.

The new appraisal modes solve a general security issue that would occur
also when appraisal is done in the root filesystem (it is possible to gain
access to all files for which the IMA policy does not require a signature
by removing the EVM keys and adding to those files a valid security.ima
with the file digest).

> > > the file metadata to the file data.  The IMA and EVM policies really
> > > need to be in sync.
> >
> > It would be nice, but at the moment EVM considers also files that are
> > not selected by the IMA policy. An example of why this is a problem is
> > the audit service that fails to start when it tries to adjust the permissions
> > of the log files. Those files don't have security.evm because they are
> > not appraised by IMA, but EVM denies the operation.
> 
> No, this is a timing issue as to whether or not the builtin policy or
> a custom policy has been loaded.  A custom policy could exclude the
> log files based on LSM labels, but they are included in the builtin
> policy.

Yes, I was referring to a custom policy. In this case, EVM will not adapt
to the custom policy but still verifies all files. If access control is done
exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
need to add security.evm to all files.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-08 10:20             ` Roberto Sassu
@ 2020-05-08 17:08               ` Mimi Zohar
  2020-05-11 14:13                 ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-08 17:08 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:

<snip>

> > > > the file metadata to the file data.  The IMA and EVM policies really
> > > > need to be in sync.
> > >
> > > It would be nice, but at the moment EVM considers also files that are
> > > not selected by the IMA policy. An example of why this is a problem is
> > > the audit service that fails to start when it tries to adjust the permissions
> > > of the log files. Those files don't have security.evm because they are
> > > not appraised by IMA, but EVM denies the operation.
> > 
> > No, this is a timing issue as to whether or not the builtin policy or
> > a custom policy has been loaded.  A custom policy could exclude the
> > log files based on LSM labels, but they are included in the builtin
> > policy.
> 
> Yes, I was referring to a custom policy. In this case, EVM will not adapt
> to the custom policy but still verifies all files. If access control is done
> exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
> need to add security.evm to all files.

Roberto, EVM is only triggered by IMA, unless you've modified the
kernel to do otherwise.

I'm not interested in a complicated solution, just one that addresses
the new EVM immutable and portable signature.  It might require EVM
HMAC, IMA differentiating between a new file and an existing file, or
it might require writing the new EVM signature last, after all the
other xattrs or metadata are updated.  Please nothing that changes
existing expectations.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-08 17:08               ` Mimi Zohar
@ 2020-05-11 14:13                 ` Roberto Sassu
  2020-05-11 21:36                   ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-11 14:13 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, May 8, 2020 7:08 PM
> On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> 
> <snip>
> 
> > > > > the file metadata to the file data.  The IMA and EVM policies really
> > > > > need to be in sync.
> > > >
> > > > It would be nice, but at the moment EVM considers also files that are
> > > > not selected by the IMA policy. An example of why this is a problem is
> > > > the audit service that fails to start when it tries to adjust the
> permissions
> > > > of the log files. Those files don't have security.evm because they are
> > > > not appraised by IMA, but EVM denies the operation.
> > >
> > > No, this is a timing issue as to whether or not the builtin policy or
> > > a custom policy has been loaded.  A custom policy could exclude the
> > > log files based on LSM labels, but they are included in the builtin
> > > policy.
> >
> > Yes, I was referring to a custom policy. In this case, EVM will not adapt
> > to the custom policy but still verifies all files. If access control is done
> > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
> > need to add security.evm to all files.
> 
> Roberto, EVM is only triggered by IMA, unless you've modified the
> kernel to do otherwise.

EVM would deny xattr/attr operations even if IMA is disabled in the
kernel configuration. For example, evm_setxattr() returns the value
from evm_protect_xattr(). IMA is not involved there.

> I'm not interested in a complicated solution, just one that addresses
> the new EVM immutable and portable signature.  It might require EVM
> HMAC, IMA differentiating between a new file and an existing file, or
> it might require writing the new EVM signature last, after all the
> other xattrs or metadata are updated.  Please nothing that changes
> existing expectations.

Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I
mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by
EVM portable signatures' seems to have an additional benefit. We
could introduce an additional exception in evm_protect_xattr(), other
than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't
cause HMAC update.

However, it won't work unless the IMA policy says that the file should
be appraised when the mknod() system call is executed. Otherwise,
integrity_iint_cache is not created for the file and the IMA_NEW_FILE
flag is not set.

Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case
where security.evm is the first xattr set. If a protected xattr is the first to
be added, then we also have to handle the INTEGRITY_NOLABEL error.
It should be fine to add an exception for this error if the HMAC key is not
loaded.

This still does not solve all problems. INTEGRITY_NOLABEL cannot be
ignored if the HMAC key is loaded, which means that all files need to be
protected by EVM to avoid issues like the one I described (auditd).

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-11 14:13                 ` Roberto Sassu
@ 2020-05-11 21:36                   ` Mimi Zohar
  2020-05-12  7:54                     ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-11 21:36 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Mon, 2020-05-11 at 14:13 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, May 8, 2020 7:08 PM
> > On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> > 
> > <snip>
> > 
> > > > > > the file metadata to the file data.  The IMA and EVM policies really
> > > > > > need to be in sync.
> > > > >
> > > > > It would be nice, but at the moment EVM considers also files that are
> > > > > not selected by the IMA policy. An example of why this is a problem is
> > > > > the audit service that fails to start when it tries to adjust the
> > permissions
> > > > > of the log files. Those files don't have security.evm because they are
> > > > > not appraised by IMA, but EVM denies the operation.
> > > >
> > > > No, this is a timing issue as to whether or not the builtin policy or
> > > > a custom policy has been loaded.  A custom policy could exclude the
> > > > log files based on LSM labels, but they are included in the builtin
> > > > policy.
> > >
> > > Yes, I was referring to a custom policy. In this case, EVM will not adapt
> > > to the custom policy but still verifies all files. If access control is done
> > > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
> > > need to add security.evm to all files.
> > 
> > Roberto, EVM is only triggered by IMA, unless you've modified the
> > kernel to do otherwise.
> 
> EVM would deny xattr/attr operations even if IMA is disabled in the
> kernel configuration. For example, evm_setxattr() returns the value
> from evm_protect_xattr(). IMA is not involved there.

Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of
EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES to allow
writing the EVM portable and immutable file signatures. 

> 
> > I'm not interested in a complicated solution, just one that addresses
> > the new EVM immutable and portable signature.  It might require EVM
> > HMAC, IMA differentiating between a new file and an existing file, or:q

> > it might require writing the new EVM signature last, after all the
> > other xattrs or metadata are updated.  Please nothing that changes
> > existing expectations.
> 
> Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I
> mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by
> EVM portable signatures' seems to have an additional benefit. We
> could introduce an additional exception in evm_protect_xattr(), other
> than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't
> cause HMAC update.

Refer to Documentation/ABI/testing/evm describes on how to permit
writing the security.evm signatures.
> 
> However, it won't work unless the IMA policy says that the file should
> be appraised when the mknod() system call is executed. Otherwise,
> integrity_iint_cache is not created for the file and the IMA_NEW_FILE
> flag is not set.
> 
> Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case
> where security.evm is the first xattr set. If a protected xattr is the first to
> be added, then we also have to handle the INTEGRITY_NOLABEL error.
> It should be fine to add an exception for this error if the HMAC key is not
> loaded.
> 
> This still does not solve all problems. INTEGRITY_NOLABEL cannot be
> ignored if the HMAC key is loaded, which means that all files need to be
> protected by EVM to avoid issues like the one I described (auditd).

The application still needs to defer writing the EVM portable and
immutable file signatures until after all the other xattrs are written
otherwise it won't validate.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-11 21:36                   ` Mimi Zohar
@ 2020-05-12  7:54                     ` Roberto Sassu
  2020-05-12 14:17                       ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-12  7:54 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen, matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 11, 2020 11:37 PM
> On Mon, 2020-05-11 at 14:13 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Friday, May 8, 2020 7:08 PM
> > > On Fri, 2020-05-08 at 10:20 +0000, Roberto Sassu wrote:
> > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > > On Thu, 2020-05-07 at 16:47 +0000, Roberto Sassu wrote:
> > >
> > > <snip>
> > >
> > > > > > > the file metadata to the file data.  The IMA and EVM policies
> really
> > > > > > > need to be in sync.
> > > > > >
> > > > > > It would be nice, but at the moment EVM considers also files that
> are
> > > > > > not selected by the IMA policy. An example of why this is a
> problem is
> > > > > > the audit service that fails to start when it tries to adjust the
> > > permissions
> > > > > > of the log files. Those files don't have security.evm because they
> are
> > > > > > not appraised by IMA, but EVM denies the operation.
> > > > >
> > > > > No, this is a timing issue as to whether or not the builtin policy or
> > > > > a custom policy has been loaded.  A custom policy could exclude the
> > > > > log files based on LSM labels, but they are included in the builtin
> > > > > policy.
> > > >
> > > > Yes, I was referring to a custom policy. In this case, EVM will not adapt
> > > > to the custom policy but still verifies all files. If access control is done
> > > > exclusively by IMA at the time evm_verifyxattr() is called, we wouldn't
> > > > need to add security.evm to all files.
> > >
> > > Roberto, EVM is only triggered by IMA, unless you've modified the
> > > kernel to do otherwise.
> >
> > EVM would deny xattr/attr operations even if IMA is disabled in the
> > kernel configuration. For example, evm_setxattr() returns the value
> > from evm_protect_xattr(). IMA is not involved there.
> 
> Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of
> EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES
> to allow
> writing the EVM portable and immutable file signatures.

According to Documentation/ABI/testing/evm:

Note that once a key has been loaded, it will no longer be
possible to enable metadata modification.

We load the EVM key from /etc/keys/x509_evm.der, during kernel
initialization, which excludes the possibility of using that flag. Deferring
the loading of the key is not possible if that key is used for appraisal and
enforcement is enabled from the beginning.

Also, once the flag is cleared portable signatures cannot be installed
until reboot. This seems a big limitation especially when software
updates are installed on a running system.

> > > I'm not interested in a complicated solution, just one that addresses
> > > the new EVM immutable and portable signature.  It might require EVM
> > > HMAC, IMA differentiating between a new file and an existing file, or:q
> 
> > > it might require writing the new EVM signature last, after all the
> > > other xattrs or metadata are updated.  Please nothing that changes
> > > existing expectations.
> >
> > Ok. Introducing the new status INTEGRITY_FAIL_IMMUTABLE, as I
> > mentioned in '[PATCH] ima: Allow imasig requirement to be satisfied by
> > EVM portable signatures' seems to have an additional benefit. We
> > could introduce an additional exception in evm_protect_xattr(), other
> > than INTEGRITY_NOXATTRS, as we know that xattr/attr update won't
> > cause HMAC update.
> 
> Refer to Documentation/ABI/testing/evm describes on how to permit
> writing the security.evm signatures.

It doesn't seem there is a solution for adding portable signatures at
run-time.

> > However, it won't work unless the IMA policy says that the file should
> > be appraised when the mknod() system call is executed. Otherwise,
> > integrity_iint_cache is not created for the file and the IMA_NEW_FILE
> > flag is not set.
> >
> > Granting an exception for INTEGRITY_FAIL_IMMUTABLE solves the case
> > where security.evm is the first xattr set. If a protected xattr is the first to
> > be added, then we also have to handle the INTEGRITY_NOLABEL error.
> > It should be fine to add an exception for this error if the HMAC key is not
> > loaded.
> >
> > This still does not solve all problems. INTEGRITY_NOLABEL cannot be
> > ignored if the HMAC key is loaded, which means that all files need to be
> > protected by EVM to avoid issues like the one I described (auditd).
> 
> The application still needs to defer writing the EVM portable and
> immutable file signatures until after all the other xattrs are written
> otherwise it won't validate.

EVM won't allow that because at the second setxattr() it will find that
security.evm is missing.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12  7:54                     ` Roberto Sassu
@ 2020-05-12 14:17                       ` Mimi Zohar
  2020-05-12 15:31                         ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-12 14:17 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen,
	matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote:
> > > > Roberto, EVM is only triggered by IMA, unless you've modified the
> > > > kernel to do otherwise.
> > >
> > > EVM would deny xattr/attr operations even if IMA is disabled in the
> > > kernel configuration. For example, evm_setxattr() returns the value
> > > from evm_protect_xattr(). IMA is not involved there.
> > 
> > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of
> > EVM-protected metadata") introduced EVM_ALLOW_METADATA_WRITES
> > to allow writing the EVM portable and immutable file signatures.
> 
> According to Documentation/ABI/testing/evm:
> 
> Note that once a key has been loaded, it will no longer be
> possible to enable metadata modification.

Not any key, but the HMAC key.
 
2         Permit modification of EVM-protected metadata at
          runtime. Not supported if HMAC validation and
          creation is enabled.

Each time the EVM protected file metadata is updated, the EVM HMAC is
updated, assuming the existing EVM HMAC is valid.  Userspace should
not have access to the HMAC key, so we only allow writing EVM
signatures.

The only difference between writing the original EVM signature and the
new portable and immutable signature is the security.ima xattr
requirement.  Since the new EVM signature does not include the
filesystem specific data, something else needs to bind the file
metadata to the file data.  Thus the IMA xattr requirement.

Assuming that the new EVM signature is written last, as long as there
is an IMA xattr, there shouldn't be a problem writing the new EVM
signature.

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12 14:17                       ` Mimi Zohar
@ 2020-05-12 15:31                         ` Roberto Sassu
  2020-05-12 15:50                           ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-12 15:31 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen, matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Tuesday, May 12, 2020 4:17 PM
> On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote:
> > > > > Roberto, EVM is only triggered by IMA, unless you've modified the
> > > > > kernel to do otherwise.
> > > >
> > > > EVM would deny xattr/attr operations even if IMA is disabled in the
> > > > kernel configuration. For example, evm_setxattr() returns the value
> > > > from evm_protect_xattr(). IMA is not involved there.
> > >
> > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of
> > > EVM-protected metadata")
> introduced EVM_ALLOW_METADATA_WRITES
> > > to allow writing the EVM portable and immutable file signatures.
> >
> > According to Documentation/ABI/testing/evm:
> >
> > Note that once a key has been loaded, it will no longer be
> > possible to enable metadata modification.
> 
> Not any key, but the HMAC key.
> 
> 2         Permit modification of EVM-protected metadata at
>           runtime. Not supported if HMAC validation and
>           creation is enabled.

#ifdef CONFIG_EVM_LOAD_X509
void __init evm_load_x509(void)
{
[...]
        rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
        if (!rc)
                evm_initialized |= EVM_INIT_X509;


static ssize_t evm_write_key(struct file *file, const char __user *buf,
                             size_t count, loff_t *ppos)
{
[...]
        /* Don't allow a request to freshly enable metadata writes if
         * keys are loaded.
         */
        if ((i & EVM_ALLOW_METADATA_WRITES) &&
            ((evm_initialized & EVM_KEY_MASK) != 0) &&
            !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
                return -EPERM;

Should have been:

        if ((i & EVM_ALLOW_METADATA_WRITES) &&
            ((evm_initialized & EVM_INIT_HMAC) != 0) &&
            !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
                return -EPERM;

> Each time the EVM protected file metadata is updated, the EVM HMAC is
> updated, assuming the existing EVM HMAC is valid.  Userspace should
> not have access to the HMAC key, so we only allow writing EVM
> signatures.
> 
> The only difference between writing the original EVM signature and the
> new portable and immutable signature is the security.ima xattr
> requirement.  Since the new EVM signature does not include the
> filesystem specific data, something else needs to bind the file
> metadata to the file data.  Thus the IMA xattr requirement.
> 
> Assuming that the new EVM signature is written last, as long as there
> is an IMA xattr, there shouldn't be a problem writing the new EVM
> signature.

        /* first need to know the sig type */
        rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char **)&xattr_data, 0,
                                GFP_NOFS);
        if (rc <= 0) {
                evm_status = INTEGRITY_FAIL;
                if (rc == -ENODATA) {
                        rc = evm_find_protected_xattrs(dentry);
                        if (rc > 0)
                                evm_status = INTEGRITY_NOLABEL;
                        else if (rc == 0)
                                evm_status = INTEGRITY_NOXATTRS; /* new file */

If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
can be written (status INTEGRITY_NOXATTRS is ok). After,
evm_find_protected_xattrs() returns rc > 0, so the status is
INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12 15:31                         ` Roberto Sassu
@ 2020-05-12 15:50                           ` Mimi Zohar
  2020-05-12 16:31                             ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-12 15:50 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen,
	matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Tue, 2020-05-12 at 15:31 +0000, Roberto Sassu wrote:
> > From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Tuesday, May 12, 2020 4:17 PM
> > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote:
> > > > > > Roberto, EVM is only triggered by IMA, unless you've modified the
> > > > > > kernel to do otherwise.
> > > > >
> > > > > EVM would deny xattr/attr operations even if IMA is disabled in the
> > > > > kernel configuration. For example, evm_setxattr() returns the value
> > > > > from evm_protect_xattr(). IMA is not involved there.
> > > >
> > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification of
> > > > EVM-protected metadata")
> > introduced EVM_ALLOW_METADATA_WRITES
> > > > to allow writing the EVM portable and immutable file signatures.
> > >
> > > According to Documentation/ABI/testing/evm:
> > >
> > > Note that once a key has been loaded, it will no longer be
> > > possible to enable metadata modification.
> > 
> > Not any key, but the HMAC key.
> > 
> > 2         Permit modification of EVM-protected metadata at
> >           runtime. Not supported if HMAC validation and
> >           creation is enabled.
> 
> #ifdef CONFIG_EVM_LOAD_X509
> void __init evm_load_x509(void)
> {
> [...]
>         rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
>         if (!rc)
>                 evm_initialized |= EVM_INIT_X509;
> 
> 
> static ssize_t evm_write_key(struct file *file, const char __user *buf,
>                              size_t count, loff_t *ppos)
> {
> [...]
>         /* Don't allow a request to freshly enable metadata writes if
>          * keys are loaded.
>          */
>         if ((i & EVM_ALLOW_METADATA_WRITES) &&
>             ((evm_initialized & EVM_KEY_MASK) != 0) &&
>             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
>                 return -EPERM;
> 
> Should have been:
> 
>         if ((i & EVM_ALLOW_METADATA_WRITES) &&
>             ((evm_initialized & EVM_INIT_HMAC) != 0) &&
>             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
>                 return -EPERM;

Ok

> 
> > Each time the EVM protected file metadata is updated, the EVM HMAC is
> > updated, assuming the existing EVM HMAC is valid.  Userspace should
> > not have access to the HMAC key, so we only allow writing EVM
> > signatures.
> > 
> > The only difference between writing the original EVM signature and the
> > new portable and immutable signature is the security.ima xattr
> > requirement.  Since the new EVM signature does not include the
> > filesystem specific data, something else needs to bind the file
> > metadata to the file data.  Thus the IMA xattr requirement.
> > 
> > Assuming that the new EVM signature is written last, as long as there
> > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > signature.
> 
>         /* first need to know the sig type */
>         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char **)&xattr_data, 0,
>                                 GFP_NOFS);
>         if (rc <= 0) {
>                 evm_status = INTEGRITY_FAIL;
>                 if (rc == -ENODATA) {
>                         rc = evm_find_protected_xattrs(dentry);
>                         if (rc > 0)
>                                 evm_status = INTEGRITY_NOLABEL;
>                         else if (rc == 0)
>                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> 
> If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> can be written (status INTEGRITY_NOXATTRS is ok). After,
> evm_find_protected_xattrs() returns rc > 0, so the status is
> INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().

With EVM HMAC enabled, as a result of writing the first protected
xattr, an EVM HMAC should be calculated and written in
evm_inode_post_setxattr().

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12 15:50                           ` Mimi Zohar
@ 2020-05-12 16:31                             ` Roberto Sassu
  2020-05-12 19:38                               ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-12 16:31 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen, matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, May 12, 2020 5:50 PM
> On Tue, 2020-05-12 at 15:31 +0000, Roberto Sassu wrote:
> > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-
> linux-
> > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > > Sent: Tuesday, May 12, 2020 4:17 PM
> > > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote:
> > > > > > > Roberto, EVM is only triggered by IMA, unless you've modified
> the
> > > > > > > kernel to do otherwise.
> > > > > >
> > > > > > EVM would deny xattr/attr operations even if IMA is disabled in
> the
> > > > > > kernel configuration. For example, evm_setxattr() returns the
> value
> > > > > > from evm_protect_xattr(). IMA is not involved there.
> > > > >
> > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification
> of
> > > > > EVM-protected metadata")
> > > introduced EVM_ALLOW_METADATA_WRITES
> > > > > to allow writing the EVM portable and immutable file signatures.
> > > >
> > > > According to Documentation/ABI/testing/evm:
> > > >
> > > > Note that once a key has been loaded, it will no longer be
> > > > possible to enable metadata modification.
> > >
> > > Not any key, but the HMAC key.
> > >
> > > 2         Permit modification of EVM-protected metadata at
> > >           runtime. Not supported if HMAC validation and
> > >           creation is enabled.
> >
> > #ifdef CONFIG_EVM_LOAD_X509
> > void __init evm_load_x509(void)
> > {
> > [...]
> >         rc = integrity_load_x509(INTEGRITY_KEYRING_EVM,
> CONFIG_EVM_X509_PATH);
> >         if (!rc)
> >                 evm_initialized |= EVM_INIT_X509;
> >
> >
> > static ssize_t evm_write_key(struct file *file, const char __user *buf,
> >                              size_t count, loff_t *ppos)
> > {
> > [...]
> >         /* Don't allow a request to freshly enable metadata writes if
> >          * keys are loaded.
> >          */
> >         if ((i & EVM_ALLOW_METADATA_WRITES) &&
> >             ((evm_initialized & EVM_KEY_MASK) != 0) &&
> >             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> >                 return -EPERM;
> >
> > Should have been:
> >
> >         if ((i & EVM_ALLOW_METADATA_WRITES) &&
> >             ((evm_initialized & EVM_INIT_HMAC) != 0) &&
> >             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> >                 return -EPERM;
> 
> Ok
> 
> >
> > > Each time the EVM protected file metadata is updated, the EVM HMAC
> is
> > > updated, assuming the existing EVM HMAC is valid.  Userspace should
> > > not have access to the HMAC key, so we only allow writing EVM
> > > signatures.
> > >
> > > The only difference between writing the original EVM signature and the
> > > new portable and immutable signature is the security.ima xattr
> > > requirement.  Since the new EVM signature does not include the
> > > filesystem specific data, something else needs to bind the file
> > > metadata to the file data.  Thus the IMA xattr requirement.
> > >
> > > Assuming that the new EVM signature is written last, as long as there
> > > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > > signature.
> >
> >         /* first need to know the sig type */
> >         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char
> **)&xattr_data, 0,
> >                                 GFP_NOFS);
> >         if (rc <= 0) {
> >                 evm_status = INTEGRITY_FAIL;
> >                 if (rc == -ENODATA) {
> >                         rc = evm_find_protected_xattrs(dentry);
> >                         if (rc > 0)
> >                                 evm_status = INTEGRITY_NOLABEL;
> >                         else if (rc == 0)
> >                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> >
> > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> > can be written (status INTEGRITY_NOXATTRS is ok). After,
> > evm_find_protected_xattrs() returns rc > 0, so the status is
> > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().
> 
> With EVM HMAC enabled, as a result of writing the first protected
> xattr, an EVM HMAC should be calculated and written in
> evm_inode_post_setxattr().

To solve the ordering issue, wouldn't allowing setxattr() on a file
with portable signature that does not yet pass verification be safe?
evm_update_evmxattr() checks if the signature is portable and
if yes, does not calculate the HMAC.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12 16:31                             ` Roberto Sassu
@ 2020-05-12 19:38                               ` Mimi Zohar
  2020-05-13  7:21                                 ` Roberto Sassu
  0 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2020-05-12 19:38 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen,
	matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]

> > > > Each time the EVM protected file metadata is updated, the EVM HMAC
> > is
> > > > updated, assuming the existing EVM HMAC is valid.  Userspace should
> > > > not have access to the HMAC key, so we only allow writing EVM
> > > > signatures.
> > > >
> > > > The only difference between writing the original EVM signature and the
> > > > new portable and immutable signature is the security.ima xattr
> > > > requirement.  Since the new EVM signature does not include the
> > > > filesystem specific data, something else needs to bind the file
> > > > metadata to the file data.  Thus the IMA xattr requirement.
> > > >
> > > > Assuming that the new EVM signature is written last, as long as there
> > > > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > > > signature.
> > >
> > >         /* first need to know the sig type */
> > >         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char
> > **)&xattr_data, 0,
> > >                                 GFP_NOFS);
> > >         if (rc <= 0) {
> > >                 evm_status = INTEGRITY_FAIL;
> > >                 if (rc == -ENODATA) {
> > >                         rc = evm_find_protected_xattrs(dentry);
> > >                         if (rc > 0)
> > >                                 evm_status = INTEGRITY_NOLABEL;
> > >                         else if (rc == 0)
> > >                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> > >
> > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> > > can be written (status INTEGRITY_NOXATTRS is ok). After,
> > > evm_find_protected_xattrs() returns rc > 0, so the status is
> > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().
> > 
> > With EVM HMAC enabled, as a result of writing the first protected
> > xattr, an EVM HMAC should be calculated and written in
> > evm_inode_post_setxattr().
> 
> To solve the ordering issue, wouldn't allowing setxattr() on a file
> with portable signature that does not yet pass verification be safe?
> evm_update_evmxattr() checks if the signature is portable and
> if yes, does not calculate the HMAC.

Before agreeing to allowing the protected xattrs to be written on a
file with a portable signature that does not yet pass verification are
safe, would we be introducing any new types of attacks?

For example, would we differentiate between portable signatures that
don't pass verification and ones that do?  If we don't differentiate,
could it be used for DoS?  Should it be limited to new files?

Mimi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-12 19:38                               ` Mimi Zohar
@ 2020-05-13  7:21                                 ` Roberto Sassu
  2020-05-13 15:09                                   ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Roberto Sassu @ 2020-05-13  7:21 UTC (permalink / raw)
  To: Mimi Zohar, david.safford, viro, jmorris, John Johansen, matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, May 12, 2020 9:38 PM
> On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> 
> > > > > Each time the EVM protected file metadata is updated, the EVM
> HMAC
> > > is
> > > > > updated, assuming the existing EVM HMAC is valid.  Userspace
> should
> > > > > not have access to the HMAC key, so we only allow writing EVM
> > > > > signatures.
> > > > >
> > > > > The only difference between writing the original EVM signature and
> the
> > > > > new portable and immutable signature is the security.ima xattr
> > > > > requirement.  Since the new EVM signature does not include the
> > > > > filesystem specific data, something else needs to bind the file
> > > > > metadata to the file data.  Thus the IMA xattr requirement.
> > > > >
> > > > > Assuming that the new EVM signature is written last, as long as there
> > > > > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > > > > signature.
> > > >
> > > >         /* first need to know the sig type */
> > > >         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char
> > > **)&xattr_data, 0,
> > > >                                 GFP_NOFS);
> > > >         if (rc <= 0) {
> > > >                 evm_status = INTEGRITY_FAIL;
> > > >                 if (rc == -ENODATA) {
> > > >                         rc = evm_find_protected_xattrs(dentry);
> > > >                         if (rc > 0)
> > > >                                 evm_status = INTEGRITY_NOLABEL;
> > > >                         else if (rc == 0)
> > > >                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> > > >
> > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> > > > can be written (status INTEGRITY_NOXATTRS is ok). After,
> > > > evm_find_protected_xattrs() returns rc > 0, so the status is
> > > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().
> > >
> > > With EVM HMAC enabled, as a result of writing the first protected
> > > xattr, an EVM HMAC should be calculated and written in
> > > evm_inode_post_setxattr().
> >
> > To solve the ordering issue, wouldn't allowing setxattr() on a file
> > with portable signature that does not yet pass verification be safe?
> > evm_update_evmxattr() checks if the signature is portable and
> > if yes, does not calculate the HMAC.
> 
> Before agreeing to allowing the protected xattrs to be written on a
> file with a portable signature that does not yet pass verification are
> safe, would we be introducing any new types of attacks?

Allowing xattr/attr update means that someone can do:

setxattr(path, "security.evm", ...);	with type=5

all subsequent setxattr()/setattr() succeed until the correct
combination is set.

At that point, any xattr/attr operation fails, even if one tries to set
an xattr with the same value. If we still deny the operation when the
verification succeeds, we have to fix that.

It is common that the signature passes verification before user space
tools finish to set xattrs/attrs. For example, if a file is created with
mode 644 and this was the mode at the time of signing, any attempt
by tar for example to set again the same mode fails.

If allowing a change of xattrs/attrs for portable signatures is safe or
not, I would say yes. Portable signatures cannot be modified even
if __vfs_setxattr_noperm() is called directly.

> For example, would we differentiate between portable signatures that
> don't pass verification and ones that do?  If we don't differentiate,
> could it be used for DoS?  Should it be limited to new files?

I would prefer to lock files when signatures pass the verification to
avoid accidental changes.

Unless we find a better way to identify new file, without depending
on the appraisal policy, I would allow the operation even for existing
files.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
  2020-05-13  7:21                                 ` Roberto Sassu
@ 2020-05-13 15:09                                   ` Mimi Zohar
  0 siblings, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2020-05-13 15:09 UTC (permalink / raw)
  To: Roberto Sassu, david.safford, viro, jmorris, John Johansen,
	matthewgarrett
  Cc: linux-fsdevel, linux-integrity, linux-security-module,
	linux-kernel, Silviu Vlasceanu

On Wed, 2020-05-13 at 07:21 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Tuesday, May 12, 2020 9:38 PM
> > On Tue, 2020-05-12 at 16:31 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > 
> > > > > > Each time the EVM protected file metadata is updated, the EVM
> > HMAC
> > > > is
> > > > > > updated, assuming the existing EVM HMAC is valid.  Userspace
> > should
> > > > > > not have access to the HMAC key, so we only allow writing EVM
> > > > > > signatures.
> > > > > >
> > > > > > The only difference between writing the original EVM signature and
> > the
> > > > > > new portable and immutable signature is the security.ima xattr
> > > > > > requirement.  Since the new EVM signature does not include the
> > > > > > filesystem specific data, something else needs to bind the file
> > > > > > metadata to the file data.  Thus the IMA xattr requirement.
> > > > > >
> > > > > > Assuming that the new EVM signature is written last, as long as there
> > > > > > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > > > > > signature.
> > > > >
> > > > >         /* first need to know the sig type */
> > > > >         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char
> > > > **)&xattr_data, 0,
> > > > >                                 GFP_NOFS);
> > > > >         if (rc <= 0) {
> > > > >                 evm_status = INTEGRITY_FAIL;
> > > > >                 if (rc == -ENODATA) {
> > > > >                         rc = evm_find_protected_xattrs(dentry);
> > > > >                         if (rc > 0)
> > > > >                                 evm_status = INTEGRITY_NOLABEL;
> > > > >                         else if (rc == 0)
> > > > >                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> > > > >
> > > > > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> > > > > can be written (status INTEGRITY_NOXATTRS is ok). After,
> > > > > evm_find_protected_xattrs() returns rc > 0, so the status is
> > > > > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().
> > > >
> > > > With EVM HMAC enabled, as a result of writing the first protected
> > > > xattr, an EVM HMAC should be calculated and written in
> > > > evm_inode_post_setxattr().
> > >
> > > To solve the ordering issue, wouldn't allowing setxattr() on a file
> > > with portable signature that does not yet pass verification be safe?
> > > evm_update_evmxattr() checks if the signature is portable and
> > > if yes, does not calculate the HMAC.
> > 
> > Before agreeing to allowing the protected xattrs to be written on a
> > file with a portable signature that does not yet pass verification are
> > safe, would we be introducing any new types of attacks?
> 
> Allowing xattr/attr update means that someone can do:
> 
> setxattr(path, "security.evm", ...);	with type=5
> 
> all subsequent setxattr()/setattr() succeed until the correct
> combination is set.
> 
> At that point, any xattr/attr operation fails, even if one tries to set
> an xattr with the same value. If we still deny the operation when the
> verification succeeds, we have to fix that.
> 
> It is common that the signature passes verification before user space
> tools finish to set xattrs/attrs. For example, if a file is created with
> mode 644 and this was the mode at the time of signing, any attempt
> by tar for example to set again the same mode fails.

We have a couple of options: always fail the write, differentiate the
reason for the failure, or check the value before failing the write.
 The easiest obviously would be to always fail the write once it is
valid.  Whatever you decide, please keep it simple.

> 
> If allowing a change of xattrs/attrs for portable signatures is safe or
> not, I would say yes. Portable signatures cannot be modified even
> if __vfs_setxattr_noperm() is called directly.
> 
> > For example, would we differentiate between portable signatures that
> > don't pass verification and ones that do?  If we don't differentiate,
> > could it be used for DoS?  Should it be limited to new files?
> 
> I would prefer to lock files when signatures pass the verification to
> avoid accidental changes.
> 
> Unless we find a better way to identify new file, without depending
> on the appraisal policy, I would allow the operation even for existing
> files.

The existing code verifies the EVM xattr before allowing the
xattr/attr change.  It sounds like for EVM_XATTR_PORTABLE_DIGSIG we
need to do exactly the reverse.

Mimi


^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, back to index

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29  7:39 [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Roberto Sassu
2020-04-29  7:39 ` [RFC][PATCH 2/3] evm: Extend API of post hooks to pass the result of pre hooks Roberto Sassu
2020-04-29  7:39 ` [RFC][PATCH 3/3] evm: Return -EAGAIN to ignore verification failures Roberto Sassu
2020-04-30 16:11 ` [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure Lev R. Oshvang .
2020-05-01  6:56   ` Roberto Sassu
2020-05-06 16:11 ` Roberto Sassu
2020-05-06 19:44 ` Mimi Zohar
2020-05-06 21:10   ` Mimi Zohar
2020-05-07  7:53     ` Roberto Sassu
2020-05-07 15:17       ` Mimi Zohar
2020-05-07 16:47         ` Roberto Sassu
2020-05-07 20:45           ` Mimi Zohar
2020-05-08 10:20             ` Roberto Sassu
2020-05-08 17:08               ` Mimi Zohar
2020-05-11 14:13                 ` Roberto Sassu
2020-05-11 21:36                   ` Mimi Zohar
2020-05-12  7:54                     ` Roberto Sassu
2020-05-12 14:17                       ` Mimi Zohar
2020-05-12 15:31                         ` Roberto Sassu
2020-05-12 15:50                           ` Mimi Zohar
2020-05-12 16:31                             ` Roberto Sassu
2020-05-12 19:38                               ` Mimi Zohar
2020-05-13  7:21                                 ` Roberto Sassu
2020-05-13 15:09                                   ` Mimi Zohar

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git