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
