Re: [PATCH v3] fsverity: improve documentation for builtin signature support

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Jun 20, 2023 at 08:15:42PM +0100, Luca Boccassi wrote:
> On Tue, 20 Jun 2023 at 05:23, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > 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@kernel.org),
> > 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@google.com>
> > ---
> >
> > v3: fixed ioctl name, and more updates to address Luca's comments
> > v2: updated two paragraphs of fsverity.rst to address Luca's comments
> >
> >  Documentation/filesystems/fsverity.rst | 192 ++++++++++++++++---------
> >  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, 149 insertions(+), 81 deletions(-)
> <...>
> > +- fs-verity builtin signatures are in PKCS#7 format, and the public
> > +  keys are in X.509 format.  These formats are commonly used,
> > +  including by some other kernel features (which is why the fs-verity
> > +  builtin signatures use them), and are very feature rich.
> > +  Unfortunately, history has shown that code that parses and handles
> > +  these formats (which are from the 1990s and are based on ASN.1)
> > +  often has vulnerabilities as a result of their complexity.  This
> > +  complexity is not inherent to the cryptography itself.
> > +
> > +  fs-verity users who do not need advanced features of X.509 and
> > +  PKCS#7 should strongly consider using simpler formats, such as plain
> > +  Ed25519 keys and signatures, and verifying signatures in userspace.
> > +
> > +  fs-verity users who choose to use X.509 and PKCS#7 anyway should
> > +  still consider that verifying those signatures in userspace is more
> > +  flexible (for other reasons mentioned earlier in this document) and
> > +  eliminates the need to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > +  and its associated increase in kernel attack surface.  In some cases
> > +  it can even be necessary, since advanced X.509 and PKCS#7 features
> > +  do not always work as intended with the kernel.  For example, the
> > +  kernel does not check X.509 certificate validity times.
> > +
> > +  Note: IMA appraisal, which supports fs-verity, does not use PKCS#7
> > +  for its signatures, so it partially avoids the issues discussed
> > +  here.  IMA appraisal does use X.509.
> 
> Thank you for the update, I find this new version much better as it is
> clearly scoped for the specific case of fs-verity, hence:
> 
> Reviewed-by: Luca Boccassi <bluca@debian.org>

Great, I'm glad we were finally able to arrive at a version that you're okay
with.

- Eric