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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240425041351.GD1401@sol.localdomain>
Date: Wed, 24 Apr 2024 21:13:51 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Fan Wu <wufan@...ux.microsoft.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org,
	serge@...lyn.com, tytso@....edu, axboe@...nel.dk, agk@...hat.com,
	snitzer@...nel.org, eparis@...hat.com, paul@...l-moore.com,
	linux-doc@...r.kernel.org, linux-integrity@...r.kernel.org,
	linux-security-module@...r.kernel.org, fsverity@...ts.linux.dev,
	linux-block@...r.kernel.org, dm-devel@...ts.linux.dev,
	audit@...r.kernel.org, linux-kernel@...r.kernel.org,
	Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v17 20/21] Documentation: add ipe documentation

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