-----Original Message-----
From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
owner@vger.kernel.org] On Behalf Of Roberto Sassu
Sent: Friday, April 24, 2020 12:40 PM
To: Mimi Zohar <zohar@linux.ibm.com>; mjg59@google.com
Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
linux-kernel@vger.kernel.org; Silviu Vlasceanu
[off-list ref]
Subject: RE: [PATCH] ima: Allow imasig requirement to be satisfied by EVM
portable signatures

quoted

-----Original Message-----
From: Mimi Zohar [mailto:zohar@linux.ibm.com]
Sent: Thursday, April 23, 2020 10:52 PM
To: Roberto Sassu <roberto.sassu@huawei.com>; mjg59@google.com
Cc: linux-integrity@vger.kernel.org; linux-security-

module@vger.kernel.org;

quoted

linux-kernel@vger.kernel.org; Silviu Vlasceanu
[off-list ref]
Subject: Re: [PATCH] ima: Allow imasig requirement to be satisfied by

EVM

quoted

portable signatures

On Tue, 2020-04-21 at 11:24 +0200, Roberto Sassu wrote:

quoted

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

Currently, only IMA signatures satisfy this requirement. However, also

EVM

quoted

portable signatures can satisfy it. Metadata, including security.ima, are
signed and cannot change.

Please expand this paragraph with a short comparison of the security
guarantees provided by EVM immutable, portable signatures versus ima-
sig.

quoted

This patch helps in the scenarios where system administrators want to
enforce this restriction but only EVM portable signatures are available.

Yes, I agree it "helps", but we still need to address the ability of
setting/removing security.ima, which isn't possible with an IMA
signature.  This sounds like we need to define an immutable file hash.

I didn't understand. Can you explain better?

Ok, got it.

I wouldn't grant access to new file depending on the security.ima type
but depending on the IMA_DIGSIG bit. In both cases, IMA signature and
EVM portable signature, the bit is set.

There is one remaining issue. Maybe the signature is portable, but you
don't get it from evm_verifyxattr() if verification fails. There is a legitimate
case when it happens, which is when you extract a file with a portable
signature with tar, and the inode uid/gid are not yet correct (fchown() is
called later after the open()). In this case, IMA_DIGSIG is not set and the
open() fails.

To avoid this issue I would introduce the new status INTEGRITY_FAIL_IMMUTABLE,
so that IMA_DIGSIG is set even if the verification of the portable signature
fails.

Roberto

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

Thanks

Roberto

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

quoted

 What do you think?

quoted

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)

quoted

quoted

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

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_appraise.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c

b/security/integrity/ima/ima_appraise.c

quoted

index a9649b04b9f1..69a6a958f811 100644

--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c

@@ -219,12 +219,16 @@ static int xattr_verify(enum ima_hooks func,

struct integrity_iint_cache *iint,

quoted

 		hash_start = 1;
 		/* fall through */
 	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)
 			/*

Nice!

Mimi