[PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[PATCH 01/11] 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>
---
 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 0d36259b690d..744c105b48d1 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -521,7 +521,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.17.1

[PATCH 02/11] 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>
---
 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 e12c4900510f..4765a266ba96 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -212,7 +212,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.17.1

[PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Roberto Sassu]

Granting metadata write is safe if the HMAC key is not loaded, as it won't
let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key()
however does not allow it if any key is loaded, including a public key,
which should not be a problem.

This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC
flag is not set.

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_secfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.17.1

[PATCH 04/11] 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>
---
 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 744c105b48d1..4e9f5e8b21d5 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.17.1

[PATCH 05/11] 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. Portable signatures are the
only ones that can be moved to different files, as they don't depend on
system-specific information such as the inode generation.

Unlike other security.evm types, portable signatures can never be replaced
even if an xattr/attr operation is granted, as once evm_update_evmxattr()
detects this type, it returns without updating the HMAC. Thus, it wouldn't
be a problem to allow those operations so that verification passes on the
destination after all xattrs/attrs are copied.

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>
---
 include/linux/integrity.h             |  1 +
 security/integrity/evm/evm_main.c     | 25 ++++++++++++++++++++-----
 security/integrity/ima/ima_appraise.c |  1 +
 3 files changed, 22 insertions(+), 5 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 4e9f5e8b21d5..30072030f05d 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;
@@ -219,7 +222,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 
 	if (rc)
 		evm_status = (rc == -ENODATA) ?
-				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
+				INTEGRITY_NOXATTRS : evm_immutable ?
+				INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL;
 out:
 	if (iint)
 		iint->evm_status = evm_status;
@@ -351,6 +355,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 				    -EPERM, 0);
 	}
 out:
+	/* It is safe to allow fail_immutable, portable signatures 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",
@@ -488,9 +498,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);
+	/* It is safe to allow fail_immutable, portable signatures 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 a9649b04b9f1..21bda264fc30 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -393,6 +393,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
 		cause = "missing-HMAC";
 		goto out;
+	case INTEGRITY_FAIL_IMMUTABLE:
 	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
 		cause = "invalid-HMAC";
 		goto out;
-- 
2.17.1

Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Mimi Zohar]

Hi Roberto,

Sorry for the delay in reviewing these patches.   Missing from this
patch set is a cover letter with an explanation for grouping these
patches into a patch set, other than for convenience.  In this case, it
would be along the lines that the original use case for EVM portable
and immutable keys support was for a few critical files, not combined
with an EVM encrypted key type.   This patch set more fully integrates
the initial EVM portable and immutable signature support.

On Thu, 2020-06-18 at 18:01 +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().
> 
> 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 0d36259b690d..744c105b48d1 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -521,7 +521,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);

Re: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal

[Mimi Zohar]

On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> 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>
> ---
>  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 e12c4900510f..4765a266ba96 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -212,7 +212,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

As much as possible IMA and EVM should remain independent of each
other.   Modifying integrity_load_x509() doesn't help.  This looks like
a good reason for calling another EVM function from within IMA.

Mimi

Re: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Mimi Zohar]

Hi Roberto,

On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> Granting metadata write is safe if the HMAC key is not loaded, as it won't
> let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key()
> however does not allow it if any key is loaded, including a public key,
> which should not be a problem.
> 

Why is the existing hebavior a problem?  What is the problem being
solved?

> This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC
> flag is not set.
> 
> 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_secfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;

>  

Documentation/ABI/testing/evm needs to be updated as well.

thanks,

Mimi

Re: [PATCH 04/11] evm: Check size of security.evm before using it

[Mimi Zohar]

On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> 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>

Re: [PATCH 05/11] evm: Allow xattr/attr operations for portable signatures if check fails

[Mimi Zohar]

On Thu, 2020-06-18 at 18:01 +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. Portable signatures are the
> only ones that can be moved to different files, as they don't depend on
> system-specific information such as the inode generation.

^Only portable signatures may be moved or copied from one file to
another, as they ...  Instead portable signatures must include
"security.ima".

> Unlike other security.evm types, portable signatures

^, EVM portable signatures are also immutable.  They

>  can never be replaced
> even if an xattr/attr operation is granted, as once evm_update_evmxattr()
> detects this type, it returns without updating the HMAC. Thus, it wouldn't
> be a problem to allow those operations so that verification passes on the
> destination after all xattrs/attrs are copied.

This needs to be reworded a bit.
> 
> 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>

After this patch, nothing prevents modifying the xattrs after all of
them are in place and the signature verification would be successful. 
(Ok, that is being addressed in subsequent patches.)

> ---
>  include/linux/integrity.h             |  1 +
>  security/integrity/evm/evm_main.c     | 25 ++++++++++++++++++++-----
>  security/integrity/ima/ima_appraise.c |  1 +
>  3 files changed, 22 insertions(+), 5 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 4e9f5e8b21d5..30072030f05d 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;
> @@ -219,7 +222,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  
>  	if (rc)
>  		evm_status = (rc == -ENODATA) ?
> -				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> +				INTEGRITY_NOXATTRS : evm_immutable ?
> +				INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL;

Embedded ternary operator should be replaced with normal C syntax.

>  out:
>  	if (iint)
>  		iint->evm_status = evm_status;
> @@ -351,6 +355,12 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
>  				    -EPERM, 0);
>  	}
>  out:
> +	/* It is safe to allow fail_immutable, portable signatures can never be
> +	 * updated
> +	 */

Replace "It" with "Writing other xattrs".   Writing other xattrs is
safe for portable signatures, as portable signatures are immutable and
...."

> +	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",
> @@ -488,9 +498,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);
> +	/* It is safe to allow fail_immutable, portable signatures can never
> +	 * be updated
> +	 */

Replace "It" with what is safe.

Mimi

>  	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 a9649b04b9f1..21bda264fc30 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -393,6 +393,7 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
>  		cause = "missing-HMAC";
>  		goto out;
> +	case INTEGRITY_FAIL_IMMUTABLE:
>  	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
>  		cause = "invalid-HMAC";
>  		goto out;

Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote:
> Sorry for the delay in reviewing these patches.   Missing from this
> patch set is a cover letter with an explanation for grouping these
> patches into a patch set, other than for convenience.  In this case, it
> would be along the lines that the original use case for EVM portable
> and immutable keys support was for a few critical files, not combined
> with an EVM encrypted key type.   This patch set more fully integrates
> the initial EVM portable and immutable signature support.

Thank you for more fully integrating the EVM portable signatures into
IMA.

" [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM
portable signatures" equates an IMA signature to having a portable and
immutable EVM signature.  That is true in terms of signature
verification, but from an attestation perspective the "ima-sig"
template will not contain a signature.  If not the EVM signature, then
at least some other indication should be included in the measurement
list.

Are you planning on posting the associated IMA/EVM regression tests?

Mimi

RE: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, August 21, 2020 10:15 PM
> Hi Roberto,
> 
> On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > let an attacker obtain a valid HMAC from corrupted xattrs.
> evm_write_key()
> > however does not allow it if any key is loaded, including a public key,
> > which should not be a problem.
> >
> 
> Why is the existing hebavior a problem?  What is the problem being
> solved?

Hi Mimi

currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
only a public key is loaded and the HMAC key is not. The patch removes
this limitation.

> > This patch allows setting EVM_ALLOW_METADATA_WRITES if the
> EVM_INIT_HMAC
> > flag is not set.
> >
> > 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_secfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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;
> 
> >
> 
> Documentation/ABI/testing/evm needs to be updated as well.

Ok.

Thanks

Roberto

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

> thanks,
> 
> Mimi
> 
> 

RE: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, August 21, 2020 8:45 PM
> On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > 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>
> > ---
> >  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 e12c4900510f..4765a266ba96 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -212,7 +212,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
> 
> As much as possible IMA and EVM should remain independent of each
> other.   Modifying integrity_load_x509() doesn't help.  This looks like
> a good reason for calling another EVM function from within IMA.

Can I add your Reviewed-by?

Thanks

Roberto

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

Re: [PATCH 02/11] evm: Load EVM key in ima_load_x509() to avoid appraisal

[Mimi Zohar]

On Mon, 2020-08-31 at 09:44 +0000, Roberto Sassu wrote:
> 
> > As much as possible IMA and EVM should remain independent of each
> > other.   Modifying integrity_load_x509() doesn't help.  This looks like
> > a good reason for calling another EVM function from within IMA.
> 
> Can I add your Reviewed-by?

Yes, that's fine.

Mimi

Re: [PATCH 03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Mimi Zohar]

On Mon, 2020-08-31 at 08:24 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, August 21, 2020 10:15 PM
> > Hi Roberto,
> > 
> > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > > let an attacker obtain a valid HMAC from corrupted xattrs.
> > evm_write_key()
> > > however does not allow it if any key is loaded, including a public key,
> > > which should not be a problem.
> > >
> > 
> > Why is the existing hebavior a problem?  What is the problem being
> > solved?
> 
> Hi Mimi
> 
> currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
> only a public key is loaded and the HMAC key is not. The patch removes
> this limitation.

Yes, I understand.  You're describing "what" the problem is, not "why"
this is a problem.  Support for loading EVM HMAC and x509 certificates
isn't new.  Please add a line or two prior to this paragraph providing
the context for why this is now a problem.

Is the problem related to previoulsy not beginning EVM verification
until after the EVM HMAC key was loaded?  Or perhaps EVM signatures
were not that common since they weren't portable.   Now, with portable
and immutable signatures loading x509 certificates is more common.

thanks,

Mimi

RE: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, August 24, 2020 7:45 PM
> Hi Roberto,
> 
> On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote:
> > Sorry for the delay in reviewing these patches.   Missing from this
> > patch set is a cover letter with an explanation for grouping these
> > patches into a patch set, other than for convenience.  In this case, it
> > would be along the lines that the original use case for EVM portable
> > and immutable keys support was for a few critical files, not combined
> > with an EVM encrypted key type.   This patch set more fully integrates
> > the initial EVM portable and immutable signature support.
> 
> Thank you for more fully integrating the EVM portable signatures into
> IMA.
> 
> " [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM
> portable signatures" equates an IMA signature to having a portable and
> immutable EVM signature.  That is true in terms of signature
> verification, but from an attestation perspective the "ima-sig"
> template will not contain a signature.  If not the EVM signature, then
> at least some other indication should be included in the measurement
> list.

Would it be ok to print the EVM portable signature in the sig field if the IMA
signature is not found? Later we can introduce the new template evm-sig
to include all metadata necessary to verify the EVM portable signature.

> Are you planning on posting the associated IMA/EVM regression tests?

I didn't have a look yet at the code. I will try to write some later.

Roberto

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

Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Mimi Zohar]

On Wed, 2020-09-02 at 11:42 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, August 24, 2020 7:45 PM
> > Hi Roberto,
> > 
> > On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote:
> > > Sorry for the delay in reviewing these patches.   Missing from this
> > > patch set is a cover letter with an explanation for grouping these
> > > patches into a patch set, other than for convenience.  In this case, it
> > > would be along the lines that the original use case for EVM portable
> > > and immutable keys support was for a few critical files, not combined
> > > with an EVM encrypted key type.   This patch set more fully integrates
> > > the initial EVM portable and immutable signature support.
> > 
> > Thank you for more fully integrating the EVM portable signatures into
> > IMA.
> > 
> > " [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM
> > portable signatures" equates an IMA signature to having a portable and
> > immutable EVM signature.  That is true in terms of signature
> > verification, but from an attestation perspective the "ima-sig"
> > template will not contain a signature.  If not the EVM signature, then
> > at least some other indication should be included in the measurement
> > list.
> 
> Would it be ok to print the EVM portable signature in the sig field if the IMA
> signature is not found? Later we can introduce the new template evm-sig
> to include all metadata necessary to verify the EVM portable signature.

As long as the attestation server can differentiate between the
signature types, including the EVM signature should be fine.

> 
> > Are you planning on posting the associated IMA/EVM regression tests?
> 
> I didn't have a look yet at the code. I will try to write some later.

It looks like ima_verify_signature() in ima-evm-utils could be extended
to support the EVM portable signature or at least not to fail the
signature verification.

Mimi