Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <64867453bee89d8ed8ec252a56f252ccdf624076.camel@linux.ibm.com>
Subject: Re: [PATCH v5 7/8] ima: support fs-verity file digest based version
 3 signatures
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-integrity@vger.kernel.org,
        Stefan Berger <stefanb@linux.ibm.com>,
        linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Thu, 17 Mar 2022 11:46:24 -0400
In-Reply-To: <YhbePM/BiRCzL3bn@sol.localdomain>
References: <20220211214310.119257-1-zohar@linux.ibm.com>
         <20220211214310.119257-8-zohar@linux.ibm.com>
         <YhbePM/BiRCzL3bn@sol.localdomain>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 2022-02-23 at 17:24 -0800, Eric Biggers wrote:
> On Fri, Feb 11, 2022 at 04:43:09PM -0500, Mimi Zohar wrote:
> > Instead of calculating a regular file hash and verifying the signature
> > stored in the 'security.ima' xattr against the calculated file hash, get
> > fs-verity's file digest and verify the signature (version 3) stored in
> > 'security.ima' against the digest.
> > 
> > The policy rule 'appraise_type=' option is extended to support 'sigv3',
> > which is initiality limited to fs-verity.
> > 
> > The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
> > option and require the 'appraise_type=sigv3' option.  The following
> > 'appraise' policy rule requires fsverity file digests.  (The rule may be
> > constrained, for example based on a fsuuid or LSM label.)
> > 
> > Basic fs-verity policy rule example:
> >   appraise func=BPRM_CHECK digest_type=verity appraise_type=sigv3
> > 
> > Lastly, for IMA to differentiate between the original IMA signature
> > from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
> > is defined.
> 
> I'm having a hard time understanding this patch.  Can you please describe the
> motivation for doing things, not just the things themselves, and make sure the
> explanation is understandable to someone who isn't an IMA expert?

Ok, will be updated in the next version.

> 
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index ff3c906738cb..508053b8dd0a 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -47,7 +47,7 @@ Description:
> >  			fgroup:= decimal value
> >  		  lsm:  are LSM specific
> >  		  option:
> > -			appraise_type:= [imasig] [imasig|modsig]
> > +			appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
> >  			appraise_flag:= [check_blacklist]
> >  			Currently, blacklist check is only for files signed with appended
> >  			signature.
> > @@ -153,9 +153,27 @@ Description:
> >  
> >  			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
> >  
> > -		Example of 'measure' rule requiring fs-verity's digests on a
> > -		particular filesystem with indication of type of digest in
> > -		the measurement list.
> > +		Example of a 'measure' rule requiring fs-verity's digests
> > +		with indication of type of digest in the measurement list.
> >  
> >  			measure func=FILE_CHECK digest_type=verity \
> > -				fsuuid=... template=ima-ngv2
> > +				template=ima-ngv2
> > +
> > +		Example of 'measure' and 'appraise' rules requiring fs-verity
> > +		signatures (version 3) stored in security.ima xattr.
> > +
> > +		The 'measure' rule specifies the 'ima-sig' template option,
> > +		which includes the file signature in the measurement list.
> > +
> > +			measure func=BPRM_CHECK digest_type=verity \
> > +				template=ima-sig
> > +
> > +		The 'appraise' rule specifies the type and signature version
> > +		(sigv3) required.
> > +
> > +			appraise func=BPRM_CHECK digest_type=verity \
> > +				appraise_type=sigv3
> > +
> > +		All of these policy rules could, for example, be constrained
> > +		either based on a filesystem's UUID (fsuuid) or based on LSM
> > +		labels.
> 
> Is there documentation for what the appraise_type argument means, or does it
> just need to be reverse engineered from the above example?

The original definition was defined in Backus–Naur form.  Other
information was subsequently included which resulted in making it less
readable.  Perhaps for now, the explanation could be indented:

                        appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
                            where 'imasig' is the original or v2 signature,
                            where 'modsig' is an appended signature,
                            where 'sigv3' is the IMA v3 signature.

> 
> > + - 'sig': the file signature, based on either the file's/fsverity's digest[1],
> > +   or the EVM portable signature if the file signature is not found;
> 
> This sentence doesn't make sense.  How can it be the file signature if the
> "file signature is not found"?

EVM protects file metadata including security xattrs.  In order to make
EVM signatures portable, the i_ino and i_generation, which are
filesystem specific, couldn't be included.  Instead EVM portable
signature requires including 'security.ima', which may be a file hash
or signature.  Requiring both EVM and IMA signatures was considered
unnecessary, maybe even redundant.  When 'security.ima' is a file hash,
the 'sig' field may contain the EVM signature.

I've updated to be:
 - 'sig': the file signature, based on either the file's/fsverity's
digest[1],
   or the EVM portable signature, if 'security.ima' contains a file
hash.

> 
> > @@ -303,6 +321,12 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
> >  	case EVM_IMA_XATTR_DIGSIG:
> >  		set_bit(IMA_DIGSIG, &iint->atomic_flags);
> >  
> > +		if (iint->flags & (IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED)) {
> > +			*cause = "verity-signature-required";
> > +			*status = INTEGRITY_FAIL;
> > +			break;
> > +		}
> 
> Shouldn't this check whether *both* of these flags are set?

Yes, thank you.

> 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 28aca1f9633b..d3006cc22ab1 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1311,6 +1311,12 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  	    !(entry->flags & IMA_MODSIG_ALLOWED))
> >  		return false;
> >  
> > +	/* Ensure APPRAISE verity file implies a v3 signature */
> > +	if (entry->action == APPRAISE &&
> > +	    (entry->flags & IMA_VERITY_REQUIRED) &&
> > +	    !(entry->flags & IMA_DIGSIG_REQUIRED))
> > +		return false;
> 
> This comment doesn't seem to match the code.h

Agreed, the comment will be updated in the next version.

> > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> > index d370fca04de4..ecbe61c53d40 100644
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> > @@ -495,7 +495,8 @@ int ima_eventsig_init(struct ima_event_data *event_data,
> >  {
> >  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> >  
> > -	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> > +	if (!xattr_value ||
> > +	    !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG)))
> >  		return ima_eventevmsig_init(event_data, field_data);
> 
> This is OR-ing together values that aren't bit flags.

Updated.

thanks,

Mimi