Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4e9eecd01d1cb51dd3e8e92783742df0b66921ab.camel@linux.ibm.com>
Subject: Re: [PATCH v7 3/5] ima: permit fsverity's file digests in the IMA
 measurement list
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:   Wed, 27 Apr 2022 22:03:58 -0400
In-Reply-To: <YkyYUzEK9Tw4TgL2@gmail.com>
References: <20220325223824.310119-1-zohar@linux.ibm.com>
         <20220325223824.310119-4-zohar@linux.ibm.com> <YkyYUzEK9Tw4TgL2@gmail.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-04-05 at 19:28 +0000, Eric Biggers wrote:
> On Fri, Mar 25, 2022 at 06:38:22PM -0400, Mimi Zohar wrote:
> > Permit fsverity's file digest (a hash of struct fsverity_digest) to be
> > included in the IMA measurement list, based on the new measurement
> > policy rule 'digest_type=verity' option.
> 
> "fsverity's file digest" *is* 'struct fsverity_digest', not a hash of it.
> Did you mean to write 'struct fsverity_descriptor'?

Fixed.

> 
> > diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
> > index 1a91d92950a7..2d4789dc7750 100644
> > --- a/Documentation/security/IMA-templates.rst
> > +++ b/Documentation/security/IMA-templates.rst
> > @@ -68,6 +68,9 @@ descriptors by adding their identifier to the format string
> >   - 'd-ng': the digest of the event, calculated with an arbitrary hash
> >     algorithm (field format: [<hash algo>:]digest, where the digest
> >     prefix is shown only if the hash algorithm is not SHA1 or MD5);
> > + - 'd-ngv2': same as d-ng, but prefixed with the digest type.
> > +    field format: [<digest type>:<hash algo>:]digest,
> > +        where the digest type is either "ima" or "verity".
> 
> As in patch 2, it is not clear what the square brackets mean here.  Maybe they
> mean that "<digest type>:<hash algo>:" is optional, but it is not explained when
> they will be present and when they will not be present.

Agreed, removed.

> 
> >   - 'd-modsig': the digest of the event without the appended modsig;
> >   - 'n-ng': the name of the event, without size limitations;
> >   - 'sig': the file signature, or the EVM portable signature if the file
> > @@ -106,3 +109,8 @@ currently the following methods are supported:
> >     the ``ima_template=`` parameter;
> >   - register a new template descriptor with custom format through the kernel
> >     command line parameter ``ima_template_fmt=``.
> > +
> > +
> > +References
> > +==========
> > +[1] Documentation/filesystems/fsverity.rst
> 
> Is this meant to be a footnote?  There are no references to it above.
> 
> > @@ -242,14 +267,29 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  	 */
> >  	i_version = inode_query_iversion(inode);
> >  	hash.hdr.algo = algo;
> > +	hash.hdr.length = hash_digest_size[algo];
> >  
> >  	/* Initialize hash digest to 0's in case of failure */
> >  	memset(&hash.digest, 0, sizeof(hash.digest));
> >  
> > -	if (buf)
> > +	if (buf) {
> >  		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> > -	else
> > +	} else if (iint->flags & IMA_VERITY_REQUIRED) {
> > +		result = ima_get_verity_digest(iint, &hash);
> > +		switch (result) {
> > +		case 0:
> > +			break;
> > +		case -ENODATA:
> > +			audit_cause = "no-verity-digest";
> > +			result = -EINVAL;
> > +			break;
> > +		default:
> > +			audit_cause = "invalid-verity-digest";
> > +			break;
> > +		}
> > +	} else {
> >  		result = ima_calc_file_hash(file, &hash.hdr);
> > +	}
> >  
> >  	if (result && result != -EBADF && result != -EINVAL)
> >  		goto out;
> 
> The above code only calls ima_get_verity_digest() if 'buf' is non-NULL,
> otherwise it calls ima_calc_buffer_hash().  Under what circumstances is 'buf'
> non-NULL?  Does this imply that 'digest_type=verity' does not always use verity
> digests, and if not, when are they used and when are they not used?

Agreed, it should always be based on policy.

FYI, instead of IMA pre-reading and calculating the file hash, there
are instances where the kernel reads the entire file into memory.   For
example, kernel_read_file() calls security_kernel_post_read_file(),
which calls ima_post_read_file().

> 
> > +/*
> > + * Make sure the policy rule and template format are in sync.
> > + */
> > +static void check_template_field(const struct ima_template_desc *template,
> > +				 const char *field, const char *msg)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < template->num_fields; i++)
> > +		if (!strcmp(template->fields[i]->field_id, field))
> > +			return;
> > +
> > +	pr_notice_once("%s", msg);
> > +}
> 
> A better description for this function would be something like "Warn if the
> template does not contain the given field."

Ok
> 
> > index daf49894fd7d..d42a01903f08 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -32,7 +32,7 @@
> >  #define IMA_HASHED		0x00000200
> >  
> >  /* iint policy rule cache flags */
> > -#define IMA_NONACTION_FLAGS	0xff000000
> > +#define IMA_NONACTION_FLAGS	0xff800000
> >  #define IMA_DIGSIG_REQUIRED	0x01000000
> >  #define IMA_PERMIT_DIRECTIO	0x02000000
> >  #define IMA_NEW_FILE		0x04000000
> > @@ -40,6 +40,7 @@
> >  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
> >  #define IMA_MODSIG_ALLOWED	0x20000000
> >  #define IMA_CHECK_BLACKLIST	0x40000000
> > +#define IMA_VERITY_REQUIRED	0x80000000
> 
> It is intentional that the new bit added to IMA_NONACTION_FLAGS is not the same
> as IMA_VERITY_REQUIRED?

Thanks for catching this.  Previous versions required an additional
bit, but that isn't the case now.

thanks,

Mimi