diff mbox series

[v2] fsverity: improve documentation for builtin signature support

Message ID 20230619221048.10335-1-ebiggers@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v2] fsverity: improve documentation for builtin signature support | expand

Commit Message

Eric Biggers June 19, 2023, 10:10 p.m. UTC
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>
---

v2: updated two paragraphs of fsverity.rst to address Luca's comments

 Documentation/filesystems/fsverity.rst | 190 ++++++++++++++++---------
 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, 147 insertions(+), 81 deletions(-)


base-commit: 74836ecbc5c7565d24a770917644e96af3e98d25

Comments

Eric Biggers June 19, 2023, 10:17 p.m. UTC | #1
On Mon, Jun 19, 2023 at 03:10:48PM -0700, Eric Biggers wrote:
> +  authenticate applications before loading them.  In these cases, this
> +  trusted userspace code can authenticate a file's contents by
> +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then

Sorry, the above is supposed to say FS_IOC_MEASURE_VERITY.

- Eric
Luca Boccassi June 19, 2023, 11:04 p.m. UTC | #2
On Mon, 19 Jun 2023 at 23:11, 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>
> ---
>
> v2: updated two paragraphs of fsverity.rst to address Luca's comments
>
>  Documentation/filesystems/fsverity.rst | 190 ++++++++++++++++---------
>  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, 147 insertions(+), 81 deletions(-)
>
> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index ede672dedf110..c33f783e74953 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
<...>
> +- Trusted userspace code.  Often, the userspace code that accesses
> +  files can be trusted to authenticate them.  Consider e.g. an
> +  application that wants to authenticate data files before using them,
> +  or an application loader that is part of the operating system (which
> +  is already authenticated in a different way, such as by being loaded
> +  from a read-only partition that uses dm-verity) and that wants to
> +  authenticate applications before loading them.  In these cases, this
> +  trusted userspace code can authenticate a file's contents by
> +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
> +  verifying a signature of it 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.

Text looks good to me now, but please just drop the last sentence with
the external projects links, as it seems best left as an exercise for
the reader to find their preferred tooling that is appropriate to be
used at the time of reading, as this will get out of date fast.

<...>

> +- fs-verity builtin signatures are in PKCS#7 format, and the public
> +  keys are in X.509 format.  These data formats are unnecessarily
> +  complex and prone to vulnerabilities.  (fs-verity builtin signatures
> +  were made to use these formats because other kernel subsystems, such
> +  as the module loader, unfortunately used these formats already.
> +  Note, these formats should still be used when they are the only
> +  option to have signatures at all.)  Userspace signature verification
> +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES and the
> +  associated kernel attack surface.  Userspace also has the
> +  flexibility to choose simpler formats.  For example, consider using
> +  straightforward Ed25519 keys and signatures with `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).
> +
> +  If you are considering making use of "advanced" features of X.509
> +  and/or PKCS#7, please also keep in mind that these "advanced"
> +  features do not always work as intended with the kernel.  For
> +  example, the kernel does not check X.509 certificate validity times.

Sorry but this still reads as way too opinionated and generic, rather
than being fsverity-specific. Please simplify to convey the same
message in more concise way, perhaps something along these lines:

- fs-verity builtin signatures are in PKCS#7 format, and the public
keys are in X.509 format. IMA appraisal, which supports fs-verity,
uses a custom signature format rather than PKCS#7 and X.509 for public
keys. Alternative formats for signatures and public keys are not
supported for builtin signatures or IMA appraisal. For fully flexible
and customized signature and public keys formats, while also avoiding
to expose the kernel to untrusted input, signature verification can be
implemented by a trusted userspace component as described at <pointer
to appropriate section>

Kind regards,
Luca Boccassi
Eric Biggers June 19, 2023, 11:49 p.m. UTC | #3
On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> > diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> > index ede672dedf110..c33f783e74953 100644
> > --- a/Documentation/filesystems/fsverity.rst
> > +++ b/Documentation/filesystems/fsverity.rst
> <...>
> > +- Trusted userspace code.  Often, the userspace code that accesses
> > +  files can be trusted to authenticate them.  Consider e.g. an
> > +  application that wants to authenticate data files before using them,
> > +  or an application loader that is part of the operating system (which
> > +  is already authenticated in a different way, such as by being loaded
> > +  from a read-only partition that uses dm-verity) and that wants to
> > +  authenticate applications before loading them.  In these cases, this
> > +  trusted userspace code can authenticate a file's contents by
> > +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
> > +  verifying a signature of it 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.
> 
> Text looks good to me now, but please just drop the last sentence with
> the external projects links, as it seems best left as an exercise for
> the reader to find their preferred tooling that is appropriate to be
> used at the time of reading, as this will get out of date fast.
> 
> <...>

Well, a significant part of the motivation for this patch is that the "exercise
for the reader" approach has already been tried, for several years, but it is
not working well.  People don't know what to use and need a little more help.

I'm planning to add some example code to fsverity-utils, probably using
libsodium.  After that, I'll make this documentation link to there.  But for
now, I think the last two sentences of the above paragraph are helpful.

> 
> > +- fs-verity builtin signatures are in PKCS#7 format, and the public
> > +  keys are in X.509 format.  These data formats are unnecessarily
> > +  complex and prone to vulnerabilities.  (fs-verity builtin signatures
> > +  were made to use these formats because other kernel subsystems, such
> > +  as the module loader, unfortunately used these formats already.
> > +  Note, these formats should still be used when they are the only
> > +  option to have signatures at all.)  Userspace signature verification
> > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES and the
> > +  associated kernel attack surface.  Userspace also has the
> > +  flexibility to choose simpler formats.  For example, consider using
> > +  straightforward Ed25519 keys and signatures with `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).
> > +
> > +  If you are considering making use of "advanced" features of X.509
> > +  and/or PKCS#7, please also keep in mind that these "advanced"
> > +  features do not always work as intended with the kernel.  For
> > +  example, the kernel does not check X.509 certificate validity times.
> 
> Sorry but this still reads as way too opinionated and generic, rather
> than being fsverity-specific.
>
> Please simplify to convey the same
> message in more concise way, perhaps something along these lines:
> 
> - fs-verity builtin signatures are in PKCS#7 format, and the public
> keys are in X.509 format. IMA appraisal, which supports fs-verity,
> uses a custom signature format rather than PKCS#7 and X.509 for public
> keys. Alternative formats for signatures and public keys are not
> supported for builtin signatures or IMA appraisal. For fully flexible
> and customized signature and public keys formats, while also avoiding
> to expose the kernel to untrusted input, signature verification can be
> implemented by a trusted userspace component as described at <pointer
> to appropriate section>

That is not the same message at all, as it omits the mention of the
disadvantages of PKCS#7 and X.509 compared to raw signatures, which was the main
point.  So no, I don't think your version would be better.

It seems that what is going on here is that you are invested heavily into
existing X.509 and PKCS#7 based systems, and as a result you do not want the
problems with these formats to be described anywhere in the kernel
documentation.  That is understandable, but that is a special interest that
should not be catered to here.  This documentation is trying to help users make
a decision of what type of signature to use in new systems.  And yes, it is
fsverity specific documentation, but there is no way for it to make the needed
point without dicussing the underlying data formats.

- Eric
Luca Boccassi June 20, 2023, 12:42 a.m. UTC | #4
On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> > > diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> > > index ede672dedf110..c33f783e74953 100644
> > > --- a/Documentation/filesystems/fsverity.rst
> > > +++ b/Documentation/filesystems/fsverity.rst
> > <...>
> > > +- Trusted userspace code.  Often, the userspace code that accesses
> > > +  files can be trusted to authenticate them.  Consider e.g. an
> > > +  application that wants to authenticate data files before using them,
> > > +  or an application loader that is part of the operating system (which
> > > +  is already authenticated in a different way, such as by being loaded
> > > +  from a read-only partition that uses dm-verity) and that wants to
> > > +  authenticate applications before loading them.  In these cases, this
> > > +  trusted userspace code can authenticate a file's contents by
> > > +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
> > > +  verifying a signature of it 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.
> >
> > Text looks good to me now, but please just drop the last sentence with
> > the external projects links, as it seems best left as an exercise for
> > the reader to find their preferred tooling that is appropriate to be
> > used at the time of reading, as this will get out of date fast.
> >
> > <...>
>
> Well, a significant part of the motivation for this patch is that the "exercise
> for the reader" approach has already been tried, for several years, but it is
> not working well.  People don't know what to use and need a little more help.
>
> I'm planning to add some example code to fsverity-utils, probably using
> libsodium.  After that, I'll make this documentation link to there.  But for
> now, I think the last two sentences of the above paragraph are helpful.

That list does not help, quite the opposite - libsodium seems all but
abandoned (last release 5 years ago, an unmaintained project is not
exactly what you want for your crypto primitives) and tink appears to
be one of those google's sources-available-proprietary projects, which
means that, as with everything else that google throws over the wall,
it's at permanent risk of getting killed with little notice, among
many other problems. If you really want to suggest something, OpenSSL
seems like an appropriate choice of a widely available, supported and
well-known solution that is the most likely to still be around and
maintained in 5/10 years.

> > > +- fs-verity builtin signatures are in PKCS#7 format, and the public
> > > +  keys are in X.509 format.  These data formats are unnecessarily
> > > +  complex and prone to vulnerabilities.  (fs-verity builtin signatures
> > > +  were made to use these formats because other kernel subsystems, such
> > > +  as the module loader, unfortunately used these formats already.
> > > +  Note, these formats should still be used when they are the only
> > > +  option to have signatures at all.)  Userspace signature verification
> > > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES and the
> > > +  associated kernel attack surface.  Userspace also has the
> > > +  flexibility to choose simpler formats.  For example, consider using
> > > +  straightforward Ed25519 keys and signatures with `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).
> > > +
> > > +  If you are considering making use of "advanced" features of X.509
> > > +  and/or PKCS#7, please also keep in mind that these "advanced"
> > > +  features do not always work as intended with the kernel.  For
> > > +  example, the kernel does not check X.509 certificate validity times.
> >
> > Sorry but this still reads as way too opinionated and generic, rather
> > than being fsverity-specific.
> >
> > Please simplify to convey the same
> > message in more concise way, perhaps something along these lines:
> >
> > - fs-verity builtin signatures are in PKCS#7 format, and the public
> > keys are in X.509 format. IMA appraisal, which supports fs-verity,
> > uses a custom signature format rather than PKCS#7 and X.509 for public
> > keys. Alternative formats for signatures and public keys are not
> > supported for builtin signatures or IMA appraisal. For fully flexible
> > and customized signature and public keys formats, while also avoiding
> > to expose the kernel to untrusted input, signature verification can be
> > implemented by a trusted userspace component as described at <pointer
> > to appropriate section>
>
> That is not the same message at all, as it omits the mention of the
> disadvantages of PKCS#7 and X.509 compared to raw signatures, which was the main
> point.  So no, I don't think your version would be better.

The 'disadvantages' are your personal opinions. It's fine to have
opinions, it's less fine to present them as if they were industry
consensus in public project-wide documentation.

> It seems that what is going on here is that you are invested heavily into
> existing X.509 and PKCS#7 based systems, and as a result you do not want the
> problems with these formats to be described anywhere in the kernel
> documentation.  That is understandable, but that is a special interest that
> should not be catered to here.  This documentation is trying to help users make
> a decision of what type of signature to use in new systems.  And yes, it is
> fsverity specific documentation, but there is no way for it to make the needed
> point without dicussing the underlying data formats.

Industry standards are by very definition the opposite of 'special
interests'. Look, I tried my best to provide constructive and
actionable feedback in previous replies, but given you seem only
interested in casting aspersions and hijacking kernel documentation to
promote the latest proprietary google-toy-of-the-month:

Nacked-by: Luca Boccassi <bluca@debian.org>
Eric Biggers June 20, 2023, 3:18 a.m. UTC | #5
On Tue, Jun 20, 2023 at 01:42:20AM +0100, Luca Boccassi wrote:
> On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> > > > diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> > > > index ede672dedf110..c33f783e74953 100644
> > > > --- a/Documentation/filesystems/fsverity.rst
> > > > +++ b/Documentation/filesystems/fsverity.rst
> > > <...>
> > > > +- Trusted userspace code.  Often, the userspace code that accesses
> > > > +  files can be trusted to authenticate them.  Consider e.g. an
> > > > +  application that wants to authenticate data files before using them,
> > > > +  or an application loader that is part of the operating system (which
> > > > +  is already authenticated in a different way, such as by being loaded
> > > > +  from a read-only partition that uses dm-verity) and that wants to
> > > > +  authenticate applications before loading them.  In these cases, this
> > > > +  trusted userspace code can authenticate a file's contents by
> > > > +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
> > > > +  verifying a signature of it 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.
> > >
> > > Text looks good to me now, but please just drop the last sentence with
> > > the external projects links, as it seems best left as an exercise for
> > > the reader to find their preferred tooling that is appropriate to be
> > > used at the time of reading, as this will get out of date fast.
> > >
> > > <...>
> >
> > Well, a significant part of the motivation for this patch is that the "exercise
> > for the reader" approach has already been tried, for several years, but it is
> > not working well.  People don't know what to use and need a little more help.
> >
> > I'm planning to add some example code to fsverity-utils, probably using
> > libsodium.  After that, I'll make this documentation link to there.  But for
> > now, I think the last two sentences of the above paragraph are helpful.
> 
> That list does not help, quite the opposite - libsodium seems all but
> abandoned (last release 5 years ago, an unmaintained project is not
> exactly what you want for your crypto primitives)

libsodium has an active maintainer.  Last commit was 3 months ago.

Also note that simple crypto libraries that focus on offering modern
cryptographic algorithms don't need to be updated often.  You actually *don't*
really want to have to constantly update your crypto library because they e.g.
messed up their ASN.1 parser.  If there is no ASN.1, that cannot happen.

> tink appears to
> be one of those google's sources-available-proprietary projects, which
> means that, as with everything else that google throws over the wall,
> it's at permanent risk of getting killed with little notice, among
> many other problems.

Tink is an open source project licensed under the Apache license.  Non-Google
contributions to Tink are being accepted on GitHub.  If Google stops maintaining
Tink, it can be forked, like any other open source project.

Anyway, my intent was simply to list some modern crypto libraries.  One of them
just happens to be from Google.  I'm sorry if it may have appeared like I listed
a Google project because of it being from Google.  That was not my intent.

> If you really want to suggest something, OpenSSL
> seems like an appropriate choice of a widely available, supported and
> well-known solution that is the most likely to still be around and
> maintained in 5/10 years.

Sure.  Which is why I included OpenSSL in the list.

Anyway, I'll go ahead and take your suggestion to omit the mentions of any
specific crypto libraries.  I do agree that it could use some more thought, and
the main place for this information will be in fsverity-utils which will have
the example code.

> > > > +- fs-verity builtin signatures are in PKCS#7 format, and the public
> > > > +  keys are in X.509 format.  These data formats are unnecessarily
> > > > +  complex and prone to vulnerabilities.  (fs-verity builtin signatures
> > > > +  were made to use these formats because other kernel subsystems, such
> > > > +  as the module loader, unfortunately used these formats already.
> > > > +  Note, these formats should still be used when they are the only
> > > > +  option to have signatures at all.)  Userspace signature verification
> > > > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES and the
> > > > +  associated kernel attack surface.  Userspace also has the
> > > > +  flexibility to choose simpler formats.  For example, consider using
> > > > +  straightforward Ed25519 keys and signatures with `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).
> > > > +
> > > > +  If you are considering making use of "advanced" features of X.509
> > > > +  and/or PKCS#7, please also keep in mind that these "advanced"
> > > > +  features do not always work as intended with the kernel.  For
> > > > +  example, the kernel does not check X.509 certificate validity times.
> > >
> > > Sorry but this still reads as way too opinionated and generic, rather
> > > than being fsverity-specific.
> > >
> > > Please simplify to convey the same
> > > message in more concise way, perhaps something along these lines:
> > >
> > > - fs-verity builtin signatures are in PKCS#7 format, and the public
> > > keys are in X.509 format. IMA appraisal, which supports fs-verity,
> > > uses a custom signature format rather than PKCS#7 and X.509 for public
> > > keys. Alternative formats for signatures and public keys are not
> > > supported for builtin signatures or IMA appraisal. For fully flexible
> > > and customized signature and public keys formats, while also avoiding
> > > to expose the kernel to untrusted input, signature verification can be
> > > implemented by a trusted userspace component as described at <pointer
> > > to appropriate section>
> >
> > That is not the same message at all, as it omits the mention of the
> > disadvantages of PKCS#7 and X.509 compared to raw signatures, which was the main
> > point.  So no, I don't think your version would be better.
> 
> The 'disadvantages' are your personal opinions. It's fine to have
> opinions, it's less fine to present them as if they were industry
> consensus in public project-wide documentation.

I think it is a bit more objective than a "personal opinion".  I've given many
examples of vulnerability fixes in the kernel's ASN.1, X.509, and PKCS#7 code,
which were not inherent to the actual crypto that users want but rather just
caused by the complexities of these formats.  I've also mentioned how signatures
can be created and verified using an industry-standard signature algorithm
without using any of these formats.  I think you're also being a bit
disrespectful to people like me who have actually taken time to find and fix
vulnerabilities in the kernel's implementation of these formats.  As well as the
developers of major crypto libraries who will tell you the exact same thing.

> > It seems that what is going on here is that you are invested heavily into
> > existing X.509 and PKCS#7 based systems, and as a result you do not want the
> > problems with these formats to be described anywhere in the kernel
> > documentation.  That is understandable, but that is a special interest that
> > should not be catered to here.  This documentation is trying to help users make
> > a decision of what type of signature to use in new systems.  And yes, it is
> > fsverity specific documentation, but there is no way for it to make the needed
> > point without dicussing the underlying data formats.
> 
> Industry standards are by very definition the opposite of 'special
> interests'. Look, I tried my best to provide constructive and
> actionable feedback in previous replies, but given you seem only
> interested in casting aspersions and hijacking kernel documentation to
> promote the latest proprietary google-toy-of-the-month:
> 
> Nacked-by: Luca Boccassi <bluca@debian.org>

It's quite strange that you consider Ed25519 to be a "Google proprietary-toy-of-
the-month".  Are you sure you really know what you're talking about?

You've also claimed that my proposed documentation contradicts other kernel
documentation.  I don't see where that's the case.  There are other features
that use PKCS#7, but none of them seems to be extolling the virtues of PKCS#7.
It's just the signature format they happened to choose, probably because it was
the first one that came to mind (which is what happened to fs-verity too).  I
just would like to help fs-verity users avoid that mistake, when possible.

Anyway, I'll send out v3 of this patch with the mentions of the specific crypto
libraries removed.  I'll also update the wording of the PKCS#7 paragraph a bit.
I expect you still won't be happy with it.  But I do believe it is important to
give good security advice, which includes avoiding unnecessary complexity.

- Eric
Eric Biggers June 20, 2023, 5:45 a.m. UTC | #6
On Mon, Jun 19, 2023 at 08:18:13PM -0700, Eric Biggers wrote:
> > That list does not help, quite the opposite - libsodium seems all but
> > abandoned (last release 5 years ago, an unmaintained project is not
> > exactly what you want for your crypto primitives)
> 
> libsodium has an active maintainer.  Last commit was 3 months ago.

Sorry, I hadn't run 'git pull' recently.  The last commit to libsodium was
actually today!  I don't know why the maintainer isn't tagging new releases more
frequently.  The last, 1.0.18, was 4 years ago.  But it seems unfair to call an
active project "abandoned" just because of lack of tagged releases.

Anyway, this is basically a moot point now, since in v3 of this patch I dropped
the mentions of any specific crypto libraries...

- Eric
James Bottomley June 20, 2023, 12:43 p.m. UTC | #7
On Tue, 2023-06-20 at 01:42 +0100, Luca Boccassi wrote:
> On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org>
> wrote:
> > On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
[...]
> > > > +- fs-verity builtin signatures are in PKCS#7 format, and the
> > > > public
> > > > +  keys are in X.509 format.  These data formats are
> > > > unnecessarily
> > > > +  complex and prone to vulnerabilities.  (fs-verity builtin
> > > > signatures
> > > > +  were made to use these formats because other kernel
> > > > subsystems, such
> > > > +  as the module loader, unfortunately used these formats
> > > > already.
> > > > +  Note, these formats should still be used when they are the
> > > > only
> > > > +  option to have signatures at all.)  Userspace signature
> > > > verification
> > > > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > > > and the
> > > > +  associated kernel attack surface.  Userspace also has the
> > > > +  flexibility to choose simpler formats.  For example,
> > > > consider using
> > > > +  straightforward Ed25519 keys and signatures with `libsodium
> > > > + 
> > > > <https://libsodium.gitbook.io/doc/public-key_cryptography/publi
> > > > c-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).
> > > > +
> > > > +  If you are considering making use of "advanced" features of
> > > > X.509
> > > > +  and/or PKCS#7, please also keep in mind that these
> > > > "advanced"
> > > > +  features do not always work as intended with the kernel. 
> > > > For
> > > > +  example, the kernel does not check X.509 certificate
> > > > validity times.
> > > 
> > > Sorry but this still reads as way too opinionated and generic,
> > > rather than being fsverity-specific.
> > > 
> > > Please simplify to convey the same
> > > message in more concise way, perhaps something along these lines:
> > > 
> > > - fs-verity builtin signatures are in PKCS#7 format, and the
> > > public keys are in X.509 format. IMA appraisal, which supports
> > > fs-verity, uses a custom signature format rather than PKCS#7 and
> > > X.509 for public keys. Alternative formats for signatures and
> > > public keys are not supported for builtin signatures or IMA
> > > appraisal. For fully flexible and customized signature and public
> > > keys formats, while also avoiding to expose the kernel to
> > > untrusted input, signature verification can be implemented by a
> > > trusted userspace component as described at <pointer to
> > > appropriate section>
> > 
> > That is not the same message at all, as it omits the mention of the
> > disadvantages of PKCS#7 and X.509 compared to raw signatures, which
> > was the main point.  So no, I don't think your version would be
> > better.
> 
> The 'disadvantages' are your personal opinions. It's fine to have
> opinions, it's less fine to present them as if they were industry
> consensus in public project-wide documentation.
> 
> > It seems that what is going on here is that you are invested
> > heavily into existing X.509 and PKCS#7 based systems, and as a
> > result you do not want the problems with these formats to be
> > described anywhere in the kernel documentation.  That is
> > understandable, but that is a special interest that should not be
> > catered to here.  This documentation is trying to help users make
> > a decision of what type of signature to use in new systems.  And
> > yes, it is fsverity specific documentation, but there is no way for
> > it to make the needed point without dicussing the underlying data
> > formats.
> 
> Industry standards are by very definition the opposite of 'special
> interests'. Look, I tried my best to provide constructive and
> actionable feedback in previous replies, but given you seem only
> interested in casting aspersions and hijacking kernel documentation
> to promote the latest proprietary google-toy-of-the-month:

OK, could I try to share the perspective of of someone who's worked in
security with users for a long time. I get that every security expert
thinks standard key formats are insecure, usually for two reasons

   1. In security simpler is better
   2. They can always come up with a simple, totally secure, key
      representation

The problem with the above is that it means every tool invented by a
different security expert has a different key format and we don't have
any interoperability between them. This is great from a security point
of view but terrible for usability: users want both interoperability
and pluggability (particularly the ability to use tokens or other
security devices in place of keys). Our security systems are not coded
for absolute security (otherwise they would be completely balkanized);
they're a tradeoff between usability and security. There's actually
even a security reason for this: a security tool no-one can use isn't
contributing to the security of the ecosystem, however internally
secure and up to date the tool is. It's not just Google who has a habit
of special coding security systems to work on Android (with Titan-M), I
can't use wireguard on Linux because I long ago mandated that critical
things like VPN keys should be in secure keystores and I use the TPM on
Linux for openvpn. Wireguard, because it has a simple and secure key
format, won't plug into the kernel TPM system without a whole load of
speciallised glue, so I'm stuck with openvpn.

The debate you two are having now is about the most important thing we
can ever decide: where to draw the line between usability and security.
If you conduct it in a more constructive manner and try to see each
other's point of view (Eric: how would I use a TPM with fsverity on
Linux? and Luca: how much interoperability does fsverity really need
for debian?) because usability without security is as equally useless
as security without usability.

Regards,

James
Eric Biggers June 20, 2023, 4:45 p.m. UTC | #8
Hi James,

On Tue, Jun 20, 2023 at 08:43:00AM -0400, James Bottomley wrote:
> On Tue, 2023-06-20 at 01:42 +0100, Luca Boccassi wrote:
> > On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org>
> > wrote:
> > > On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> [...]
> > > > > +- fs-verity builtin signatures are in PKCS#7 format, and the
> > > > > public
> > > > > +  keys are in X.509 format.  These data formats are
> > > > > unnecessarily
> > > > > +  complex and prone to vulnerabilities.  (fs-verity builtin
> > > > > signatures
> > > > > +  were made to use these formats because other kernel
> > > > > subsystems, such
> > > > > +  as the module loader, unfortunately used these formats
> > > > > already.
> > > > > +  Note, these formats should still be used when they are the
> > > > > only
> > > > > +  option to have signatures at all.)  Userspace signature
> > > > > verification
> > > > > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > > > > and the
> > > > > +  associated kernel attack surface.  Userspace also has the
> > > > > +  flexibility to choose simpler formats.  For example,
> > > > > consider using
> > > > > +  straightforward Ed25519 keys and signatures with `libsodium
> > > > > + 
> > > > > <https://libsodium.gitbook.io/doc/public-key_cryptography/publi
> > > > > c-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).
> > > > > +
> > > > > +  If you are considering making use of "advanced" features of
> > > > > X.509
> > > > > +  and/or PKCS#7, please also keep in mind that these
> > > > > "advanced"
> > > > > +  features do not always work as intended with the kernel. 
> > > > > For
> > > > > +  example, the kernel does not check X.509 certificate
> > > > > validity times.
> > > > 
> > > > Sorry but this still reads as way too opinionated and generic,
> > > > rather than being fsverity-specific.
> > > > 
> > > > Please simplify to convey the same
> > > > message in more concise way, perhaps something along these lines:
> > > > 
> > > > - fs-verity builtin signatures are in PKCS#7 format, and the
> > > > public keys are in X.509 format. IMA appraisal, which supports
> > > > fs-verity, uses a custom signature format rather than PKCS#7 and
> > > > X.509 for public keys. Alternative formats for signatures and
> > > > public keys are not supported for builtin signatures or IMA
> > > > appraisal. For fully flexible and customized signature and public
> > > > keys formats, while also avoiding to expose the kernel to
> > > > untrusted input, signature verification can be implemented by a
> > > > trusted userspace component as described at <pointer to
> > > > appropriate section>
> > > 
> > > That is not the same message at all, as it omits the mention of the
> > > disadvantages of PKCS#7 and X.509 compared to raw signatures, which
> > > was the main point.  So no, I don't think your version would be
> > > better.
> > 
> > The 'disadvantages' are your personal opinions. It's fine to have
> > opinions, it's less fine to present them as if they were industry
> > consensus in public project-wide documentation.
> > 
> > > It seems that what is going on here is that you are invested
> > > heavily into existing X.509 and PKCS#7 based systems, and as a
> > > result you do not want the problems with these formats to be
> > > described anywhere in the kernel documentation.  That is
> > > understandable, but that is a special interest that should not be
> > > catered to here.  This documentation is trying to help users make
> > > a decision of what type of signature to use in new systems.  And
> > > yes, it is fsverity specific documentation, but there is no way for
> > > it to make the needed point without dicussing the underlying data
> > > formats.
> > 
> > Industry standards are by very definition the opposite of 'special
> > interests'. Look, I tried my best to provide constructive and
> > actionable feedback in previous replies, but given you seem only
> > interested in casting aspersions and hijacking kernel documentation
> > to promote the latest proprietary google-toy-of-the-month:
> 
> OK, could I try to share the perspective of of someone who's worked in
> security with users for a long time. I get that every security expert
> thinks standard key formats are insecure, usually for two reasons
> 
>    1. In security simpler is better
>    2. They can always come up with a simple, totally secure, key
>       representation
> 
> The problem with the above is that it means every tool invented by a
> different security expert has a different key format and we don't have
> any interoperability between them. This is great from a security point
> of view but terrible for usability: users want both interoperability
> and pluggability (particularly the ability to use tokens or other
> security devices in place of keys). Our security systems are not coded
> for absolute security (otherwise they would be completely balkanized);
> they're a tradeoff between usability and security. There's actually
> even a security reason for this: a security tool no-one can use isn't
> contributing to the security of the ecosystem, however internally
> secure and up to date the tool is. It's not just Google who has a habit
> of special coding security systems to work on Android (with Titan-M), I
> can't use wireguard on Linux because I long ago mandated that critical
> things like VPN keys should be in secure keystores and I use the TPM on
> Linux for openvpn. Wireguard, because it has a simple and secure key
> format, won't plug into the kernel TPM system without a whole load of
> speciallised glue, so I'm stuck with openvpn.
> 
> The debate you two are having now is about the most important thing we
> can ever decide: where to draw the line between usability and security.
> If you conduct it in a more constructive manner and try to see each
> other's point of view (Eric: how would I use a TPM with fsverity on
> Linux? and Luca: how much interoperability does fsverity really need
> for debian?) because usability without security is as equally useless
> as security without usability.

By using a TPM with fsverity, I assume you mean doing the signing with a TPM?
(Note that signing usually occurs on a system different from the one that
verifies the signature.)  Even that implies the use of X.509, which I don't
think it does, I don't think it implies the use of PKCS#7.  And it certainly
doesn't imply the use of in-kernel signature verification.  So don't immediately
see the relevance of this.

In any case, I think my latest patch (v3) is very reasonable.  I've worked hard
to address Luca's concerns, and the patch makes it clear that there are multiple
options for verification of signatures of fsverity files, both with and without
PKCS#7, and with and without in-kernel verification.  Each has its advantages
and disadvantages.  I would encourage you to read the latest patch fully; I
think Luca's comments are making it sound like it says things that it doesn't
actually say (and I also made some further revisions v2 => v3).

- Eric
Colin Walters June 20, 2023, 4:55 p.m. UTC | #9
On Mon, Jun 19, 2023, at 6:10 PM, Eric Biggers 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.

FWIW,
Reviewed-by: Colin Walters <walters@verbum.org>

And I agree with your points enough that our project using fsverity will switch to documenting userspace crypto first.

I did spend a few minutes reading through `git log -p crypto/asymmetric_keys` and beyond the links to the bugfixes you already sent, I think you're clearly within your rights to add this text to the fsverity docs.
Luca Boccassi June 20, 2023, 7:15 p.m. UTC | #10
On Tue, 20 Jun 2023 at 04:18, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 20, 2023 at 01:42:20AM +0100, Luca Boccassi wrote:
> > On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> > > > > diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> > > > > index ede672dedf110..c33f783e74953 100644
> > > > > --- a/Documentation/filesystems/fsverity.rst
> > > > > +++ b/Documentation/filesystems/fsverity.rst
> > > > <...>
> > > > > +- Trusted userspace code.  Often, the userspace code that accesses
> > > > > +  files can be trusted to authenticate them.  Consider e.g. an
> > > > > +  application that wants to authenticate data files before using them,
> > > > > +  or an application loader that is part of the operating system (which
> > > > > +  is already authenticated in a different way, such as by being loaded
> > > > > +  from a read-only partition that uses dm-verity) and that wants to
> > > > > +  authenticate applications before loading them.  In these cases, this
> > > > > +  trusted userspace code can authenticate a file's contents by
> > > > > +  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
> > > > > +  verifying a signature of it 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.
> > > >
> > > > Text looks good to me now, but please just drop the last sentence with
> > > > the external projects links, as it seems best left as an exercise for
> > > > the reader to find their preferred tooling that is appropriate to be
> > > > used at the time of reading, as this will get out of date fast.
> > > >
> > > > <...>
> > >
> > > Well, a significant part of the motivation for this patch is that the "exercise
> > > for the reader" approach has already been tried, for several years, but it is
> > > not working well.  People don't know what to use and need a little more help.
> > >
> > > I'm planning to add some example code to fsverity-utils, probably using
> > > libsodium.  After that, I'll make this documentation link to there.  But for
> > > now, I think the last two sentences of the above paragraph are helpful.
> >
> > That list does not help, quite the opposite - libsodium seems all but
> > abandoned (last release 5 years ago, an unmaintained project is not
> > exactly what you want for your crypto primitives)
>
> libsodium has an active maintainer.  Last commit was 3 months ago.
>
> Also note that simple crypto libraries that focus on offering modern
> cryptographic algorithms don't need to be updated often.  You actually *don't*
> really want to have to constantly update your crypto library because they e.g.
> messed up their ASN.1 parser.  If there is no ASN.1, that cannot happen.

Maintenance is not only CVE-driven, no release for almost 5 years in a
security-critical component to me looks like a red flag. And I
maintain projects using libsodium (zeromq), so it's not like I've
never seen it before. But when a package is shipped bit-by-bit
identical across two different Debian releases it's usually not a sign
of good health for the project.

> > tink appears to
> > be one of those google's sources-available-proprietary projects, which
> > means that, as with everything else that google throws over the wall,
> > it's at permanent risk of getting killed with little notice, among
> > many other problems.
>
> Tink is an open source project licensed under the Apache license.  Non-Google
> contributions to Tink are being accepted on GitHub.  If Google stops maintaining
> Tink, it can be forked, like any other open source project.

The website that was linked said a CLA was required. On top of that, I
can't speak for this one in particular, but my experience with other
google projects that were allegedly open source has been atrocious -
the 'real' source tree being proprietary and private, with code
occasionally synced to Github, and external contributors requiring an
internal corporate 'sponsor' to get their changes merged internally
first, passing invisible, private CI and getting reverted without much
consideration when some more important, internal change conflicted,
and with no way to find out till the internal real tree was thrown
over the wall again at some point in the future. All of this while
claiming it was just a normal open source project on Github. That's
not open source, that's proprietary with sources-available.

> Anyway, my intent was simply to list some modern crypto libraries.  One of them
> just happens to be from Google.  I'm sorry if it may have appeared like I listed
> a Google project because of it being from Google.  That was not my intent.
>
> > If you really want to suggest something, OpenSSL
> > seems like an appropriate choice of a widely available, supported and
> > well-known solution that is the most likely to still be around and
> > maintained in 5/10 years.
>
> Sure.  Which is why I included OpenSSL in the list.
>
> Anyway, I'll go ahead and take your suggestion to omit the mentions of any
> specific crypto libraries.  I do agree that it could use some more thought, and
> the main place for this information will be in fsverity-utils which will have
> the example code.

Thank you. It seems best to keep such recommendations, if any,
together with the example code, yes.
Luca Boccassi June 20, 2023, 7:34 p.m. UTC | #11
On Tue, 20 Jun 2023 at 13:59, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Tue, 2023-06-20 at 01:42 +0100, Luca Boccassi wrote:
> > On Tue, 20 Jun 2023 at 00:49, Eric Biggers <ebiggers@kernel.org>
> > wrote:
> > > On Tue, Jun 20, 2023 at 12:04:39AM +0100, Luca Boccassi wrote:
> [...]
> > > > > +- fs-verity builtin signatures are in PKCS#7 format, and the
> > > > > public
> > > > > +  keys are in X.509 format.  These data formats are
> > > > > unnecessarily
> > > > > +  complex and prone to vulnerabilities.  (fs-verity builtin
> > > > > signatures
> > > > > +  were made to use these formats because other kernel
> > > > > subsystems, such
> > > > > +  as the module loader, unfortunately used these formats
> > > > > already.
> > > > > +  Note, these formats should still be used when they are the
> > > > > only
> > > > > +  option to have signatures at all.)  Userspace signature
> > > > > verification
> > > > > +  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> > > > > and the
> > > > > +  associated kernel attack surface.  Userspace also has the
> > > > > +  flexibility to choose simpler formats.  For example,
> > > > > consider using
> > > > > +  straightforward Ed25519 keys and signatures with `libsodium
> > > > > +
> > > > > <https://libsodium.gitbook.io/doc/public-key_cryptography/publi
> > > > > c-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).
> > > > > +
> > > > > +  If you are considering making use of "advanced" features of
> > > > > X.509
> > > > > +  and/or PKCS#7, please also keep in mind that these
> > > > > "advanced"
> > > > > +  features do not always work as intended with the kernel.
> > > > > For
> > > > > +  example, the kernel does not check X.509 certificate
> > > > > validity times.
> > > >
> > > > Sorry but this still reads as way too opinionated and generic,
> > > > rather than being fsverity-specific.
> > > >
> > > > Please simplify to convey the same
> > > > message in more concise way, perhaps something along these lines:
> > > >
> > > > - fs-verity builtin signatures are in PKCS#7 format, and the
> > > > public keys are in X.509 format. IMA appraisal, which supports
> > > > fs-verity, uses a custom signature format rather than PKCS#7 and
> > > > X.509 for public keys. Alternative formats for signatures and
> > > > public keys are not supported for builtin signatures or IMA
> > > > appraisal. For fully flexible and customized signature and public
> > > > keys formats, while also avoiding to expose the kernel to
> > > > untrusted input, signature verification can be implemented by a
> > > > trusted userspace component as described at <pointer to
> > > > appropriate section>
> > >
> > > That is not the same message at all, as it omits the mention of the
> > > disadvantages of PKCS#7 and X.509 compared to raw signatures, which
> > > was the main point.  So no, I don't think your version would be
> > > better.
> >
> > The 'disadvantages' are your personal opinions. It's fine to have
> > opinions, it's less fine to present them as if they were industry
> > consensus in public project-wide documentation.
> >
> > > It seems that what is going on here is that you are invested
> > > heavily into existing X.509 and PKCS#7 based systems, and as a
> > > result you do not want the problems with these formats to be
> > > described anywhere in the kernel documentation.  That is
> > > understandable, but that is a special interest that should not be
> > > catered to here.  This documentation is trying to help users make
> > > a decision of what type of signature to use in new systems.  And
> > > yes, it is fsverity specific documentation, but there is no way for
> > > it to make the needed point without dicussing the underlying data
> > > formats.
> >
> > Industry standards are by very definition the opposite of 'special
> > interests'. Look, I tried my best to provide constructive and
> > actionable feedback in previous replies, but given you seem only
> > interested in casting aspersions and hijacking kernel documentation
> > to promote the latest proprietary google-toy-of-the-month:
>
> OK, could I try to share the perspective of of someone who's worked in
> security with users for a long time. I get that every security expert
> thinks standard key formats are insecure, usually for two reasons
>
>    1. In security simpler is better
>    2. They can always come up with a simple, totally secure, key
>       representation
>
> The problem with the above is that it means every tool invented by a
> different security expert has a different key format and we don't have
> any interoperability between them. This is great from a security point
> of view but terrible for usability: users want both interoperability
> and pluggability (particularly the ability to use tokens or other
> security devices in place of keys). Our security systems are not coded
> for absolute security (otherwise they would be completely balkanized);
> they're a tradeoff between usability and security. There's actually
> even a security reason for this: a security tool no-one can use isn't
> contributing to the security of the ecosystem, however internally
> secure and up to date the tool is. It's not just Google who has a habit
> of special coding security systems to work on Android (with Titan-M), I
> can't use wireguard on Linux because I long ago mandated that critical
> things like VPN keys should be in secure keystores and I use the TPM on
> Linux for openvpn. Wireguard, because it has a simple and secure key
> format, won't plug into the kernel TPM system without a whole load of
> speciallised glue, so I'm stuck with openvpn.
>
> The debate you two are having now is about the most important thing we
> can ever decide: where to draw the line between usability and security.
> If you conduct it in a more constructive manner and try to see each
> other's point of view (Eric: how would I use a TPM with fsverity on
> Linux? and Luca: how much interoperability does fsverity really need
> for debian?) because usability without security is as equally useless
> as security without usability.

You make very good points, it's always about finding compromises. For
my use case, interoperability is fundamental: if I needed to have a
completely different infrastructure, signing, tooling and maintenance
strategy for dm-verity, fs-verity, kernel modules, livepatching, UEFI,
TPM signed policies and who knows what else, I'd lose my mind. But I
am happy to compromise and ack the latest revision, and I've already
done so.

Kind regards,
Luca Boccassi
diff mbox series

Patch

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index ede672dedf110..c33f783e74953 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -38,20 +38,14 @@  fail at runtime.
 Use cases
 =========
 
-By itself, the base fs-verity feature only provides integrity
-protection, i.e. detection of accidental (non-malicious) corruption.
+By itself, fs-verity only provides integrity protection, i.e.
+detection of accidental (non-malicious) corruption.
 
 However, because fs-verity makes retrieving the file hash extremely
 efficient, it's primarily meant to be used as a tool to support
 authentication (detection of malicious modifications) or auditing
 (logging file hashes before use).
 
-Trusted userspace code (e.g. operating system code running on a
-read-only partition that is itself authenticated by dm-verity) can
-authenticate the contents of an fs-verity file by using the
-`FS_IOC_MEASURE_VERITY`_ ioctl to retrieve its hash, then verifying a
-digital signature of it.
-
 A standard file hash could be used instead of fs-verity.  However,
 this is inefficient if the file is large and only a small portion may
 be accessed.  This is often the case for Android application package
@@ -69,24 +63,34 @@  still be used on read-only filesystems.  fs-verity is for files that
 must live on a read-write filesystem because they are independently
 updated and potentially user-installed, so dm-verity cannot be used.
 
-The base fs-verity feature is a hashing mechanism only; actually
-authenticating the files may be done by:
-
-* Userspace-only
-
-* Builtin signature verification + userspace policy
-
-  fs-verity optionally supports a simple signature verification
-  mechanism where users can configure the kernel to require that
-  all fs-verity files be signed by a key loaded into a keyring;
-  see `Built-in signature verification`_.
-
-* Integrity Measurement Architecture (IMA)
-
-  IMA supports including fs-verity file digests and signatures in the
-  IMA measurement list and verifying fs-verity based file signatures
-  stored as security.ima xattrs, based on policy.
-
+fs-verity does not mandate a particular scheme for authenticating its
+file hashes.  (Similarly, dm-verity does not mandate a particular
+scheme for authenticating its block device root hashes.)  Options for
+authenticating fs-verity file hashes include:
+
+- Trusted userspace code.  Often, the userspace code that accesses
+  files can be trusted to authenticate them.  Consider e.g. an
+  application that wants to authenticate data files before using them,
+  or an application loader that is part of the operating system (which
+  is already authenticated in a different way, such as by being loaded
+  from a read-only partition that uses dm-verity) and that wants to
+  authenticate applications before loading them.  In these cases, this
+  trusted userspace code can authenticate a file's contents by
+  retrieving its fs-verity digest using `FS_IOC_ENABLE_VERITY`_, then
+  verifying a signature of it 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.
+
+- Integrity Measurement Architecture (IMA).  IMA supports fs-verity
+  file digests as an alternative to its traditional full file digests.
+  "IMA appraisal" enforces that files contain a valid, matching
+  signature in their "security.ima" extended attribute, as controlled
+  by the IMA policy.  For more information, see the IMA documentation.
+
+- Trusted userspace code in combination with `Built-in signature
+  verification`_.  This approach should be used only with great care.
 
 User API
 ========
@@ -111,8 +115,7 @@  follows::
     };
 
 This structure contains the parameters of the Merkle tree to build for
-the file, and optionally contains a signature.  It must be initialized
-as follows:
+the file.  It must be initialized as follows:
 
 - ``version`` must be 1.
 - ``hash_algorithm`` must be the identifier for the hash algorithm to
@@ -129,12 +132,14 @@  as follows:
   file or device.  Currently the maximum salt size is 32 bytes.
 - ``salt_ptr`` is the pointer to the salt, or NULL if no salt is
   provided.
-- ``sig_size`` is the size of the signature in bytes, or 0 if no
-  signature is provided.  Currently the signature is (somewhat
-  arbitrarily) limited to 16128 bytes.  See `Built-in signature
-  verification`_ for more information.
-- ``sig_ptr``  is the pointer to the signature, or NULL if no
-  signature is provided.
+- ``sig_size`` is the size of the builtin signature in bytes, or 0 if no
+  builtin signature is provided.  Currently the builtin signature is
+  (somewhat arbitrarily) limited to 16128 bytes.
+- ``sig_ptr``  is the pointer to the builtin signature, or NULL if no
+  builtin signature is provided.  A builtin signature is only needed
+  if the `Built-in signature verification`_ feature is being used.  It
+  is not needed for IMA appraisal, and it is not needed if the file
+  signature is being handled entirely in userspace.
 - All reserved fields must be zeroed.
 
 FS_IOC_ENABLE_VERITY causes the filesystem to build a Merkle tree for
@@ -158,7 +163,7 @@  fatal signal), no changes are made to the file.
 FS_IOC_ENABLE_VERITY can fail with the following errors:
 
 - ``EACCES``: the process does not have write access to the file
-- ``EBADMSG``: the signature is malformed
+- ``EBADMSG``: the builtin signature is malformed
 - ``EBUSY``: this ioctl is already running on the file
 - ``EEXIST``: the file already has verity enabled
 - ``EFAULT``: the caller provided inaccessible memory
@@ -168,10 +173,10 @@  FS_IOC_ENABLE_VERITY can fail with the following errors:
   reserved bits are set; or the file descriptor refers to neither a
   regular file nor a directory.
 - ``EISDIR``: the file descriptor refers to a directory
-- ``EKEYREJECTED``: the signature doesn't match the file
-- ``EMSGSIZE``: the salt or signature is too long
-- ``ENOKEY``: the fs-verity keyring doesn't contain the certificate
-  needed to verify the signature
+- ``EKEYREJECTED``: the builtin signature doesn't match the file
+- ``EMSGSIZE``: the salt or builtin signature is too long
+- ``ENOKEY``: the ".fs-verity" keyring doesn't contain the certificate
+  needed to verify the builtin signature
 - ``ENOPKG``: fs-verity recognizes the hash algorithm, but it's not
   available in the kernel's crypto API as currently configured (e.g.
   for SHA-512, missing CONFIG_CRYPTO_SHA512).
@@ -180,8 +185,8 @@  FS_IOC_ENABLE_VERITY can fail with the following errors:
   support; or the filesystem superblock has not had the 'verity'
   feature enabled on it; or the filesystem does not support fs-verity
   on this file.  (See `Filesystem support`_.)
-- ``EPERM``: the file is append-only; or, a signature is required and
-  one was not provided.
+- ``EPERM``: the file is append-only; or, a builtin signature is
+  required and one was not provided.
 - ``EROFS``: the filesystem is read-only
 - ``ETXTBSY``: someone has the file open for writing.  This can be the
   caller's file descriptor, another open file descriptor, or the file
@@ -270,9 +275,9 @@  This ioctl takes in a pointer to the following structure::
 - ``FS_VERITY_METADATA_TYPE_DESCRIPTOR`` reads the fs-verity
   descriptor.  See `fs-verity descriptor`_.
 
-- ``FS_VERITY_METADATA_TYPE_SIGNATURE`` reads the signature which was
-  passed to FS_IOC_ENABLE_VERITY, if any.  See `Built-in signature
-  verification`_.
+- ``FS_VERITY_METADATA_TYPE_SIGNATURE`` reads the builtin signature
+  which was passed to FS_IOC_ENABLE_VERITY, if any.  See `Built-in
+  signature verification`_.
 
 The semantics are similar to those of ``pread()``.  ``offset``
 specifies the offset in bytes into the metadata item to read from, and
@@ -299,7 +304,7 @@  FS_IOC_READ_VERITY_METADATA can fail with the following errors:
   overflowed
 - ``ENODATA``: the file is not a verity file, or
   FS_VERITY_METADATA_TYPE_SIGNATURE was requested but the file doesn't
-  have a built-in signature
+  have a builtin signature
 - ``ENOTTY``: this type of filesystem does not implement fs-verity, or
   this ioctl is not yet implemented on it
 - ``EOPNOTSUPP``: the kernel was not configured with fs-verity
@@ -347,8 +352,8 @@  non-verity one, with the following exceptions:
   with EIO (for read()) or SIGBUS (for mmap() reads).
 
 - If the sysctl "fs.verity.require_signatures" is set to 1 and the
-  file is not signed by a key in the fs-verity keyring, then opening
-  the file will fail.  See `Built-in signature verification`_.
+  file is not signed by a key in the ".fs-verity" keyring, then
+  opening the file will fail.  See `Built-in signature verification`_.
 
 Direct access to the Merkle tree is not supported.  Therefore, if a
 verity file is copied, or is backed up and restored, then it will lose
@@ -433,20 +438,25 @@  root hash as well as other fields such as the file size::
 Built-in signature verification
 ===============================
 
-With CONFIG_FS_VERITY_BUILTIN_SIGNATURES=y, fs-verity supports putting
-a portion of an authentication policy (see `Use cases`_) in the
-kernel.  Specifically, it adds support for:
+CONFIG_FS_VERITY_BUILTIN_SIGNATURES=y adds supports for in-kernel
+verification of fs-verity builtin signatures.
+
+**IMPORTANT**!  Please take great care before using this feature.
+It is not the only way to do signatures with fs-verity, and the
+alternatives (such as userspace signature verification, and IMA
+appraisal) can be much better.  It's also easy to fall into a trap
+of thinking this feature solves more problems than it actually does.
+
+Enabling this option adds the following:
 
-1. At fs-verity module initialization time, a keyring ".fs-verity" is
-   created.  The root user can add trusted X.509 certificates to this
-   keyring using the add_key() system call, then (when done)
-   optionally use keyctl_restrict_keyring() to prevent additional
-   certificates from being added.
+1. At boot time, the kernel creates a keyring named ".fs-verity".  The
+   root user can add trusted X.509 certificates to this keyring using
+   the add_key() system call.
 
 2. `FS_IOC_ENABLE_VERITY`_ accepts a pointer to a PKCS#7 formatted
    detached signature in DER format of the file's fs-verity digest.
-   On success, this signature is persisted alongside the Merkle tree.
-   Then, any time the file is opened, the kernel will verify the
+   On success, the ioctl persists the signature alongside the Merkle
+   tree.  Then, any time the file is opened, the kernel verifies the
    file's actual digest against this signature, using the certificates
    in the ".fs-verity" keyring.
 
@@ -454,8 +464,8 @@  kernel.  Specifically, it adds support for:
    When set to 1, the kernel requires that all verity files have a
    correctly signed digest as described in (2).
 
-fs-verity file digests must be signed in the following format, which
-is similar to the structure used by `FS_IOC_MEASURE_VERITY`_::
+The data that the signature as described in (2) must be a signature of
+is the fs-verity file digest in the following format::
 
     struct fsverity_formatted_digest {
             char magic[8];                  /* must be "FSVerity" */
@@ -464,13 +474,61 @@  is similar to the structure used by `FS_IOC_MEASURE_VERITY`_::
             __u8 digest[];
     };
 
-fs-verity's built-in signature verification support is meant as a
-relatively simple mechanism that can be used to provide some level of
-authenticity protection for verity files, as an alternative to doing
-the signature verification in userspace or using IMA-appraisal.
-However, with this mechanism, userspace programs still need to check
-that the verity bit is set, and there is no protection against verity
-files being swapped around.
+That's it.  It should be emphasized again that fs-verity builtin
+signatures are not the only way to do signatures with fs-verity.  See
+`Use cases`_ for an overview of ways in which fs-verity can be used.
+fs-verity builtin signatures have some major limitations that should
+be carefully considered before using them:
+
+- Builtin signature verification does *not* make the kernel enforce
+  that any files actually have fs-verity enabled.  Thus, it is not a
+  complete authentication policy.  Currently, if it is used, the only
+  way to complete the authentication policy is for trusted userspace
+  code to explicitly check whether files have fs-verity enabled with a
+  signature before they are accessed.  (With
+  fs.verity.require_signatures=1, just checking whether fs-verity is
+  enabled suffices.)  But, in this case the trusted userspace code
+  could just store the signature alongside the file and verify it
+  itself using a cryptographic library, instead of using this feature.
+
+- Builtin signature verification uses the same set of public keys for
+  all fs-verity enabled files on the system.  Different keys cannot be
+  trusted for different files; each key is all or nothing.
+
+- The sysctl fs.verity.require_signatures applies system-wide.
+  Setting it to 1 only works when all users of fs-verity on the system
+  agree that it should be set to 1.  This limitation can prevent
+  fs-verity from being used in cases where it would be helpful.
+
+- Builtin signature verification can only use signature algorithms
+  that are supported by the kernel.  For example, the kernel does not
+  yet support Ed25519, even though this is often the signature
+  algorithm that is recommended for new cryptographic designs.
+
+- fs-verity builtin signatures are in PKCS#7 format, and the public
+  keys are in X.509 format.  These data formats are unnecessarily
+  complex and prone to vulnerabilities.  (fs-verity builtin signatures
+  were made to use these formats because other kernel subsystems, such
+  as the module loader, unfortunately used these formats already.
+  Note, these formats should still be used when they are the only
+  option to have signatures at all.)  Userspace signature verification
+  avoids having to enable CONFIG_FS_VERITY_BUILTIN_SIGNATURES and the
+  associated kernel attack surface.  Userspace also has the
+  flexibility to choose simpler formats.  For example, consider using
+  straightforward Ed25519 keys and signatures with `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).
+
+  If you are considering making use of "advanced" features of X.509
+  and/or PKCS#7, please also keep in mind that these "advanced"
+  features do not always work as intended with the kernel.  For
+  example, the kernel does not check X.509 certificate validity times.
+
+- A file's builtin signature can only be set at the same time that
+  fs-verity is being enabled on the file.  Changing or deleting the
+  builtin signature later requires re-creating the file.
 
 Filesystem support
 ==================
diff --git a/fs/verity/Kconfig b/fs/verity/Kconfig
index a7ffd718f1719..e1036e5353521 100644
--- a/fs/verity/Kconfig
+++ b/fs/verity/Kconfig
@@ -39,14 +39,14 @@  config FS_VERITY_BUILTIN_SIGNATURES
 	depends on FS_VERITY
 	select SYSTEM_DATA_VERIFICATION
 	help
-	  Support verifying signatures of verity files against the X.509
-	  certificates that have been loaded into the ".fs-verity"
-	  kernel keyring.
+	  This option adds support for in-kernel verification of
+	  fs-verity builtin signatures.
 
-	  This is meant as a relatively simple mechanism that can be
-	  used to provide an authenticity guarantee for verity files, as
-	  an alternative to IMA appraisal.  Userspace programs still
-	  need to check that the verity bit is set in order to get an
-	  authenticity guarantee.
+	  Please take great care before using this feature.  It is not
+	  the only way to do signatures with fs-verity, and the
+	  alternatives (such as userspace signature verification, and
+	  IMA appraisal) can be much better.  For details about the
+	  limitations of this feature, see
+	  Documentation/filesystems/fsverity.rst.
 
 	  If unsure, say N.
diff --git a/fs/verity/enable.c b/fs/verity/enable.c
index bd86b25ac084b..c284f46d1b535 100644
--- a/fs/verity/enable.c
+++ b/fs/verity/enable.c
@@ -208,7 +208,7 @@  static int enable_verity(struct file *filp,
 	}
 	desc->salt_size = arg->salt_size;
 
-	/* Get the signature if the user provided one */
+	/* Get the builtin signature if the user provided one */
 	if (arg->sig_size &&
 	    copy_from_user(desc->signature, u64_to_user_ptr(arg->sig_ptr),
 			   arg->sig_size)) {
diff --git a/fs/verity/open.c b/fs/verity/open.c
index f0383bef86eaa..1db5106a9c385 100644
--- a/fs/verity/open.c
+++ b/fs/verity/open.c
@@ -156,7 +156,7 @@  int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
 
 /*
  * Compute the file digest by hashing the fsverity_descriptor excluding the
- * signature and with the sig_size field set to 0.
+ * builtin signature and with the sig_size field set to 0.
  */
 static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
 			       struct fsverity_descriptor *desc,
@@ -174,7 +174,7 @@  static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
 
 /*
  * Create a new fsverity_info from the given fsverity_descriptor (with optional
- * appended signature), and check the signature if present.  The
+ * appended builtin signature), and check the signature if present.  The
  * fsverity_descriptor must have already undergone basic validation.
  */
 struct fsverity_info *fsverity_create_info(const struct inode *inode,
@@ -319,8 +319,8 @@  static bool validate_fsverity_descriptor(struct inode *inode,
 }
 
 /*
- * Read the inode's fsverity_descriptor (with optional appended signature) from
- * the filesystem, and do basic validation of it.
+ * Read the inode's fsverity_descriptor (with optional appended builtin
+ * signature) from the filesystem, and do basic validation of it.
  */
 int fsverity_get_descriptor(struct inode *inode,
 			    struct fsverity_descriptor **desc_ret)
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c
index 2aefc5565152a..f58432772d9ea 100644
--- a/fs/verity/read_metadata.c
+++ b/fs/verity/read_metadata.c
@@ -105,7 +105,7 @@  static int fsverity_read_descriptor(struct inode *inode,
 	if (res)
 		return res;
 
-	/* don't include the signature */
+	/* don't include the builtin signature */
 	desc_size = offsetof(struct fsverity_descriptor, signature);
 	desc->sig_size = 0;
 
@@ -131,7 +131,7 @@  static int fsverity_read_signature(struct inode *inode,
 	}
 
 	/*
-	 * Include only the signature.  Note that fsverity_get_descriptor()
+	 * Include only the builtin signature.  fsverity_get_descriptor()
 	 * already verified that sig_size is in-bounds.
 	 */
 	res = fsverity_read_buffer(buf, offset, length, desc->signature,
diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index b8c51ad40d3a3..72034bc71c9d9 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -5,6 +5,14 @@ 
  * Copyright 2019 Google LLC
  */
 
+/*
+ * This file implements verification of fs-verity builtin signatures.  Please
+ * take great care before using this feature.  It is not the only way to do
+ * signatures with fs-verity, and the alternatives (such as userspace signature
+ * verification, and IMA appraisal) can be much better.  For details about the
+ * limitations of this feature, see Documentation/filesystems/fsverity.rst.
+ */
+
 #include "fsverity_private.h"
 
 #include <linux/cred.h>