RE: [PATCH] ima: Allow imasig requirement to be satisfied by EVM portable signatures

[Roberto Sassu <roberto.sassu@huawei.com>]

> -----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
> <Silviu.Vlasceanu@huawei.com>
> Subject: RE: [PATCH] ima: Allow imasig requirement to be satisfied by EVM
> portable signatures
> 
> > -----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;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH] ima: Allow imasig requirement to be satisfied by
> EVM
> > portable signatures
> >
> > On Tue, 2020-04-21 at 11:24 +0200, Roberto Sassu wrote:
> > > 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
> > > 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.
> >
> > >
> > > 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
> 
> 
> >  What do you think?
> >
> > > 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)
> > >
> > > 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
> > > 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,
> > >  		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