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

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

[Roberto Sassu]

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.

This patch helps in the scenarios where system administrators want to
enforce this restriction but only EVM portable signatures are available.
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)
 			/*
-- 
2.17.1

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

[Mimi Zohar]

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.
 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

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

[Roberto Sassu]

> -----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?

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

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

[Roberto Sassu]

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