Received-SPF: pass (google.com: domain of linux-kernel+bounces-157987-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 24 Apr 2024 21:13:51 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Fan Wu <wufan@linux.microsoft.com>
Cc: corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
	serge@hallyn.com, tytso@mit.edu, axboe@kernel.dk, agk@redhat.com,
	snitzer@kernel.org, eparis@redhat.com, paul@paul-moore.com,
	linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, fsverity@lists.linux.dev,
	linux-block@vger.kernel.org, dm-devel@lists.linux.dev,
	audit@vger.kernel.org, linux-kernel@vger.kernel.org,
	Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [PATCH v17 20/21] Documentation: add ipe documentation
Message-ID: <20240425041351.GD1401@sol.localdomain>
References: <1712969764-31039-1-git-send-email-wufan@linux.microsoft.com>
 <1712969764-31039-21-git-send-email-wufan@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1712969764-31039-21-git-send-email-wufan@linux.microsoft.com>

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