[3/4] ima: don't ignore INTEGRITY_UNKNOWN EVM status
diff mbox series

Message ID 20190516161257.6640-3-roberto.sassu@huawei.com
State New
Headers show
Series
  • [1/4] evm: check hash algorithm passed to init_desc()
Related show

Commit Message

Roberto Sassu May 16, 2019, 4:12 p.m. UTC
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 status.

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
ima_appraise_req_evm to the kernel command line.

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           | 12 ++++++++++++
 2 files changed, 15 insertions(+)

Comments

Mimi Zohar May 20, 2019, 9:20 p.m. UTC | #1
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 52e6fbb042cc..80e1c233656b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1588,6 +1588,9 @@
>  			Format: { "off" | "enforce" | "fix" | "log" }
>  			default: "enforce"
>  
> +	ima_appraise_req_evm
> +			[IMA] require EVM for appraisal with file digests.

As much as possible we want to limit the number of new boot command
line options as possible.  Is there a reason for not extending
"ima_appraise=" with "require-evm" or "enforce-evm"?

Mimi

> +
>  	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
>  			The builtin appraise policy appraises all files
>  			owned by uid=0.
Roberto Sassu May 21, 2019, 7:26 a.m. UTC | #2
On 5/20/2019 11:20 PM, Mimi Zohar wrote:
> On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 52e6fbb042cc..80e1c233656b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1588,6 +1588,9 @@
>>   			Format: { "off" | "enforce" | "fix" | "log" }
>>   			default: "enforce"
>>   
>> +	ima_appraise_req_evm
>> +			[IMA] require EVM for appraisal with file digests.
> 
> As much as possible we want to limit the number of new boot command
> line options as possible.  Is there a reason for not extending
> "ima_appraise=" with "require-evm" or "enforce-evm"?

ima-appraise= can be disabled with CONFIG_IMA_APPRAISE_BOOTPARAM, which
probably is done when the system is in production.

Should I allow to use ima-appraise=require-evm even if
CONFIG_IMA_APPRAISE_BOOTPARAM=n?

Thanks

Roberto
Mimi Zohar May 21, 2019, 11:48 a.m. UTC | #3
On Tue, 2019-05-21 at 09:26 +0200, Roberto Sassu wrote:
> On 5/20/2019 11:20 PM, Mimi Zohar wrote:
> > On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 52e6fbb042cc..80e1c233656b 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1588,6 +1588,9 @@
> >>   			Format: { "off" | "enforce" | "fix" | "log" }
> >>   			default: "enforce"
> >>   
> >> +	ima_appraise_req_evm
> >> +			[IMA] require EVM for appraisal with file digests.
> > 
> > As much as possible we want to limit the number of new boot command
> > line options as possible.  Is there a reason for not extending
> > "ima_appraise=" with "require-evm" or "enforce-evm"?
> 
> ima-appraise= can be disabled with CONFIG_IMA_APPRAISE_BOOTPARAM, which
> probably is done when the system is in production.
> 
> Should I allow to use ima-appraise=require-evm even if
> CONFIG_IMA_APPRAISE_BOOTPARAM=n?

Yes, that should be fine.  It's making "ima_appraise" stricter.

Mimi

Patch
diff mbox series

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 52e6fbb042cc..80e1c233656b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1588,6 +1588,9 @@ 
 			Format: { "off" | "enforce" | "fix" | "log" }
 			default: "enforce"
 
+	ima_appraise_req_evm
+			[IMA] require EVM for appraisal with file digests.
+
 	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
 			The builtin appraise policy appraises all files
 			owned by uid=0.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..a32ed5d7afd1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -33,6 +33,14 @@  static int __init default_appraise_setup(char *str)
 
 __setup("ima_appraise=", default_appraise_setup);
 
+static bool ima_appraise_req_evm;
+static int __init appraise_req_evm_setup(char *str)
+{
+	ima_appraise_req_evm = true;
+	return 1;
+}
+__setup("ima_appraise_req_evm", appraise_req_evm_setup);
+
 /*
  * is_ima_appraise_enabled - return appraise status
  *
@@ -245,7 +253,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. */