Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 23 Feb 2022 17:24:12 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Mimi Zohar <zohar@linux.ibm.com>
Cc:     linux-integrity@vger.kernel.org,
        Stefan Berger <stefanb@linux.ibm.com>,
        linux-fscrypt@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 7/8] ima: support fs-verity file digest based version
 3 signatures
Message-ID: <YhbePM/BiRCzL3bn@sol.localdomain>
References: <20220211214310.119257-1-zohar@linux.ibm.com>
 <20220211214310.119257-8-zohar@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220211214310.119257-8-zohar@linux.ibm.com>
Precedence: bulk

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?

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

> + - '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"?

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

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

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

- Eric