On Fri, 2023-06-16 at 13:57 +0100, Luca Boccassi wrote: > On Fri, 16 Jun 2023 at 10:51, Roberto Sassu > <roberto.sassu@xxxxxxxxxxxxxxx> wrote: > > On Thu, 2023-06-15 at 19:17 -0700, Eric Biggers 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. > > > > > > > > +- 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") > > > > > > 971b42c038dc is noteworthy; it turned out the kernel did not properly verify > > > certificate chains in PKCS#7 messages. That was fundamentally a PKCS#7-specific > > > security bug that was directly caused by the complexity that is specific to > > > PKCS#7. Simple signatures do not have certificate chains. > > > > > > I hope the code is in slightly better shape now. But I really haven't looked at > > > it in several years. In any case, the fact is that these formats are complex, > > > which causes bugs. I don't think we should be trying to pretend otherwise. > > > > That is a quite extensive explanation why is not a good idea to parse > > key/certificates in the kernel. > > > > Actually, I tried to address that with this patch set: > > > > https://lore.kernel.org/linux-kernel//20230425173557.724688-1-roberto.sassu@xxxxxxxxxxxxxxx/ > > > > The idea was to develop an asymmetric key parser to forward the key > > material from the kernel to a user space process for parsing, and get > > back a well formatted key (basically the same fields of struct > > public_key). > > > > Maybe that would not work for X.509 certificates, as they are > > extensively used in kernel code, but for simpler formats like PGP, > > maybe. And the mechanism is interchangeable. If you want to support > > another key format, you need to change only user space. > > > > The challenge is if the user space process makes some security > > decisions, like for key expiration, etc. I thought that we could > > enforce strong isolation of that process by denying ptrace on it, but > > it is still work in progress... > > Something like that makes sense on systems where userspace sits at a > higher privilege level than the kernel, so to speak, in the threat > model. While I'm sure there are such systems, it doesn't mean it's the > best solution everywhere. > So, having that as an option on equal footing: fine, no problem. > Having that as the only option, or implying it's the only > possible/sensible option everywhere: sorry, not fine. (don't want to deviate from the main topic of the discussion, maybe it is better to start another thread, just last short answer) I agree with that, and actually the feedback that I got is that is rather better to still develop kernel code. If a user space process is going to do some processing on behalf of the kernel, there must be strong isolation guarantees. If I can imagine something closer, that would be a kernel thread. Denying ptrace is one part of the solution. Strong isolation would mean that the process must exchange data exclusively with the kernel, or might be corrupted by other user space processes through the other communication channels. The kernel should have the ability to make that process completely isolated, it is just to ensure that all communication channels are considered. The advantage? In case of corruption, the process cannot tamper with kernel memory. If you are worried about running too much code (such as glibc), the process could use klibc (the binary with a PGP implementation ported from kernel space has a size of 73K; no external depedencies, for crypto it uses the kernel Crypto API). Thanks Roberto
