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'?
> 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.
> - '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?
> +/*
> + * 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."
> 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?
- Eric
