* [PATCH] fsverity: improve documentation for builtin signature support
@ 2023-06-15 23:05 Eric Biggers
2023-06-16 1:10 ` Luca Boccassi
0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2023-06-15 23:05 UTC (permalink / raw
To: fsverity
Cc: linux-integrity, linux-doc, Colin Walters, Alexander Larsson,
Luca Boccassi, Victor Hsieh
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>
---
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
@@ -38,8 +38,8 @@ 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
@@ -69,24 +69,29 @@ 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. 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.
+
+- 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 +116,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 +133,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 +164,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 +174,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 +186,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 +276,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 +305,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 +353,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 +439,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 +465,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 +475,58 @@ 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 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).
+
+ 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>
base-commit: 74836ecbc5c7565d24a770917644e96af3e98d25
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-15 23:05 [PATCH] fsverity: improve documentation for builtin signature support Eric Biggers
@ 2023-06-16 1:10 ` Luca Boccassi
2023-06-16 2:17 ` Eric Biggers
0 siblings, 1 reply; 9+ messages in thread
From: Luca Boccassi @ 2023-06-16 1:10 UTC (permalink / raw
To: Eric Biggers
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Fri, 16 Jun 2023 at 00:07, 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>
> ---
>
> 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.
> +- 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.
Kind regards,
Luca Boccassi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 1:10 ` Luca Boccassi
@ 2023-06-16 2:17 ` Eric Biggers
2023-06-16 9:31 ` Roberto Sassu
2023-06-16 13:27 ` Luca Boccassi
0 siblings, 2 replies; 9+ messages in thread
From: Eric Biggers @ 2023-06-16 2:17 UTC (permalink / raw
To: Luca Boccassi
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
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@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>
> > ---
> >
> > 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.
As for under what circumstances these risks are worth taking anyway, it's an
interesting question. Part of my concern is actually about people who don't
actually use any of these integrity/authenticity oriented kernel features at
all. They are getting no benefit from them, and we don't want to create
problems for them. But, by CONFIG_FS_VERITY_BUILTIN_SIGNATURES being in their
kernel config, their system is potentially opened up to exploits by
FS_IOC_ENABLE_VERITY(malicious_pkcs7_signature). Or just by
CONFIG_X509_CERTIFICATE_PARSER being in their kernel config, their system is
potentially opened up to exploits by sys_add_key(malicious_X509_certificate).
They could eliminate this risk by disabling these kernel config options.
So I think that mentioning the risks of processing these data formats in the
kernel is useful. Though it maybe should be made clear that attack surface
mainly comes from these features being configured into the kernel, not whether
they're actually being used.
- Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 2:17 ` Eric Biggers
@ 2023-06-16 9:31 ` Roberto Sassu
2023-06-16 12:57 ` Luca Boccassi
2023-06-16 13:27 ` Luca Boccassi
1 sibling, 1 reply; 9+ messages in thread
From: Roberto Sassu @ 2023-06-16 9:31 UTC (permalink / raw
To: Eric Biggers, Luca Boccassi
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
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@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>
> > > ---
> > >
> > > 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@huaweicloud.com/
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...
Roberto
> As for under what circumstances these risks are worth taking anyway, it's an
> interesting question. Part of my concern is actually about people who don't
> actually use any of these integrity/authenticity oriented kernel features at
> all. They are getting no benefit from them, and we don't want to create
> problems for them. But, by CONFIG_FS_VERITY_BUILTIN_SIGNATURES being in their
> kernel config, their system is potentially opened up to exploits by
> FS_IOC_ENABLE_VERITY(malicious_pkcs7_signature). Or just by
> CONFIG_X509_CERTIFICATE_PARSER being in their kernel config, their system is
> potentially opened up to exploits by sys_add_key(malicious_X509_certificate).
> They could eliminate this risk by disabling these kernel config options.
>
> So I think that mentioning the risks of processing these data formats in the
> kernel is useful. Though it maybe should be made clear that attack surface
> mainly comes from these features being configured into the kernel, not whether
> they're actually being used.
>
> - Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 9:31 ` Roberto Sassu
@ 2023-06-16 12:57 ` Luca Boccassi
2023-06-16 13:15 ` Roberto Sassu
0 siblings, 1 reply; 9+ messages in thread
From: Luca Boccassi @ 2023-06-16 12:57 UTC (permalink / raw
To: Roberto Sassu
Cc: Eric Biggers, fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Fri, 16 Jun 2023 at 10:51, Roberto Sassu
<roberto.sassu@huaweicloud.com> 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@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>
> > > > ---
> > > >
> > > > 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@huaweicloud.com/
>
> 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.
Kind regards,
Luca Boccassi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 12:57 ` Luca Boccassi
@ 2023-06-16 13:15 ` Roberto Sassu
0 siblings, 0 replies; 9+ messages in thread
From: Roberto Sassu @ 2023-06-16 13:15 UTC (permalink / raw
To: Luca Boccassi
Cc: Eric Biggers, fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Fri, 2023-06-16 at 13:57 +0100, Luca Boccassi wrote:
> On Fri, 16 Jun 2023 at 10:51, Roberto Sassu
> <roberto.sassu@huaweicloud.com> 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@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>
> > > > > ---
> > > > >
> > > > > 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@huaweicloud.com/
> >
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 2:17 ` Eric Biggers
2023-06-16 9:31 ` Roberto Sassu
@ 2023-06-16 13:27 ` Luca Boccassi
2023-06-17 4:51 ` Eric Biggers
1 sibling, 1 reply; 9+ messages in thread
From: Luca Boccassi @ 2023-06-16 13:27 UTC (permalink / raw
To: Eric Biggers
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Fri, 16 Jun 2023 at 03:17, Eric Biggers <ebiggers@kernel.org> 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@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>
> > > ---
> > >
> > > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-16 13:27 ` Luca Boccassi
@ 2023-06-17 4:51 ` Eric Biggers
2023-06-19 19:39 ` Luca Boccassi
0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2023-06-17 4:51 UTC (permalink / raw
To: Luca Boccassi
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Fri, Jun 16, 2023 at 02:27:28PM +0100, Luca Boccassi wrote:
> > 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.
My point was not that there are bugs, but rather that there are *unnecessary*
bugs (many with possible security impact) that are directly caused by the
complexities of these formats versus the alternatives.
> 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.
I'll add a note that PKCS#7 and X.509 should still be used in situations where
they are the only option. I think that would handle your main concern here,
which is that people might misunderstand the paragraph as recommending using no
signatures, instead of signatures using a PKCS#7 and X.509 based system.
I don't think it would be appropriate to remove the paragraph entirely. It
provides useful information that helps users decide what type of signatures to
use. I understand that people who are invested greatly into PKCS#7 and X.509
based systems might be resistant to learning about the problems with these
formats, but that is to be expected.
- Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fsverity: improve documentation for builtin signature support
2023-06-17 4:51 ` Eric Biggers
@ 2023-06-19 19:39 ` Luca Boccassi
0 siblings, 0 replies; 9+ messages in thread
From: Luca Boccassi @ 2023-06-19 19:39 UTC (permalink / raw
To: Eric Biggers
Cc: fsverity, linux-integrity, linux-doc, Colin Walters,
Alexander Larsson, Victor Hsieh
On Sat, 17 Jun 2023 at 05:51, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 16, 2023 at 02:27:28PM +0100, Luca Boccassi wrote:
> > > 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.
>
> My point was not that there are bugs, but rather that there are *unnecessary*
> bugs (many with possible security impact) that are directly caused by the
> complexities of these formats versus the alternatives.
>
> > 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.
>
> I'll add a note that PKCS#7 and X.509 should still be used in situations where
> they are the only option. I think that would handle your main concern here,
> which is that people might misunderstand the paragraph as recommending using no
> signatures, instead of signatures using a PKCS#7 and X.509 based system.
>
> I don't think it would be appropriate to remove the paragraph entirely. It
> provides useful information that helps users decide what type of signatures to
> use. I understand that people who are invested greatly into PKCS#7 and X.509
> based systems might be resistant to learning about the problems with these
> formats, but that is to be expected.
Given this is a kernel doc (as opposed to, say, fsverity-utils's
readme), my concern is with having confusing and contradicting
documentation. If you reword it to make it specific to fsverity or so,
it would probably be fine.
Kind regards,
Luca Boccassi
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-19 19:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-15 23:05 [PATCH] fsverity: improve documentation for builtin signature support Eric Biggers
2023-06-16 1:10 ` Luca Boccassi
2023-06-16 2:17 ` Eric Biggers
2023-06-16 9:31 ` Roberto Sassu
2023-06-16 12:57 ` Luca Boccassi
2023-06-16 13:15 ` Roberto Sassu
2023-06-16 13:27 ` Luca Boccassi
2023-06-17 4:51 ` Eric Biggers
2023-06-19 19:39 ` Luca Boccassi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).