Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Mimi Zohar <zohar@linux.ibm.com>]

On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> Currently, ima_appraise_measurement() ignores the EVM status when
> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
> security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
> ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
> status. The problem is that the EVM status is overwritten with the
> > appraisal statu

Roberto, your framing of this problem is harsh and misleading.  IMA
and EVM are intentionally independent of each other and can be
configured independently of each other.  The intersection of the two
is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is returned for a
number of reasons - when EVM is not configured, the EVM hmac key has
not yet been loaded, the protected security attribute is unknown, or
the file is not in policy.

This patch does not differentiate between any of the above cases,
requiring mutable files to always be protected by EVM, when specified
as an "ima_appraise=" option on the boot command line.

IMA could be extended to require EVM on a per IMA policy rule basis.  
Instead of framing allowing IMA file hashes without EVM as a bug that
has existed from the very beginning, now that IMA/EVM have matured and
is being used, you could frame it as extending IMA or hardening.

> 
> This patch mitigates the issue by selecting signature verification as the
> only method allowed for appraisal when EVM is not initialized. Since the
> new behavior might break user space, it must be turned on by adding the
> '-evm' suffix to the value of the ima_appraise= kernel option.
> 
> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 3 ++-
>  security/integrity/ima/ima_appraise.c           | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f6664b2e2..d84a2e612b93 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1585,7 +1585,8 @@
>  			Set number of hash buckets for inode cache.
>  
>  	ima_appraise=	[IMA] appraise integrity measurements
> -			Format: { "off" | "enforce" | "fix" | "log" }
> +			Format: { "off" | "enforce" | "fix" | "log" |
> +				  "enforce-evm" | "log-evm" } 

Is it necessary to define both "enforce-evm" and "log-evm"?  Perhaps
defining "require-evm" is sufficient.

Mimi

>  			default: "enforce"
>  
>  	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 5fb7127bbe68..afef06e10fb9 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -18,6 +18,7 @@
>  
>  #include "ima.h"
>  
> +static bool ima_appraise_req_evm __ro_after_init;
>  static int __init default_appraise_setup(char *str)
>  {
>  #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> @@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
>  	else if (strncmp(str, "fix", 3) == 0)
>  		ima_appraise = IMA_APPRAISE_FIX;
>  #endif
> +	if (strcmp(str, "enforce-evm") == 0 ||
> +	    strcmp(str, "log-evm") == 0)
> +		ima_appraise_req_evm = true;
>  	return 1;
>  }
>  
> @@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	switch (status) {
>  	case INTEGRITY_PASS:
>  	case INTEGRITY_PASS_IMMUTABLE:
> +		break;
>  	case INTEGRITY_UNKNOWN:
> +		if (ima_appraise_req_evm &&
> +		    xattr_value->type != EVM_IMA_XATTR_DIGSIG)
> +			goto out;
>  		break;
>  	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
>  	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */