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: <18587675b3c6ebc74b81cabe53dc293099304861.camel@linux.ibm.com>
Subject: Re: [PATCH v8 6/7] ima: support fs-verity file digest based version
 3 signatures
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org
Cc:     Eric Biggers <ebiggers@kernel.org>, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Mon, 02 May 2022 08:16:04 -0400
In-Reply-To: <8a18eb04-4d07-7bad-e6f9-0015788e6a11@linux.ibm.com>
References: <20220429112601.1421947-1-zohar@linux.ibm.com>
         <20220429112601.1421947-7-zohar@linux.ibm.com>
         <8a18eb04-4d07-7bad-e6f9-0015788e6a11@linux.ibm.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

> > @@ -225,6 +232,40 @@ int ima_read_xattr(struct dentry *dentry,
> >   	return ret;
> >   }
> >   
> > +/*
> > + * calc_file_id_hash - calculate the hash of the ima_file_id struct data
> > + * @type: xattr type [enum evm_ima_xattr_type]
> > + * @algo: hash algorithm [enum hash_algo]
> > + * @digest: pointer to the digest to be hashed
> > + * @hash: (out) pointer to the hash
> > + *
> > + * IMA signature version 3 disambiguates the data that is signed by
> > + * indirectly signing the hash of the ima_file_id structure data.
> > + *
> > + * Signing the ima_file_id struct is currently only supported for
> > + * IMA_VERITY_DIGSIG type xattrs.
> > + *
> > + * Return 0 on success, error code otherwise.
> > + */
> > +static int calc_file_id_hash(enum evm_ima_xattr_type type,
> > +			     enum hash_algo algo, const u8 *digest,
> > +			     struct ima_digest_data *hash)
> > +{
> > +	struct ima_file_id file_id = {
> > +		.hash_type = IMA_VERITY_DIGSIG, .hash_algorithm = algo};
> > +	unsigned int unused = HASH_MAX_DIGESTSIZE - hash_digest_size[algo];
> > +
> > +	if (type != IMA_VERITY_DIGSIG)
> > +		return -EINVAL;
> > +
> > +	memcpy(file_id.hash, digest, hash_digest_size[algo]);
> > +
> > +	hash->algo = algo;
> > +	hash->length = hash_digest_size[algo];
> > +
> > +	return ima_calc_buffer_hash(&file_id, sizeof(file_id) - unused, hash);
> 
> +struct ima_file_id {
> +	__u8 hash_type;		/* xattr type [enum evm_ima_xattr_type] */
> +	__u8 hash_algorithm;	/* Digest algorithm [enum hash_algo] */
> +	__u8 hash[HASH_MAX_DIGESTSIZE];
> +} __packed;
> 
> did you maybe mean 'sizeof(file_id.hash) - unused' ?

No, the hash includes the other fields as well.  Instead of including a
flexible array in struct ima_file_id and dynamically allocating the
memory for the struct with the specific hash size, the maximum sized
hash is included in the struct.
> 
> 
> > +}
> > +


> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 390a8faa77f9..e24531db95cd 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1310,6 +1310,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   	    !(entry->flags & IMA_MODSIG_ALLOWED))
> >   		return false;
> >   
> > +	/*
> > +	 * Ensure verity appraise rules require signature format v3 signatures
> > +	 * ('appraise_type=sigv3').
> 
> This comment doesn't seem to reflect what is actually checked below ... 
> at least for me it's difficult to see that.
> 
> It's more like 'ensure that appraise rules for verity signature type 
> also have the IMA_DIGSIG_REQUIRED flag set.'

Generally code should be understandable without requiring a comment.  
In this case, the purpose of the comment is to require a file
signature.  Here's the updated comment:

+        * Unlike for regular IMA 'appraise' policy rules where
security.ima
+        * xattr may contain either a file hash or signature, the
security.ima
+        * xattr for fsverity must contain a file signature
(sigv3).  Ensure
+        * that 'appraise' rules for fsverity require file signatures
by
+        * checking the IMA_DIGSIG_REQUIRED flag is set.


> > +	 */
> > +	if (entry->action == APPRAISE &&
> > +	    (entry->flags & IMA_VERITY_REQUIRED) &&
> > +	    !(entry->flags & IMA_DIGSIG_REQUIRED))
> > +		return false;
> > +
> >   	return true;
> >   }
> >   

thanks,

Mimi