diff mbox series

[v7,6/6] evm: Support multiple LSMs providing an xattr

Message ID 20221201104125.919483-7-roberto.sassu@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series evm: Do HMAC of multiple per LSM xattrs for new inodes | expand

Commit Message

Roberto Sassu Dec. 1, 2022, 10:41 a.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

Currently, evm_inode_init_security() processes a single LSM xattr from
the array passed by security_inode_init_security(), and calculates the
HMAC on it and other inode metadata.

Given that initxattrs() callbacks, called by
security_inode_init_security(), expect that this array is terminated when
the xattr name is set to NULL, reuse the same assumption to scan all xattrs
and to calculate the HMAC on all of them.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/integrity/evm/evm.h        |  2 ++
 security/integrity/evm/evm_crypto.c |  9 ++++++++-
 security/integrity/evm/evm_main.c   | 16 +++++++++++-----
 3 files changed, 21 insertions(+), 6 deletions(-)

Comments

Mimi Zohar Feb. 19, 2023, 7:42 p.m. UTC | #1 
On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Currently, evm_inode_init_security() processes a single LSM xattr from
> the array passed by security_inode_init_security(), and calculates the
> HMAC on it and other inode metadata.
> 
> Given that initxattrs() callbacks, called by
> security_inode_init_security(), expect that this array is terminated when
> the xattr name is set to NULL, reuse the same assumption to scan all xattrs
> and to calculate the HMAC on all of them.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>


Normally changing the contents of the EVM HMAC calculation would break
existing systems.  Assuming for the time being this is safe, at what
point will it affect backwards compatability?  Should it be documented
now or then?
Roberto Sassu Feb. 20, 2023, 9:49 a.m. UTC | #2 
On Sun, 2023-02-19 at 14:42 -0500, Mimi Zohar wrote:
> On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Currently, evm_inode_init_security() processes a single LSM xattr from
> > the array passed by security_inode_init_security(), and calculates the
> > HMAC on it and other inode metadata.
> > 
> > Given that initxattrs() callbacks, called by
> > security_inode_init_security(), expect that this array is terminated when
> > the xattr name is set to NULL, reuse the same assumption to scan all xattrs
> > and to calculate the HMAC on all of them.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> Normally changing the contents of the EVM HMAC calculation would break
> existing systems.  Assuming for the time being this is safe, at what
> point will it affect backwards compatability?  Should it be documented
> now or then?

Actually, the current patch set continues to fullfill user space
expectation on the EVM behavior. If the LSM infrastructure created more
xattrs and EVM calculated the HMAC on just one, there would be a
problem on subsequent xattr operations and on IMA verification.

By updating both the LSM infrastructure and EVM to support multiple
xattrs, everything will continue to work.

Thanks

Roberto
Mimi Zohar Feb. 20, 2023, 10:56 a.m. UTC | #3 
On Mon, 2023-02-20 at 10:49 +0100, Roberto Sassu wrote:
> On Sun, 2023-02-19 at 14:42 -0500, Mimi Zohar wrote:
> > On Thu, 2022-12-01 at 11:41 +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Currently, evm_inode_init_security() processes a single LSM xattr from
> > > the array passed by security_inode_init_security(), and calculates the
> > > HMAC on it and other inode metadata.
> > > 
> > > Given that initxattrs() callbacks, called by
> > > security_inode_init_security(), expect that this array is terminated when
> > > the xattr name is set to NULL, reuse the same assumption to scan all xattrs
> > > and to calculate the HMAC on all of them.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > 
> > Normally changing the contents of the EVM HMAC calculation would break
> > existing systems.  Assuming for the time being this is safe, at what
> > point will it affect backwards compatability?  Should it be documented
> > now or then?
> 
> Actually, the current patch set continues to fullfill user space
> expectation on the EVM behavior. If the LSM infrastructure created more
> xattrs and EVM calculated the HMAC on just one, there would be a
> problem on subsequent xattr operations and on IMA verification.
> 
> By updating both the LSM infrastructure and EVM to support multiple
> xattrs, everything will continue to work.

Agreed.  Thank you for the reminder of the bug report being addressed
by this patch set.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
diff mbox series

Patch

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index f8b8c5004fc7..f799d72a59fa 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -46,6 +46,8 @@  struct evm_digest {
 	char digest[IMA_MAX_DIGEST_SIZE];
 } __packed;
 
+int evm_protected_xattr(const char *req_xattr_name);
+
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
 			const char *req_xattr_name,
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 708de9656bbd..68f99faac316 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -389,6 +389,7 @@  int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		  char *hmac_val)
 {
 	struct shash_desc *desc;
+	const struct xattr *xattr;
 
 	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
 	if (IS_ERR(desc)) {
@@ -396,7 +397,13 @@  int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		return PTR_ERR(desc);
 	}
 
-	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
+	for (xattr = lsm_xattr; xattr->name != NULL; xattr++) {
+		if (!evm_protected_xattr(xattr->name))
+			continue;
+
+		crypto_shash_update(desc, xattr->value, xattr->value_len);
+	}
+
 	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
 	kfree(desc);
 	return 0;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0a312cafb7de..1cf6871a0019 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -305,7 +305,7 @@  static int evm_protected_xattr_common(const char *req_xattr_name,
 	return found;
 }
 
-static int evm_protected_xattr(const char *req_xattr_name)
+int evm_protected_xattr(const char *req_xattr_name)
 {
 	return evm_protected_xattr_common(req_xattr_name, false);
 }
@@ -851,14 +851,20 @@  int evm_inode_init_security(struct inode *inode, struct inode *dir,
 {
 	struct evm_xattr *xattr_data;
 	struct xattr *xattr, *evm_xattr;
+	bool evm_protected_xattrs = false;
 	int rc;
 
-	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs ||
-	    !evm_protected_xattr(xattrs->name))
+	if (!(evm_initialized & EVM_INIT_HMAC) || !xattrs)
 		return -EOPNOTSUPP;
 
-	for (xattr = xattrs; xattr->value != NULL; xattr++)
-		;
+	for (xattr = xattrs; xattr->value != NULL; xattr++) {
+		if (evm_protected_xattr(xattr->name))
+			evm_protected_xattrs = true;
+	}
+
+	/* EVM xattr not needed. */
+	if (!evm_protected_xattrs)
+		return -EOPNOTSUPP;
 
 	evm_xattr = xattr;