[PATCH v2 00/12] IMA/EVM fixes

[PATCH v2 00/12] IMA/EVM fixes

[Roberto Sassu]

This patch set includes various fixes for IMA and EVM.

Patches 1-3 are trivial fixes. The remaining improve support and usability
of EVM portable signatures. In particular patch 4 allows EVM to be used
without an HMAC key. Patch 5 avoids appraisal verification of public keys
(they are already verified by the key subsystem).

Patches 6-7 allow metadata verification to be turned off when the HMAC key
is not already loaded and to use this mode in a safe way (by ensuring that
IMA revalidates metadata when there is a change).

Patches 8-9 make portable signatures more usable if metadata verification
cannot be turned off (because the HMAC key is loaded) by accepting any
metadata modification until signature verification succeeds (useful when
xattrs/attrs are copied in a sequence from a source) and by allowing
operations that don't change metadata.

Patch 10 makes it possible to use portable signatures when the IMA policy
requires file signatures and patch 11 shows portable signatures when the
ima-sig measurement list template is selected.

Lastly, patch 12 avoids undesired removal of security.ima when a file is
not selected by the IMA policy.

Roberto Sassu (12):
  ima: Don't ignore errors from crypto_shash_update()
  ima: Remove semicolon at the end of ima_get_binary_runtime_size()
  evm: Check size of security.evm before using it
  evm: Execute evm_inode_init_security() only when the HMAC key is
    loaded
  evm: Load EVM key in ima_load_x509() to avoid appraisal
  evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded
  evm: Introduce EVM_RESET_STATUS atomic flag
  evm: Allow xattr/attr operations for portable signatures if check
    fails
  evm: Allow setxattr() and setattr() if metadata digest won't change
  ima: Allow imasig requirement to be satisfied by EVM portable
    signatures
  ima: Introduce template field evmsig and write to field sig as
    fallback
  ima: Don't remove security.ima if file must not be appraised

 Documentation/ABI/testing/evm             |   6 +-
 Documentation/security/IMA-templates.rst  |   4 +-
 include/linux/integrity.h                 |   1 +
 security/integrity/evm/evm_main.c         | 151 ++++++++++++++++++++--
 security/integrity/evm/evm_secfs.c        |   2 +-
 security/integrity/iint.c                 |   2 +
 security/integrity/ima/ima_appraise.c     |  26 ++--
 security/integrity/ima/ima_crypto.c       |   2 +
 security/integrity/ima/ima_init.c         |   4 +
 security/integrity/ima/ima_main.c         |   8 +-
 security/integrity/ima/ima_queue.c        |   2 +-
 security/integrity/ima/ima_template.c     |   2 +
 security/integrity/ima/ima_template_lib.c |  39 +++++-
 security/integrity/ima/ima_template_lib.h |   2 +
 security/integrity/integrity.h            |   1 +
 15 files changed, 225 insertions(+), 27 deletions(-)

-- 
2.27.GIT

[PATCH v2 01/12] ima: Don't ignore errors from crypto_shash_update()

[Roberto Sassu]

Errors returned by crypto_shash_update() are not checked in
ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next
iteration of the loop. This patch adds a check after calling
crypto_shash_update() and returns immediately if the result is not zero.

Cc: stable@vger.kernel.org
Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 011c3c76af86..21989fa0c107 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -829,6 +829,8 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 		/* now accumulate with current aggregate */
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
+		if (rc != 0)
+			return rc;
 	}
 	/*
 	 * Extend cumulative digest over TPM registers 8-9, which contain
-- 
2.27.GIT

Re: [PATCH v2 01/12] ima: Don't ignore errors from crypto_shash_update()

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 3323eec921ef ("integrity: IMA as an integrity service provider").

The bot has tested the following trees: v5.8.7, v5.4.63, v4.19.143, v4.14.196, v4.9.235, v4.4.235.

v5.8.7: Build OK!
v5.4.63: Build OK!
v4.19.143: Failed to apply! Possible dependencies:
    100b16a6f290 ("tpm: sort objects in the Makefile")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    70a3199a7101 ("tpm: factor out tpm_get_timeouts()")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    95adc6b410b7 ("tpm: use u32 instead of int for PCR index")
    b03c43702e7b ("tpm: add tpm_auto_startup() into tpm-interface.c")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c")
    d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")

v4.14.196: Failed to apply! Possible dependencies:
    5ef924d9e2e8 ("tpm: use tpm_msleep() value as max delay")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    95adc6b410b7 ("tpm: use u32 instead of int for PCR index")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    b03c43702e7b ("tpm: add tpm_auto_startup() into tpm-interface.c")
    c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c")
    d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c")
    fc1d52b745ba ("tpm: rename tpm_chip_find_get() to tpm_find_get_ops()")

v4.9.235: Failed to apply! Possible dependencies:
    06e93279ca77 ("tpm: move endianness conversion of TPM_TAG_RQU_COMMAND to tpm_input_header")
    175d5b2a570c ("tpm: move TPM 1.2 code of tpm_pcr_extend() to tpm1_pcr_extend()")
    37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()")
    51b0be640cf6 ("tpm: Fix expected number of response bytes of TPM1.2 PCR Extend")
    62bfdacbac4c ("tpm: Do not print an error message when doing TPM auto startup")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    84fda15286d1 ("tpm: sanitize constant expressions")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    a69faebf4d3e ("tpm: move endianness conversion of ordinals to tpm_input_header")
    aaa6f7f6c8bf ("tpm: Clean up reading of timeout and duration capabilities")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    c659af78eb7b ("tpm: Check size of response before accessing data")
    ca6d45802201 ("tpm: place kdoc just above tpm_pcr_extend")
    f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")

v4.4.235: Failed to apply! Possible dependencies:
    0014777f989b ("tpm: constify TPM 1.x header structures")
    062807f20e3f ("tpm: Remove all uses of drvdata from the TPM Core")
    06e93279ca77 ("tpm: move endianness conversion of TPM_TAG_RQU_COMMAND to tpm_input_header")
    175d5b2a570c ("tpm: move TPM 1.2 code of tpm_pcr_extend() to tpm1_pcr_extend()")
    25112048cd59 ("tpm: rework tpm_get_timeouts()")
    3635e2ec7cbb ("tpm: Get rid of devname")
    37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()")
    570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
    6e599f6f261f ("tpm: drop 'read_queue' from struct tpm_vendor_specific")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
    84fda15286d1 ("tpm: sanitize constant expressions")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    a69faebf4d3e ("tpm: move endianness conversion of ordinals to tpm_input_header")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    af782f339a5d ("tpm: Move tpm_vendor_specific data related with PTP specification to tpm_chip")
    c659af78eb7b ("tpm: Check size of response before accessing data")
    ddab0e34288a ("tpm/st33zp24: Remove unneeded tpm_reg in get_burstcount")
    e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
    f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

[PATCH v2 02/12] ima: Remove semicolon at the end of ima_get_binary_runtime_size()

[Roberto Sassu]

This patch removes the unnecessary semicolon at the end of
ima_get_binary_runtime_size().

Cc: stable@vger.kernel.org
Fixes: d158847ae89a2 ("ima: maintain memory size needed for serializing the measurement list")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index fb4ec270f620..c096ef8945c7 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -133,7 +133,7 @@ unsigned long ima_get_binary_runtime_size(void)
 		return ULONG_MAX;
 	else
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);
-};
+}
 
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
-- 
2.27.GIT

[PATCH v2 03/12] evm: Check size of security.evm before using it

[Roberto Sassu]

This patch checks the size for the EVM_IMA_XATTR_DIGSIG and
EVM_XATTR_PORTABLE_DIGSIG types to ensure that the algorithm is read from
the buffer returned by vfs_getxattr_alloc().

Cc: stable@vger.kernel.org # 4.19.x
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0d36259b690d..e4b47759ba1c 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -181,6 +181,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		/* accept xattr with non-empty signature field */
+		if (xattr_len <= sizeof(struct signature_v2_hdr)) {
+			evm_status = INTEGRITY_FAIL;
+			goto out;
+		}
+
 		hdr = (struct signature_v2_hdr *)xattr_data;
 		digest.hdr.algo = hdr->hash_algo;
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
-- 
2.27.GIT

[PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Roberto Sassu]

evm_inode_init_security() requires the HMAC key to calculate the HMAC on
initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded()
check, the function continues even if the HMAC key is not loaded
(evm_key_loaded() returns true also if EVM has been initialized only with a
public key). If the HMAC key is not loaded, evm_inode_init_security()
returns an error later when it calls evm_init_hmac().

Thus, this patch replaces the evm_key_loaded() check with a check of the
EVM_INIT_HMAC flag in evm_initialized, so that evm_inode_init_security()
returns 0 instead of an error.

Cc: stable@vger.kernel.org # 4.5.x
Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index e4b47759ba1c..4e9f5e8b21d5 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -527,7 +527,8 @@ int evm_inode_init_security(struct inode *inode,
 	struct evm_xattr *xattr_data;
 	int rc;
 
-	if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
+	if (!(evm_initialized & EVM_INIT_HMAC) ||
+	    !evm_protected_xattr(lsm_xattr->name))
 		return 0;
 
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
-- 
2.27.GIT

Re: [PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 26ddabfe96bb ("evm: enable EVM when X509 certificate is loaded").

The bot has tested the following trees: v5.8.7, v5.4.63, v4.19.143, v4.14.196, v4.9.235.

v5.8.7: Build OK!
v5.4.63: Build OK!
v4.19.143: Build OK!
v4.14.196: Failed to apply! Possible dependencies:
    21af76631476 ("EVM: turn evm_config_xattrnames into a list")
    5feeb61183dd ("evm: Allow non-SHA1 digital signatures")
    650b29dbdf2c ("integrity: Introduce struct evm_xattr")
    ae1ba1676b88 ("EVM: Allow userland to permit modification of EVM-protected metadata")
    b33e3cc5c90b ("Merge branch 'next-integrity' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security")
    f00d79750712 ("EVM: Allow userspace to signal an RSA key has been loaded")

v4.9.235: Failed to apply! Possible dependencies:
    21af76631476 ("EVM: turn evm_config_xattrnames into a list")
    5feeb61183dd ("evm: Allow non-SHA1 digital signatures")
    650b29dbdf2c ("integrity: Introduce struct evm_xattr")
    ae1ba1676b88 ("EVM: Allow userland to permit modification of EVM-protected metadata")
    b33e3cc5c90b ("Merge branch 'next-integrity' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security")
    b4bfec7f4a86 ("security/integrity: Harden against malformed xattrs")
    f00d79750712 ("EVM: Allow userspace to signal an RSA key has been loaded")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-09-04 at 11:23 +0200, Roberto Sassu wrote:
> evm_inode_init_security() requires the HMAC key to calculate the HMAC on
> initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded()
> check, the function continues even if the HMAC key is not loaded
> (evm_key_loaded() returns true also if EVM has been initialized only with a
> public key). If the HMAC key is not loaded, evm_inode_init_security()
> returns an error later when it calls evm_init_hmac().

This is all true, but the context for why it wasn't an issue previously
is missing.

The original usecase for allowing signature verificaton prior to
loading the HMAC key was a fully signed, possibly immutable, initrd. 
No new files were created or, at least, were in policy until the HMAC
key was loaded.   More recently support for requiring an EVM HMAC key
was removed.  Files having a portable and immutable signature were
given additional privileges.

Please update the patch description with the context of what has
changed.

Mimi

[PATCH v2 05/12] evm: Load EVM key in ima_load_x509() to avoid appraisal

[Roberto Sassu]

Public keys do not need to be appraised by IMA as the restriction on the
IMA/EVM keyrings ensures that a key is loaded only if it is signed with a
key in the primary or secondary keyring.

However, when evm_load_x509() is loaded, appraisal is already enabled and
a valid IMA signature must be added to the EVM key to pass verification.

Since the restriction is applied on both IMA and EVM keyrings, it is safe
to disable appraisal also when the EVM key is loaded. This patch calls
evm_load_x509() inside ima_load_x509() if CONFIG_IMA_LOAD_X509 is defined.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/iint.c         | 2 ++
 security/integrity/ima/ima_init.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..7d08c31c612f 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -200,7 +200,9 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 void __init integrity_load_keys(void)
 {
 	ima_load_x509();
+#ifndef CONFIG_IMA_LOAD_X509
 	evm_load_x509();
+#endif
 }
 
 static int __init integrity_fs_init(void)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 4902fe7bd570..9d29a1680da8 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -106,6 +106,10 @@ void __init ima_load_x509(void)
 
 	ima_policy_flag &= ~unset_flags;
 	integrity_load_x509(INTEGRITY_KEYRING_IMA, CONFIG_IMA_X509_PATH);
+
+	/* load also EVM key to avoid appraisal */
+	evm_load_x509();
+
 	ima_policy_flag |= unset_flags;
 }
 #endif
-- 
2.27.GIT

[PATCH v2 06/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Roberto Sassu]

EVM_ALLOW_METADATA_WRITES is an EVM initialization flag that can be set to
temporarily disable metadata verification until all xattrs/attrs necessary
to verify an EVM portable signature are copied to the file. This flag is
cleared when EVM is initialized with an HMAC key, to avoid that the HMAC is
calculated on unverified xattrs/attrs.

Currently EVM unnecessarily denies setting this flag if EVM is initialized
with public key, which is not a concern as it cannot be used to trust
xattrs/attrs updates. This patch removes this limitation.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 Documentation/ABI/testing/evm      | 6 ++++--
 security/integrity/evm/evm_secfs.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
index 201d10319fa1..cbb50ab09c78 100644
--- a/Documentation/ABI/testing/evm
+++ b/Documentation/ABI/testing/evm
@@ -42,8 +42,10 @@ Description:
 		modification of EVM-protected metadata and
 		disable all further modification of policy
 
-		Note that once a key has been loaded, it will no longer be
-		possible to enable metadata modification.
+		Note that once HMAC validation and creation is enabled,
+		it will no longer be possible to enable metadata modification
+		and if metadata modification is already enabled, it will be
+		disabled.
 
 		Until key loading has been signaled EVM can not create
 		or validate the 'security.evm' xattr, but returns
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index cfc3075769bb..92fe26ace797 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
 	 * keys are loaded.
 	 */
 	if ((i & EVM_ALLOW_METADATA_WRITES) &&
-	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
+	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
 	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
 		return -EPERM;
 
-- 
2.27.GIT

[PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Roberto Sassu]

When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when
they are protected by a portable signature, until the HMAC key is loaded.

However, IMA is not notified about metadata changes and, after the first
successful appraisal, always allows access to the files without checking
metadata again.

This patch introduces the new atomic flag EVM_RESET_STATUS in
integrity_iint_cache that is set in the EVM post hooks and cleared in
evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
it is set, it clears the appraisal flags.

Although the flag could be cleared also by evm_inode_setxattr() and
evm_inode_setattr() before IMA sees it, this does not happen if
EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
evm_verifyxattr(), this ensures that IMA always sees the flag set before it
is cleared.

This patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() so that EVM won't return the cached status the
next time appraisal is performed.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 17 +++++++++++++++--
 security/integrity/ima/ima_main.c |  8 ++++++--
 security/integrity/integrity.h    |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 4e9f5e8b21d5..05be1ad3e6f3 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		evm_status = (rc == -ENODATA) ?
 				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
 out:
-	if (iint)
+	if (iint) {
+		/*
+		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
+		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
+		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
+		 */
+		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
 		iint->evm_status = evm_status;
+	}
 	kfree(xattr_data);
 	return evm_status;
 }
@@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
 	struct integrity_iint_cache *iint;
 
 	iint = integrity_iint_find(inode);
-	if (iint)
+	if (iint) {
+		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
+
 		iint->evm_status = INTEGRITY_UNKNOWN;
+	}
 }
 
 /**
@@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 	if (!evm_key_loaded())
 		return;
 
+	evm_reset_status(dentry->d_inode);
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..bb9976dc2b74 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,8 +246,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	mutex_lock(&iint->mutex);
 
-	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
-		/* reset appraisal flags if ima_inode_post_setattr was called */
+	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags) ||
+	    test_bit(EVM_RESET_STATUS, &iint->atomic_flags))
+		/*
+		 * Reset appraisal flags if ima_inode_post_setattr was called or
+		 * EVM reset its status and metadata modification was enabled.
+		 */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
 				 IMA_ACTION_FLAGS);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 413c803c5208..2adec51c0f6e 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -70,6 +70,7 @@
 #define IMA_CHANGE_ATTR		2
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
+#define EVM_RESET_STATUS	5
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
-- 
2.27.GIT

Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Mimi Zohar]

[Cc'ing John Johansen]

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> successful appraisal, always allows access to the files without checking
> metadata again.
> 
> This patch introduces the new atomic flag EVM_RESET_STATUS in
> integrity_iint_cache that is set in the EVM post hooks and cleared in
> evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
> it is set, it clears the appraisal flags.
> 
> Although the flag could be cleared also by evm_inode_setxattr() and
> evm_inode_setattr() before IMA sees it, this does not happen if
> EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> is cleared.
> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
>  security/integrity/ima/ima_main.c |  8 ++++++--
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 4e9f5e8b21d5..05be1ad3e6f3 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		evm_status = (rc == -ENODATA) ?
>  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
>  out:
> -	if (iint)
> +	if (iint) {
> +		/*
> +		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
> +		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
> +		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
> +		 */
> +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
>  		iint->evm_status = evm_status;

True IMA is currently the only caller of evm_verifyxattr() in the
upstreamed kernel, but it is an exported function, which may be called
from elsewhere.  The previous version crossed the boundary between EVM
& IMA with EVM modifying the IMA flag directly.  This version assumes
that IMA will be the only caller.  Otherwise, I like this version.

Mimi

> +	}
>  	kfree(xattr_data);
>  	return evm_status;
>  }
> @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**
> @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }

RE: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 2:01 PM
> [Cc'ing John Johansen]
> 
> Hi Roberto,
> 
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > successful appraisal, always allows access to the files without checking
> > metadata again.
> >
> > This patch introduces the new atomic flag EVM_RESET_STATUS in
> > integrity_iint_cache that is set in the EVM post hooks and cleared in
> > evm_verify_hmac(). IMA checks the new flag in process_measurement()
> and if
> > it is set, it clears the appraisal flags.
> >
> > Although the flag could be cleared also by evm_inode_setxattr() and
> > evm_inode_setattr() before IMA sees it, this does not happen if
> > EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> > evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> > is cleared.
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> >  security/integrity/ima/ima_main.c |  8 ++++++--
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -221,8 +221,15 @@ static enum integrity_status
> evm_verify_hmac(struct dentry *dentry,
> >  		evm_status = (rc == -ENODATA) ?
> >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> >  out:
> > -	if (iint)
> > +	if (iint) {
> > +		/*
> > +		 * EVM_RESET_STATUS can be cleared only by
> evm_verifyxattr()
> > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> guarantees that
> > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> cleared.
> > +		 */
> > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> >  		iint->evm_status = evm_status;
> 
> True IMA is currently the only caller of evm_verifyxattr() in the
> upstreamed kernel, but it is an exported function, which may be called
> from elsewhere.  The previous version crossed the boundary between EVM
> & IMA with EVM modifying the IMA flag directly.  This version assumes
> that IMA will be the only caller.  Otherwise, I like this version.

Ok, I think it is better, as you suggested, to export a new EVM function
that tells if evm_reset_status() will be executed in the EVM post hooks, and
to call this function from IMA. IMA would then call ima_reset_appraise_flags()
also depending on the result of the new EVM function.

ima_reset_appraise_flags() should be called in a post hook in IMA.
Should I introduce it?

Thanks

Roberto

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

> Mimi
> 
> > +	}
> >  	kfree(xattr_data);
> >  	return evm_status;
> >  }
> > @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> >  	struct integrity_iint_cache *iint;
> >
> >  	iint = integrity_iint_find(inode);
> > -	if (iint)
> > +	if (iint) {
> > +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > +
> >  		iint->evm_status = INTEGRITY_UNKNOWN;
> > +	}
> >  }
> >
> >  /**
> > @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> >  	if (!evm_key_loaded())
> >  		return;
> >
> > +	evm_reset_status(dentry->d_inode);
> > +
> >  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> >  		evm_update_evmxattr(dentry, NULL, NULL, 0);
> >  }

Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Mimi Zohar]

On Thu, 2020-09-17 at 17:36 +0000, Roberto Sassu wrote:
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -221,8 +221,15 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > >  		evm_status = (rc == -ENODATA) ?
> > >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > >  out:
> > > -	if (iint)
> > > +	if (iint) {
> > > +		/*
> > > +		 * EVM_RESET_STATUS can be cleared only by
> > evm_verifyxattr()
> > > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> > guarantees that
> > > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> > cleared.
> > > +		 */
> > > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > >  		iint->evm_status = evm_status;
> > 
> > True IMA is currently the only caller of evm_verifyxattr() in the
> > upstreamed kernel, but it is an exported function, which may be called
> > from elsewhere.  The previous version crossed the boundary between EVM
> > & IMA with EVM modifying the IMA flag directly.  This version assumes
> > that IMA will be the only caller.  Otherwise, I like this version.
> 
> Ok, I think it is better, as you suggested, to export a new EVM function
> that tells if evm_reset_status() will be executed in the EVM post hooks, and
> to call this function from IMA. IMA would then call ima_reset_appraise_flags()
> also depending on the result of the new EVM function.
> 
> ima_reset_appraise_flags() should be called in a post hook in IMA.
> Should I introduce it?

Yes, so any callers of evm_verifyxattr() will need to implement the
post hook as well.  As much as possible, please limit code duplication.

The last time I looked, there didn't seem to be a locking concern, but
please make sure.

thanks,

Mimi

[PATCH v2 08/12] evm: Allow xattr/attr operations for portable signatures if check fails

[Roberto Sassu]

If files with portable signatures are copied from one location to another
or are extracted from an archive, verification can temporarily fail until
all xattrs/attrs are set in the destination. Only portable signatures may
be moved or copied from one file to another, as they don't depend on
system-specific information such as the inode generation. Instead portable
signatures must include security.ima.

Unlike other security.evm types, EVM portable signatures are also
immutable. Thus, it wouldn't be a problem to allow xattr/attr operations
when verification fails, as portable signatures will never be replaced with
an HMAC on possibly corrupted xattrs/attrs.

This patch first introduces a new integrity status called
INTEGRITY_FAIL_IMMUTABLE, that allows callers of
evm_verify_current_integrity() to detect that a portable signature didn't
pass verification and then adds an exception in evm_protect_xattr() and
evm_inode_setattr() for this status and returns 0 instead of -EPERM.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/linux/integrity.h             |  1 +
 security/integrity/evm/evm_main.c     | 31 +++++++++++++++++++++------
 security/integrity/ima/ima_appraise.c |  2 ++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index 2271939c5c31..2ea0f2f65ab6 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -13,6 +13,7 @@ enum integrity_status {
 	INTEGRITY_PASS = 0,
 	INTEGRITY_PASS_IMMUTABLE,
 	INTEGRITY_FAIL,
+	INTEGRITY_FAIL_IMMUTABLE,
 	INTEGRITY_NOLABEL,
 	INTEGRITY_NOXATTRS,
 	INTEGRITY_UNKNOWN,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 05be1ad3e6f3..a5dab1ac9374 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -27,7 +27,8 @@
 int evm_initialized;
 
 static const char * const integrity_status_msg[] = {
-	"pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown"
+	"pass", "pass_immutable", "fail", "fail_immutable", "no_label",
+	"no_xattrs", "unknown"
 };
 int evm_hmac_attrs;
 
@@ -134,7 +135,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	struct evm_digest digest;
 	struct inode *inode;
-	int rc, xattr_len;
+	int rc, xattr_len, evm_immutable = 0;
 
 	if (iint && (iint->evm_status == INTEGRITY_PASS ||
 		     iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
@@ -179,8 +180,10 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		if (rc)
 			rc = -EINVAL;
 		break;
-	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		evm_immutable = 1;
+		fallthrough;
+	case EVM_IMA_XATTR_DIGSIG:
 		/* accept xattr with non-empty signature field */
 		if (xattr_len <= sizeof(struct signature_v2_hdr)) {
 			evm_status = INTEGRITY_FAIL;
@@ -217,9 +220,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		break;
 	}
 
-	if (rc)
-		evm_status = (rc == -ENODATA) ?
-				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
+	if (rc) {
+		evm_status = INTEGRITY_NOXATTRS;
+		if (rc != -ENODATA)
+			evm_status = evm_immutable ?
+				     INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL;
+	}
 out:
 	if (iint) {
 		/*
@@ -358,6 +364,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 				    -EPERM, 0);
 	}
 out:
+	/* Writing other xattrs is safe for portable signatures, as portable
+	 * signatures are immutable and can never be updated.
+	 */
+	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -499,9 +511,14 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
 		return 0;
 	evm_status = evm_verify_current_integrity(dentry);
+	/* Writing attrs is safe for portable signatures, as portable signatures
+	 * are immutable and can never be updated.
+	 */
 	if ((evm_status == INTEGRITY_PASS) ||
-	    (evm_status == INTEGRITY_NOXATTRS))
+	    (evm_status == INTEGRITY_NOXATTRS) ||
+	    (evm_status == INTEGRITY_FAIL_IMMUTABLE))
 		return 0;
+
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
 			    integrity_status_msg[evm_status], -EPERM, 0);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index b8848f53c8cc..4d682bc3a77f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -399,6 +399,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
 		cause = "missing-HMAC";
 		goto out;
+	case INTEGRITY_FAIL_IMMUTABLE:
+		fallthrough;
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
 		goto out;
-- 
2.27.GIT

Re: [PATCH v2 08/12] evm: Allow xattr/attr operations for portable signatures if check fails

[Mimi Zohar]

Hi Roberto,

"if check fails" in the Subject line is unnecessary.

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> If files with portable signatures are copied from one location to another
> or are extracted from an archive, verification can temporarily fail until
> all xattrs/attrs are set in the destination. Only portable signatures may
> be moved or copied from one file to another, as they don't depend on
> system-specific information such as the inode generation. Instead portable
> signatures must include security.ima.
> 
> Unlike other security.evm types, EVM portable signatures are also
> immutable. Thus, it wouldn't be a problem to allow xattr/attr operations
> when verification fails, as portable signatures will never be replaced with
> an HMAC on possibly corrupted xattrs/attrs.
> 
> This patch first introduces a new integrity status called
> INTEGRITY_FAIL_IMMUTABLE, that allows callers of
> evm_verify_current_integrity() to detect that a portable signature didn't
> pass verification and then adds an exception in evm_protect_xattr() and
> evm_inode_setattr() for this status and returns 0 instead of -EPERM.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

< snip >

> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 05be1ad3e6f3..a5dab1ac9374 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> 
> @@ -358,6 +364,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
>  				    -EPERM, 0);
>  	}
>  out:
> +	/* Writing other xattrs is safe for portable signatures, as portable
> +	 * signatures are immutable and can never be updated.
> +	 */

This is the second time I'm seeing this comment format style.   Why? 
What changed?

Mimi

> +	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> +		return 0;
> +
>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  				    dentry->d_name.name, "appraise_metadata",

[PATCH v2 09/12] evm: Allow setxattr() and setattr() if metadata digest won't change

[Roberto Sassu]

With the patch to allow xattr/attr operations if a portable signature
verification fails, cp and tar can copy all xattrs/attrs so that at the
end of the process verification succeeds.

However, it might happen that xattrs/attrs are already set to the correct
value (taken at signing time) and signature verification succeeds before
the copy is completed. For example, an archive might contains files owned
by root and the archive is extracted by root.

Then, since portable signatures are immutable, all subsequent operations
fail (e.g. fchown()), even if the operation is legitimate (does not alter
the current value).

This patch avoids this problem by reporting successful operation to user
space when that operation does not alter the current value of xattrs/attrs.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 94 +++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index a5dab1ac9374..f43780ae8ae4 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -18,6 +18,7 @@
 #include <linux/integrity.h>
 #include <linux/evm.h>
 #include <linux/magic.h>
+#include <linux/posix_acl_xattr.h>
 
 #include <crypto/hash.h>
 #include <crypto/hash_info.h>
@@ -314,6 +315,78 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 	return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
 }
 
+/*
+ * evm_xattr_acl_change - check if passed ACL changes the inode mode
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed ACL changes the inode mode, which is protected by EVM.
+ *
+ * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
+ */
+static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
+				const void *xattr_value, size_t xattr_value_len)
+{
+	umode_t mode;
+	struct posix_acl *acl = NULL, *acl_res;
+	struct inode *inode = d_backing_inode(dentry);
+	int rc;
+
+	/* UID/GID in ACL have been already converted from user to init ns */
+	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	if (!acl)
+		return 1;
+
+	acl_res = acl;
+	rc = posix_acl_update_mode(inode, &mode, &acl_res);
+
+	posix_acl_release(acl);
+
+	if (rc)
+		return 1;
+
+	if (acl_res && inode->i_mode != mode)
+		return 1;
+
+	return 0;
+}
+
+/*
+ * evm_xattr_change - check if passed xattr value differs from current value
+ * @dentry: pointer to the affected dentry
+ * @xattr_name: requested xattr
+ * @xattr_value: requested xattr value
+ * @xattr_value_len: requested xattr value length
+ *
+ * Check if passed xattr value differs from current value.
+ *
+ * Returns 1 if passed xattr value differs from current value, 0 otherwise.
+ */
+static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
+			    const void *xattr_value, size_t xattr_value_len)
+{
+	char *xattr_data = NULL;
+	int rc = 0;
+
+	if (posix_xattr_acl(xattr_name))
+		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
+					    xattr_value_len);
+
+	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
+	if (rc < 0)
+		return 1;
+
+	if (rc == xattr_value_len)
+		rc = memcmp(xattr_value, xattr_data, rc);
+	else
+		rc = 1;
+
+	kfree(xattr_data);
+	return rc;
+}
+
 /*
  * evm_protect_xattr - protect the EVM extended attribute
  *
@@ -370,6 +443,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
+		return 0;
+
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -490,6 +567,19 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
 
+static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
+{
+	struct inode *inode = d_backing_inode(dentry);
+	unsigned int ia_valid = attr->ia_valid;
+
+	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
+	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
+	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
+		return 0;
+
+	return 1;
+}
+
 /**
  * evm_inode_setattr - prevent updating an invalid EVM extended attribute
  * @dentry: pointer to the affected dentry
@@ -519,6 +609,10 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	    (evm_status == INTEGRITY_FAIL_IMMUTABLE))
 		return 0;
 
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_attr_change(dentry, attr))
+		return 0;
+
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",
 			    integrity_status_msg[evm_status], -EPERM, 0);
-- 
2.27.GIT

Re: [PATCH v2 09/12] evm: Allow setxattr() and setattr() if metadata digest won't change

[Mimi Zohar]

Hi Roberto,

"if" doesn't belong in the subject line.  In this case, instead of "if
metadata ...", how about something like "for unmodified metadata"?

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> With the patch to allow xattr/attr operations if a portable signature
> verification fails, cp and tar can copy all xattrs/attrs so that at the
> end of the process verification succeeds.
> 
> However, it might happen that xattrs/attrs are already set to the correct

^ the xattrs/attrs

> value (taken at signing time) and signature verification succeeds before
> the copy is completed. For example, an archive might contains files owned

^ has completed.

> by root and the archive is extracted by root.
> 
> Then, since portable signatures are immutable, all subsequent operations
> fail (e.g. fchown()), even if the operation is legitimate (does not alter
> the current value).
> 
> This patch avoids this problem by reporting successful operation to user
> space when that operation does not alter the current value of xattrs/attrs.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

<snip>

> +/*
> + * evm_xattr_change - check if passed xattr value differs from current value
> + * @dentry: pointer to the affected dentry
> + * @xattr_name: requested xattr
> + * @xattr_value: requested xattr value
> + * @xattr_value_len: requested xattr value length
> + *
> + * Check if passed xattr value differs from current value.
> + *
> + * Returns 1 if passed xattr value differs from current value, 0 otherwise.
> + */
> +static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
> +			    const void *xattr_value, size_t xattr_value_len)
> +{
> +	char *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (posix_xattr_acl(xattr_name))
> +		return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
> +					    xattr_value_len);
> +
> +	rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
> +	if (rc < 0)
> +		return 1;
> +
> +	if (rc == xattr_value_len)
> +		rc = memcmp(xattr_value, xattr_data, rc);

This should probably be changed to crypto_memneq().   Refer to commit
613317bd212c ("EVM: Use crypto_memneq() for digest comparisons").

thanks,

Mimi

> +	else
> +		rc = 1;
> +
> +	kfree(xattr_data);
> +	return rc;
> +}
> +
> 

[PATCH v2 10/12] ima: Allow imasig requirement to be satisfied by EVM portable signatures

[Roberto Sassu]

System administrators can require that all accessed files have a signature
by specifying appraise_type=imasig in a policy rule.

Currently, IMA signatures satisfy this requirement. Appended signatures may
also satisfy this requirement, but are not applicable as IMA signatures.
IMA/appended signatures ensure data source authentication for file content
and prevent any change. EVM signatures instead ensure data source
authentication for file metadata. Given that the digest or signature of the
file content must be included in the metadata, EVM signatures provide the
same file data guarantees of IMA signatures, as well as providing file
metadata guarantees.

This patch lets systems protected with EVM signatures pass appraisal
verification if the appraise_type=imasig requirement is specified in the
policy. This facilitates deployment in the scenarios where only EVM
signatures are available.

The patch makes the following changes:

file xattr types:
security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG
security.evm: EVM_XATTR_PORTABLE_DIGSIG

execve(), mmap(), open() behavior (with appraise_type=imasig):
before: denied (file without IMA signature, imasig requirement not met)
after: allowed (file with EVM portable signature, imasig requirement met)

open(O_WRONLY) behavior (without appraise_type=imasig):
before: allowed (file without IMA signature, not immutable)
after: denied (file with EVM portable signature, immutable)

In addition, similarly to IMA signatures, this patch temporarily allows
new files without or with incomplete metadata to be opened so that content
can be written.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 4d682bc3a77f..95c7a1fc0d01 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -225,12 +225,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		hash_start = 1;
 		fallthrough;
 	case IMA_XATTR_DIGEST:
-		if (iint->flags & IMA_DIGSIG_REQUIRED) {
-			*cause = "IMA-signature-required";
-			*status = INTEGRITY_FAIL;
-			break;
+		if (*status != INTEGRITY_PASS_IMMUTABLE) {
+			if (iint->flags & IMA_DIGSIG_REQUIRED) {
+				*cause = "IMA-signature-required";
+				*status = INTEGRITY_FAIL;
+				break;
+			}
+			clear_bit(IMA_DIGSIG, &iint->atomic_flags);
+		} else {
+			set_bit(IMA_DIGSIG, &iint->atomic_flags);
 		}
-		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 		if (xattr_len - sizeof(xattr_value->type) - hash_start >=
 				iint->ima_hash->length)
 			/*
@@ -400,6 +404,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		cause = "missing-HMAC";
 		goto out;
 	case INTEGRITY_FAIL_IMMUTABLE:
+		set_bit(IMA_DIGSIG, &iint->atomic_flags);
 		fallthrough;
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
@@ -444,9 +449,12 @@ int ima_appraise_measurement(enum ima_hooks func,
 				status = INTEGRITY_PASS;
 		}
 
-		/* Permit new files with file signatures, but without data. */
+		/*
+		 * Permit new files with file/EVM portable signatures, but
+		 * without data.
+		 */
 		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
-		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
 			status = INTEGRITY_PASS;
 		}
 
-- 
2.27.GIT

[PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback

[Roberto Sassu]

With the patch to accept EVM portable signatures when the
appraise_type=imasig requirement is specified in the policy, appraisal can
be successfully done even if the file does not have an IMA signature.

However, remote attestation would not see that a different signature type
was used, as only IMA signatures can be included in the measurement list.
This patch solves the issue by introducing the new template field 'evmsig'
to show EVM portable signatures and by including its value in the existing
field 'sig' if the IMA signature is not found.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/security/IMA-templates.rst  |  4 ++-
 security/integrity/ima/ima_template.c     |  2 ++
 security/integrity/ima/ima_template_lib.c | 39 ++++++++++++++++++++++-
 security/integrity/ima/ima_template_lib.h |  2 ++
 4 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index c5a8432972ef..9f3e86ab028a 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -70,9 +70,11 @@ descriptors by adding their identifier to the format string
    prefix is shown only if the hash algorithm is not SHA1 or MD5);
  - 'd-modsig': the digest of the event without the appended modsig;
  - 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature;
+ - 'sig': the file signature, or the EVM portable signature if the file
+   signature is not found;
  - 'modsig' the appended file signature;
  - 'buf': the buffer data that was used to generate the hash without size limitations;
+ - 'evmsig': the EVM portable signature;
 
 
 Below, there is the list of defined template descriptors:
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 1e89e2d3851f..02afc4116606 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -45,6 +45,8 @@ static const struct ima_template_field supported_fields[] = {
 	 .field_show = ima_show_template_digest_ng},
 	{.field_id = "modsig", .field_init = ima_eventmodsig_init,
 	 .field_show = ima_show_template_sig},
+	{.field_id = "evmsig", .field_init = ima_eventevmsig_init,
+	 .field_show = ima_show_template_sig},
 };
 
 /*
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index c022ee9e2a4e..2c596c2a89cc 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -10,6 +10,7 @@
  */
 
 #include "ima_template_lib.h"
+#include <linux/xattr.h>
 
 static bool ima_template_hash_algo_allowed(u8 algo)
 {
@@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
 	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
 
 	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
-		return 0;
+		return ima_eventevmsig_init(event_data, field_data);
 
 	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
 					     DATA_FMT_HEX, field_data);
@@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data *event_data,
 	return ima_write_template_field_data(data, data_len, DATA_FMT_HEX,
 					     field_data);
 }
+
+/*
+ *  ima_eventevmsig_init - include the EVM portable signature as part of the
+ *  template data
+ */
+int ima_eventevmsig_init(struct ima_event_data *event_data,
+			 struct ima_field_data *field_data)
+{
+	struct evm_ima_xattr_data *xattr_data = NULL;
+	int rc = 0;
+
+	if (!event_data->file)
+		return 0;
+
+	if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
+		return 0;
+
+	rc = vfs_getxattr_alloc(file_dentry(event_data->file), XATTR_NAME_EVM,
+				(char **)&xattr_data, 0, GFP_NOFS);
+	if (rc <= 0) {
+		if (!rc || rc == -ENODATA)
+			return 0;
+
+		return rc;
+	}
+
+	if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
+		kfree(xattr_data);
+		return 0;
+	}
+
+	rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
+					   field_data);
+	kfree(xattr_data);
+	return rc;
+}
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index 6b3b880637a0..f4b2a2056d1d 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -46,4 +46,6 @@ int ima_eventbuf_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data);
 int ima_eventmodsig_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data);
+int ima_eventevmsig_init(struct ima_event_data *event_data,
+			 struct ima_field_data *field_data);
 #endif /* __LINUX_IMA_TEMPLATE_LIB_H */
-- 
2.27.GIT

Re: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> With the patch to accept EVM portable signatures when the
> appraise_type=imasig requirement is specified in the policy, appraisal can
> be successfully done even if the file does not have an IMA signature.
> 
> However, remote attestation would not see that a different signature type
> was used, as only IMA signatures can be included in the measurement list.
> This patch solves the issue by introducing the new template field 'evmsig'
> to show EVM portable signatures and by including its value in the existing
> field 'sig' if the IMA signature is not found.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>

Thank you!   Just a minor comment below.

<snip>

> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index c022ee9e2a4e..2c596c2a89cc 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> 
> @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
>  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
>  
>  	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> -		return 0;
> +		return ima_eventevmsig_init(event_data, field_data);
>  
>  	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
>  					     DATA_FMT_HEX, field_data);
> @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data *event_data,
>  	return ima_write_template_field_data(data, data_len, DATA_FMT_HEX,
>  					     field_data);
>  }
> +
> +/*
> + *  ima_eventevmsig_init - include the EVM portable signature as part of the
> + *  template data
> + */
> +int ima_eventevmsig_init(struct ima_event_data *event_data,
> +			 struct ima_field_data *field_data)
> +{
> +	struct evm_ima_xattr_data *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (!event_data->file)
> +		return 0;
> +
> +	if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> +		return 0;
> +
> +	rc = vfs_getxattr_alloc(file_dentry(event_data->file), XATTR_NAME_EVM,
> +				(char **)&xattr_data, 0, GFP_NOFS);
> +	if (rc <= 0) {
> +		if (!rc || rc == -ENODATA)
> +			return 0;
> +
> +		return rc;

We're including the EVM signature on a best effort basis to help with
attestation.  Do we really care why it failed?   Are we going to act on
it?

Mimi

> +	}
> +
> +	if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
> +		kfree(xattr_data);
> +		return 0;
> +	}
> +
> +	rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
> +					   field_data);
> +	kfree(xattr_data);
> +	return rc;
> +}

RE: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 4:25 PM
> Hi Roberto,
> 
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > With the patch to accept EVM portable signatures when the
> > appraise_type=imasig requirement is specified in the policy, appraisal can
> > be successfully done even if the file does not have an IMA signature.
> >
> > However, remote attestation would not see that a different signature
> type
> > was used, as only IMA signatures can be included in the measurement list.
> > This patch solves the issue by introducing the new template field 'evmsig'
> > to show EVM portable signatures and by including its value in the existing
> > field 'sig' if the IMA signature is not found.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> Thank you!   Just a minor comment below.
> 
> <snip>
> 
> > diff --git a/security/integrity/ima/ima_template_lib.c
> b/security/integrity/ima/ima_template_lib.c
> > index c022ee9e2a4e..2c596c2a89cc 100644
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> >
> > @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data
> *event_data,
> >  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> >
> >  	if ((!xattr_value) || (xattr_value->type !=
> EVM_IMA_XATTR_DIGSIG))
> > -		return 0;
> > +		return ima_eventevmsig_init(event_data, field_data);
> >
> >  	return ima_write_template_field_data(xattr_value, event_data-
> >xattr_len,
> >  					     DATA_FMT_HEX, field_data);
> > @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data
> *event_data,
> >  	return ima_write_template_field_data(data, data_len,
> DATA_FMT_HEX,
> >  					     field_data);
> >  }
> > +
> > +/*
> > + *  ima_eventevmsig_init - include the EVM portable signature as part of
> the
> > + *  template data
> > + */
> > +int ima_eventevmsig_init(struct ima_event_data *event_data,
> > +			 struct ima_field_data *field_data)
> > +{
> > +	struct evm_ima_xattr_data *xattr_data = NULL;
> > +	int rc = 0;
> > +
> > +	if (!event_data->file)
> > +		return 0;
> > +
> > +	if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> > +		return 0;
> > +
> > +	rc = vfs_getxattr_alloc(file_dentry(event_data->file),
> XATTR_NAME_EVM,
> > +				(char **)&xattr_data, 0, GFP_NOFS);
> > +	if (rc <= 0) {
> > +		if (!rc || rc == -ENODATA)
> > +			return 0;
> > +
> > +		return rc;
> 
> We're including the EVM signature on a best effort basis to help with
> attestation.  Do we really care why it failed?   Are we going to act on
> it?

Hi Mimi

other template field functions have a similar behavior. They return
an error if an operation necessary to retrieve the data cannot be
performed. Should I always return 0?

Thanks

Roberto

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

> Mimi
> 
> > +	}
> > +
> > +	if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
> > +		kfree(xattr_data);
> > +		return 0;
> > +	}
> > +
> > +	rc = ima_write_template_field_data((char *)xattr_data, rc,
> DATA_FMT_HEX,
> > +					   field_data);
> > +	kfree(xattr_data);
> > +	return rc;
> > +}
> 

Re: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback

[Mimi Zohar]

On Thu, 2020-09-17 at 15:05 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, September 17, 2020 4:25 PM
> > Hi Roberto,
> > 
> > On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > > With the patch to accept EVM portable signatures when the
> > > appraise_type=imasig requirement is specified in the policy, appraisal can
> > > be successfully done even if the file does not have an IMA signature.
> > >
> > > However, remote attestation would not see that a different signature
> > type
> > > was used, as only IMA signatures can be included in the measurement list.
> > > This patch solves the issue by introducing the new template field 'evmsig'
> > > to show EVM portable signatures and by including its value in the existing
> > > field 'sig' if the IMA signature is not found.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> > 
> > Thank you!   Just a minor comment below.
> > 
> > <snip>
> > 
> > > diff --git a/security/integrity/ima/ima_template_lib.c
> > b/security/integrity/ima/ima_template_lib.c
> > > index c022ee9e2a4e..2c596c2a89cc 100644
> > > --- a/security/integrity/ima/ima_template_lib.c
> > > +++ b/security/integrity/ima/ima_template_lib.c
> > >
> > > @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data
> > *event_data,
> > >  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> > >
> > >  	if ((!xattr_value) || (xattr_value->type !=
> > EVM_IMA_XATTR_DIGSIG))
> > > -		return 0;
> > > +		return ima_eventevmsig_init(event_data, field_data);
> > >
> > >  	return ima_write_template_field_data(xattr_value, event_data-
> > >xattr_len,
> > >  					     DATA_FMT_HEX, field_data);
> > > @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data
> > *event_data,
> > >  	return ima_write_template_field_data(data, data_len,
> > DATA_FMT_HEX,
> > >  					     field_data);
> > >  }
> > > +
> > > +/*
> > > + *  ima_eventevmsig_init - include the EVM portable signature as part of
> > the
> > > + *  template data
> > > + */
> > > +int ima_eventevmsig_init(struct ima_event_data *event_data,
> > > +			 struct ima_field_data *field_data)
> > > +{
> > > +	struct evm_ima_xattr_data *xattr_data = NULL;
> > > +	int rc = 0;
> > > +
> > > +	if (!event_data->file)
> > > +		return 0;
> > > +
> > > +	if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> > > +		return 0;
> > > +
> > > +	rc = vfs_getxattr_alloc(file_dentry(event_data->file),
> > XATTR_NAME_EVM,
> > > +				(char **)&xattr_data, 0, GFP_NOFS);
> > > +	if (rc <= 0) {
> > > +		if (!rc || rc == -ENODATA)
> > > +			return 0;
> > > +
> > > +		return rc;
> > 
> > We're including the EVM signature on a best effort basis to help with
> > attestation.  Do we really care why it failed?   Are we going to act on
> > it?
> 
> Hi Mimi
> 
> other template field functions have a similar behavior. They return
> an error if an operation necessary to retrieve the data cannot be
> performed. Should I always return 0?

The EVM signature case is more similar to the IMA signature case, than
to other fields.  In the signature cases, if the signature exists, it
is included.   My suggestion is based on the difference in how the
vfs_getxattr_alloc() results are handled.

thanks,

Mimi

[PATCH v2 12/12] ima: Don't remove security.ima if file must not be appraised

[Roberto Sassu]

Files might come from a remote source and might have xattrs, including
security.ima. It should not be IMA task to decide whether security.ima
should be kept or not. This patch removes the removexattr() system
call in ima_inode_post_setattr().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 95c7a1fc0d01..2dbf0417f9e6 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -513,8 +513,6 @@ void ima_inode_post_setattr(struct dentry *dentry)
 		return;
 
 	action = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
-	if (!action)
-		__vfs_removexattr(dentry, XATTR_NAME_IMA);
 	iint = integrity_iint_find(inode);
 	if (iint) {
 		set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
-- 
2.27.GIT

Re: [PATCH v2 00/12] IMA/EVM fixes

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-09-04 at 11:23 +0200, Roberto Sassu wrote:
> This patch set includes various fixes for IMA and EVM.
> 
> Patches 1-3 are trivial fixes. 

I've queued these patches in the next-integrity-testing branch for now.
When reposting this patch set please replace the cover letter subject
line with a more appropriate one.

> The remaining improve support and usability
> of EVM portable signatures. In particular patch 4 allows EVM to be used
> without an HMAC key.

EVM already supports using EVM portable and immutable sigatures without
an HMAC key.   

I was able to apply this patch set, but patch 10/12 does not apply
cleanly due to the "fallthrough" line.  Please hold off on reposting,
until I've finished reviewing the entire patch set.

thanks,

Mimi

Re: [PATCH v2 00/12] IMA/EVM fixes

[Mimi Zohar]

Hi Roberto,

On Wed, 2020-09-16 at 12:14 -0400, Mimi Zohar wrote:
> On Fri, 2020-09-04 at 11:23 +0200, Roberto Sassu wrote:
> > This patch set includes various fixes for IMA and EVM.
> > 
> > Patches 1-3 are trivial fixes. 
> 
> I've queued these patches in the next-integrity-testing branch for now.
> When reposting this patch set please replace the cover letter subject
> line with a more appropriate one.
> 
> > The remaining improve support and usability
> > of EVM portable signatures. In particular patch 4 allows EVM to be used
> > without an HMAC key.
> 
> EVM already supports using EVM portable and immutable sigatures without
> an HMAC key.   
> 
> I was able to apply this patch set, but patch 10/12 does not apply
> cleanly due to the "fallthrough" line.  Please hold off on reposting,
> until I've finished reviewing the entire patch set.

Done.  Other than the one issue of clearing the EVM_RESET_STATUS in
evm_verifyxattr(), the remaining changes are straight forward.

thanks,

Mimi