On Fri, 16 Jun 2023 at 03:17, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > Hi Luca, > > On Fri, Jun 16, 2023 at 02:10:35AM +0100, Luca Boccassi wrote: > > On Fri, 16 Jun 2023 at 00:07, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > > > fsverity builtin signatures (CONFIG_FS_VERITY_BUILTIN_SIGNATURES) aren't > > > the only way to do signatures with fsverity, and they have some major > > > limitations. Yet, more users have tried to use them, e.g. recently by > > > https://github.com/ostreedev/ostree/pull/2640. In most cases this seems > > > to be because users aren't sufficiently familiar with the limitations of > > > this feature and what the alternatives are. > > > > > > Therefore, make some updates to the documentation to try to clarify the > > > properties of this feature and nudge users in the right direction. > > > > > > Note that the Integrity Policy Enforcement (IPE) LSM, which is not yet > > > upstream, is planned to use the builtin signatures. (This differs from > > > IMA, which uses its own signature mechanism.) For that reason, my > > > earlier patch "fsverity: mark builtin signatures as deprecated" > > > (https://lore.kernel.org/r/20221208033548.122704-1-ebiggers@xxxxxxxxxx), > > > which marked builtin signatures as "deprecated", was controversial. > > > > > > This patch therefore stops short of marking the feature as deprecated. > > > I've also revised the language to focus on better explaining the feature > > > and what its alternatives are. > > > > > > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> > > > --- > > > > > > This patch applies to https://git.kernel.org/pub/scm/fs/fsverity/linux.git/log/?h=for-next > > > > > > Documentation/filesystems/fsverity.rst | 176 ++++++++++++++++--------- > > > fs/verity/Kconfig | 16 +-- > > > fs/verity/enable.c | 2 +- > > > fs/verity/open.c | 8 +- > > > fs/verity/read_metadata.c | 4 +- > > > fs/verity/signature.c | 8 ++ > > > 6 files changed, 139 insertions(+), 75 deletions(-) > > > > > > diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst > > > index ede672dedf110..e990149cfdf5c 100644 > > > --- a/Documentation/filesystems/fsverity.rst > > > +++ b/Documentation/filesystems/fsverity.rst > > > > Most of the patch looks fine, two notes: > > > > > +- Trusted userspace code. When the accesses to a file happen in a > > > + well-defined way, userspace code can authenticate the file's > > > + fs-verity digest before accessing the file. This can be done by > > > + verifying a signature of the fs-verity file digest using any > > > + userspace cryptographic library that supports digital signatures. > > > + Consider using `libsodium > > > + <https://libsodium.gitbook.io/doc/public-key_cryptography/public-key_signatures>`_ > > > + or `Tink <https://developers.google.com/tink/digitally-sign-data>`_. > > > + Other options include OpenSSL, JCA, and libgcrypt. > > > > This should at least mention something like "depending on whether the > > threat model allows trusting userspace with such tasks", because it is > > by no means guaranteed that it is the case. > > Sure, that's why it says "Trusted userspace code", but I can make it clearer. Thanks, that would be great. > > > +- fs-verity builtin signatures are in PKCS#7 format, and the public > > > + keys are in X.509 format. These data formats are complex and prone > > > + to vulnerabilities, so parsing them is preferably done in userspace. > > > + (fs-verity builtin signatures were made to use these formats because > > > + other kernel subsystems, such as the module loader, unfortunately > > > + used these formats already.) Most cryptographic libraries also > > > + support working with raw keys and signatures, which are much > > > + simpler. For example, consider using `libsodium > > > + <https://libsodium.gitbook.io/doc/public-key_cryptography/public-key_signatures>`_. > > > + > > > + IMA appraisal, which supports fs-verity, does not use PKCS#7, so it > > > + partially avoids this issue as well (though it does use X.509). > > > > The kernel makes extensive use of PKCS7, it's the foundation of the > > trust chain with secure boot (and kernel modules as noted) after all, > > among other things, so this description looks very out of place as > > part of the same project. Readers might be led to believe that using > > secure boot or signed modules is useless, or worse, dangerous, and > > that it's better not to, and I'm quite sure that's not something we > > want. > > > > Unfortunately just because PKCS#7, X.509, and ASN.1 is being used does not mean > it is a good idea. Have you read the kernel code that implements these formats? > A few years ago I went through some of it. Here are some of the bugs I fixed: > > 2eb9eabf1e86 ("KEYS: fix out-of-bounds read during ASN.1 parsing") > 624f5ab8720b ("KEYS: fix NULL pointer dereference during ASN.1 parsing [ver #2]") > e0058f3a874e ("ASN.1: fix out-of-bounds read when parsing indefinite length item") > 81a7be2cd69b ("ASN.1: check for error from ASN1_OP_END__ACT actions") > 0f30cbea005b ("X.509: reject invalid BIT STRING for subjectPublicKey") > 54c1fb39fe04 ("X.509: fix comparisons of ->pkey_algo") > 971b42c038dc ("PKCS#7: fix certificate chain verification") > 29f4a67c17e1 ("PKCS#7: fix certificate blacklisting") > 437499eea429 ("X.509: fix BUG_ON() when hash algorithm is unsupported") > 4b34968e77ad ("X.509: fix NULL dereference when restricting key with unsupported_sig") I have no doubt that there are bugs, as I have no doubts that there are bugs in every other subsystem, including fsverity, once you start looking hard enough. That's not the point. The point is that having the documentation of one kernel subsystem disparaging the mechanisms that are central to other kernel subsystems' functionality is weird and out of place. Something like that is fine to post on social media or a blog post or so. A user jumping from one page of kernel doc saying, paraphrasing heavily for the sake of argument, "use pkcs7 to ensure the security of your system via secure boot, measured boot and signed kernel modules" and another saying "pkcs7 is bad and broken, stay away from it" is just strange, confusing and incoherent from the point of view of a reader. Kind regards, Luca Boccassi
