Re: [PATCH v17 20/21] Documentation: add ipe documentation

Eric Biggers
2024-04-25T04:13:53Z

On Fri, Apr 12, 2024 at 05:56:03PM -0700, Fan Wu wrote:
> +dmverity_roothash
> +~~~~~~~~~~~~~~~~~
> +
> + This property can be utilized for authorization or revocation of
> + specific dm-verity volumes, identified via its root hash. It has a
> + dependency on the DM_VERITY module. This property is controlled by
> + the ``IPE_PROP_DM_VERITY`` config option, it will be automatically
> + selected when ``IPE_SECURITY`` , ``DM_VERITY`` and
> + ``DM_VERITY_VERIFY_ROOTHASH_SIG`` are all enabled.
> + The format of this property is::
> +
> + dmverity_roothash=DigestName:HexadecimalString
> +
> + The supported DigestNames for dmverity_roothash are [#dmveritydigests]_ [#securedigest]_ :
> +
> + + blake2b-512
> + + blake2s-256
> + + sha1
> + + sha256
> + + sha384
> + + sha512
> + + sha3-224
> + + sha3-256
> + + sha3-384
> + + sha3-512
> + + md4
> + + md5
> + + sm3
> + + rmd160

It's not the 90s anymore. Insecure algorithms like md4, md5, and sha1 should
not be here.

> +dmverity_signature
> +~~~~~~~~~~~~~~~~~~
> +
> + This property can be utilized for authorization of all dm-verity
> + volumes that have a signed roothash that validated by a keyring
> + specified by dm-verity's configuration, either the system trusted
> + keyring, or the secondary keyring. It depends on
> + ``DM_VERITY_VERIFY_ROOTHASH_SIG`` config option and is controlled by
> + the ``IPE_PROP_DM_VERITY`` config option, it will be automatically
> + selected when ``IPE_SECURITY``, ``DM_VERITY`` and
> + ``DM_VERITY_VERIFY_ROOTHASH_SIG`` are all enabled.
> + The format of this property is::
> +
> + dmverity_signature=(TRUE|FALSE)
> +
> +fsverity_digest
> +~~~~~~~~~~~~~~~
> +
> + This property can be utilized for authorization or revocation of
> + specific fsverity enabled file, identified via its fsverity digest.
> + It depends on ``FS_VERITY`` config option and is controlled by
> + ``CONFIG_IPE_PROP_FS_VERITY``. The format of this property is::
> +
> + fsverity_digest=DigestName:HexadecimalString
> +
> + The supported DigestNames for fsverity_roothash are [#fsveritydigest]_ [#securedigest]_ :

fsverity_digest, not fsverity_roothash.

> +Allow any signed fs-verity file
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +::
> +
> + policy_name=AllowSignedFSVerity policy_version=0.0.0
> + DEFAULT action=DENY
> +
> + op=EXECUTE fsverity_signature=TRUE action=ALLOW

As elsewhere, ideally this would be more specific about what is meant by a
signed file. The goal is not to allow *any* signed file, but rather only allow
files that are signed by a particular someone/something.

> +Prohibit execution of a specific fs-verity file
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +::
> +
> + policy_name=ProhibitSpecificFSVF policy_version=0.0.0
> + DEFAULT action=DENY
> +
> + op=EXECUTE fsverity_digest=sha256:fd88f2b8824e197f850bf4c5109bea5cf0ee38104f710843bb72da796ba5af9e action=DENY
> + op=EXECUTE boot_verified=TRUE action=ALLOW
> + op=EXECUTE dmverity_signature=TRUE action=ALLOW

This example is a bit weird because it's a denylist, not an allowlist. In
general this could be trivially circumvented by creating a new binary that has
fsverity disabled or that doesn't meaningfully differ from the original.

> +.. [#fsveritydigest] These hash algorithms are based on values accepted by fsverity-utils;
> + IPE does not impose any restrictions on the digest algorithm itself;
> + thus, this list may be out of date.

It's the kernel's fsverity support, not fsverity-utils, that matters here.
fsverity-utils is kept up to date with the kernel, so in practice the list of
algorithms is the same on both sides, but it's the kernel that matters here.

> +.. [#dmveritydigests] These hash algorithms are based on values accepted by dm-verity,
> + specifically ``crypto_alloc_ahash`` in ``verity_ctr``; ``veritysetup``
> + does support more algorithms than the list above. IPE does not impose
> + any restrictions on the digest algorithm itself; thus, this list
> + may be out of date.

References to specific functions and locations in the code tend to get out of
date. I think you mean something like: any hash algorithm that's supported by
the Linux crypto API is supported.

> +
> +.. [#securedigest] Please ensure you are using cryptographically secure hash functions;
> + just because something is *supported* does not mean it is *secure*.

Instead of giving insecure algorithms like md4 as examples and then giving this
disclaimer, how about only giving secure algorithms as examples?

- Eric