[v3,2/2] ima: add enforce-evm and log-evm modes to strictly check EVM status
diff mbox series

Message ID 20190606112620.26488-3-roberto.sassu@huawei.com
State New
Headers show
Series
  • ima/evm fixes for v5.2
Related show

Commit Message

Roberto Sassu June 6, 2019, 11:26 a.m. UTC
IMA and EVM have been designed as two independent subsystems: the first for
checking the integrity of file data; the second for checking file metadata.
Making them independent allows users to adopt them incrementally.

The point of intersection is in IMA-Appraisal, which calls
evm_verifyxattr() to ensure that security.ima wasn't modified during an
offline attack. The design choice, to ensure incremental adoption, was to
continue appraisal verification if evm_verifyxattr() returns
INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the
kernel configuration, or if the HMAC key has not been loaded yet.

Although this choice appears legitimate, it might not be suitable for
hardened systems, where the administrator expects that access is denied if
there is any error. An attacker could intentionally delete the EVM keys
from the system and set the file digest in security.ima to the actual file
digest so that the final appraisal status is INTEGRITY_PASS.

This patch allows such hardened systems to strictly enforce an access
control policy based on the validity of signatures/HMACs, by introducing
two new values for the ima_appraise= kernel option: enforce-evm and
log-evm.

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(-)

Comments

Mimi Zohar June 7, 2019, 2:24 p.m. UTC | #1
Hi Roberto,

Thank you for updating the patch description.

On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote:
> IMA and EVM have been designed as two independent subsystems: the first for
> checking the integrity of file data; the second for checking file metadata.
> Making them independent allows users to adopt them incrementally.
> 
> The point of intersection is in IMA-Appraisal, which calls
> evm_verifyxattr() to ensure that security.ima wasn't modified during an
> offline attack. The design choice, to ensure incremental adoption, was to
> continue appraisal verification if evm_verifyxattr() returns
> INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the
> kernel configuration, or if the HMAC key has not been loaded yet.
> 
> Although this choice appears legitimate, it might not be suitable for
> hardened systems, where the administrator expects that access is denied if
> there is any error. An attacker could intentionally delete the EVM keys
> from the system and set the file digest in security.ima to the actual file
> digest so that the final appraisal status is INTEGRITY_PASS.

Assuming that the EVM HMAC key is stored in the initramfs, not on some
other file system, and the initramfs is signed, INTEGRITY_UNKNOWN
would be limited to the rootfs filesystem.

> 
> This patch allows such hardened systems to strictly enforce an access
> control policy based on the validity of signatures/HMACs, by introducing
> two new values for the ima_appraise= kernel option: enforce-evm and
> log-evm.
> 

This patch defines a global policy requiring EVM on all filesystems.
I've previously suggested extending the IMA policy to support
enforcing or maybe exempting EVM on a per IMA policy rule basis.  As
seen by the need for an additional patch, included in this patch set,
which defines a temporary random number HMAC key to address
INTEGRITY_UNKNOWN on the rootfs filesystem, exempting certain
filesystems on a per policy rule basis might be simpler and achieve
similar results.

I'd like to hear other people's thoughts on defining a temporary,
random number HMAC key.

thanks,

Mimi
Roberto Sassu June 7, 2019, 2:40 p.m. UTC | #2
On 6/7/2019 4:24 PM, Mimi Zohar wrote:
> Hi Roberto,
> 
> Thank you for updating the patch description.

Hi Mimi

no problem.


> On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote:
>> IMA and EVM have been designed as two independent subsystems: the first for
>> checking the integrity of file data; the second for checking file metadata.
>> Making them independent allows users to adopt them incrementally.
>>
>> The point of intersection is in IMA-Appraisal, which calls
>> evm_verifyxattr() to ensure that security.ima wasn't modified during an
>> offline attack. The design choice, to ensure incremental adoption, was to
>> continue appraisal verification if evm_verifyxattr() returns
>> INTEGRITY_UNKNOWN. This value is returned when EVM is not enabled in the
>> kernel configuration, or if the HMAC key has not been loaded yet.
>>
>> Although this choice appears legitimate, it might not be suitable for
>> hardened systems, where the administrator expects that access is denied if
>> there is any error. An attacker could intentionally delete the EVM keys
>> from the system and set the file digest in security.ima to the actual file
>> digest so that the final appraisal status is INTEGRITY_PASS.
> 
> Assuming that the EVM HMAC key is stored in the initramfs, not on some
> other file system, and the initramfs is signed, INTEGRITY_UNKNOWN
> would be limited to the rootfs filesystem.

There is another issue. The HMAC key, like the public keys, should be
loaded when appraisal is disabled. This means that we have to create a
trusted key at early boot and defer the unsealing.

Roberto
Mimi Zohar June 7, 2019, 3:08 p.m. UTC | #3
On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote:
> > On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote:

> >> Although this choice appears legitimate, it might not be suitable for
> >> hardened systems, where the administrator expects that access is denied if
> >> there is any error. An attacker could intentionally delete the EVM keys
> >> from the system and set the file digest in security.ima to the actual file
> >> digest so that the final appraisal status is INTEGRITY_PASS.
> > 
> > Assuming that the EVM HMAC key is stored in the initramfs, not on some
> > other file system, and the initramfs is signed, INTEGRITY_UNKNOWN
> > would be limited to the rootfs filesystem.
> 
> There is another issue. The HMAC key, like the public keys, should be
> loaded when appraisal is disabled. This means that we have to create a
> trusted key at early boot and defer the unsealing.

There is no need for IMA to appraise the public key file signature,
since the certificate is signed by a key on the builtin/secondary
trusted keyring.  With CONFIG_IMA_LOAD_X509 enabled, the public key
can be loaded onto the IMA keyring with IMA-appraisal enabled, but
without verifying the file signature.

Mimi
Roberto Sassu June 7, 2019, 3:14 p.m. UTC | #4
On 6/7/2019 5:08 PM, Mimi Zohar wrote:
> On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote:
>>> On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote:
> 
>>>> Although this choice appears legitimate, it might not be suitable for
>>>> hardened systems, where the administrator expects that access is denied if
>>>> there is any error. An attacker could intentionally delete the EVM keys
>>>> from the system and set the file digest in security.ima to the actual file
>>>> digest so that the final appraisal status is INTEGRITY_PASS.
>>>
>>> Assuming that the EVM HMAC key is stored in the initramfs, not on some
>>> other file system, and the initramfs is signed, INTEGRITY_UNKNOWN
>>> would be limited to the rootfs filesystem.
>>
>> There is another issue. The HMAC key, like the public keys, should be
>> loaded when appraisal is disabled. This means that we have to create a
>> trusted key at early boot and defer the unsealing.
> 
> There is no need for IMA to appraise the public key file signature,
> since the certificate is signed by a key on the builtin/secondary
> trusted keyring.  With CONFIG_IMA_LOAD_X509 enabled, the public key
> can be loaded onto the IMA keyring with IMA-appraisal enabled, but
> without verifying the file signature.

Yes, but access to the files containing the master key and the EVM key
is denied if appraisal is enabled.

Roberto
Mimi Zohar June 7, 2019, 3:25 p.m. UTC | #5
On Fri, 2019-06-07 at 17:14 +0200, Roberto Sassu wrote:
> On 6/7/2019 5:08 PM, Mimi Zohar wrote:
> > On Fri, 2019-06-07 at 16:40 +0200, Roberto Sassu wrote:
> >>> On Thu, 2019-06-06 at 13:26 +0200, Roberto Sassu wrote:
> > 
> >>>> Although this choice appears legitimate, it might not be suitable for
> >>>> hardened systems, where the administrator expects that access is denied if
> >>>> there is any error. An attacker could intentionally delete the EVM keys
> >>>> from the system and set the file digest in security.ima to the actual file
> >>>> digest so that the final appraisal status is INTEGRITY_PASS.
> >>>
> >>> Assuming that the EVM HMAC key is stored in the initramfs, not on some
> >>> other file system, and the initramfs is signed, INTEGRITY_UNKNOWN
> >>> would be limited to the rootfs filesystem.
> >>
> >> There is another issue. The HMAC key, like the public keys, should be
> >> loaded when appraisal is disabled. This means that we have to create a
> >> trusted key at early boot and defer the unsealing.
> > 
> > There is no need for IMA to appraise the public key file signature,
> > since the certificate is signed by a key on the builtin/secondary
> > trusted keyring.  With CONFIG_IMA_LOAD_X509 enabled, the public key
> > can be loaded onto the IMA keyring with IMA-appraisal enabled, but
> > without verifying the file signature.
> 
> Yes, but access to the files containing the master key and the EVM key
> is denied if appraisal is enabled.

This is a key loading ordering issue.  Assuming you load the IMA key
first, you should be able to verify the master and EVM keys.

Mimi

Patch
diff mbox series

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fe5cde58c11b..0585194ca736 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1587,7 +1587,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" }
 			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. */