Received-SPF: pass (google.com: domain of linux-kernel+bounces-99783-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 11 Mar 2024 19:57:12 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Fan Wu <wufan@linux.microsoft.com>
Cc: corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
	serge@hallyn.com, tytso@mit.edu, axboe@kernel.dk, agk@redhat.com,
	snitzer@kernel.org, eparis@redhat.com, paul@paul-moore.com,
	linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
	dm-devel@lists.linux.dev, audit@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [RFC PATCH v14 15/19] fsverity: consume builtin signature via
 LSM hook
Message-ID: <20240312025712.GE1182@sol.localdomain>
References: <1709768084-22539-1-git-send-email-wufan@linux.microsoft.com>
 <1709768084-22539-16-git-send-email-wufan@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1709768084-22539-16-git-send-email-wufan@linux.microsoft.com>

On Wed, Mar 06, 2024 at 03:34:40PM -0800, Fan Wu wrote:
> fsverity represents a mechanism to support both integrity and
> authenticity protection of a file, supporting both signed and unsigned
> digests.
> 
> An LSM which controls access to a resource based on authenticity and
> integrity of said resource, can then use this data to make an informed
> decision on the authorization (provided by the LSM's policy) of said
> claim.
> 
> This effectively allows the extension of a policy enforcement layer in
> LSM for fsverity, allowing for more granular control of how a
> particular authenticity claim can be used. For example, "all (built-in)
> signed fsverity files should be allowed to execute, but only these
> hashes are allowed to be loaded as kernel modules".
> 
> This enforcement must be done in kernel space, as a userspace only
> solution would fail a simple litmus test: Download a self-contained
> malicious binary that never touches the userspace stack. This
> binary would still be able to execute.
> 
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>

As I've said before, this commit message needs some work.  It currently doesn't
say anything about what the patch actually does.

BTW, please make sure you're Cc'ing the fsverity mailing list
(fsverity@lists.linux.dev), not fscrypt (linux-fscrypt@vger.kernel.org).

> diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
> index 13e4b18e5dbb..64618a6141ab 100644
> --- a/Documentation/filesystems/fsverity.rst
> +++ b/Documentation/filesystems/fsverity.rst
> @@ -461,7 +461,9 @@ Enabling this option adds the following:
>  
>  3. A new sysctl "fs.verity.require_signatures" is made available.
>     When set to 1, the kernel requires that all verity files have a
> -   correctly signed digest as described in (2).
> +   correctly signed digest as described in (2). Note that verification
> +   happens as long as the file's signature exists regardless the state of
> +   "fs.verity.require_signatures".
>  
>  The data that the signature as described in (2) must be a signature of
>  is the fs-verity file digest in the following format::

Doesn't anything else in this file need to be updated to document the IPE
support?

> diff --git a/fs/verity/open.c b/fs/verity/open.c
> index 6c31a871b84b..f917023255c8 100644
> --- a/fs/verity/open.c
> +++ b/fs/verity/open.c
> @@ -8,6 +8,7 @@
>  #include "fsverity_private.h"
>  
>  #include <linux/mm.h>
> +#include <linux/security.h>
>  #include <linux/slab.h>
>  
>  static struct kmem_cache *fsverity_info_cachep;
> @@ -172,12 +173,28 @@ static int compute_file_digest(const struct fsverity_hash_alg *hash_alg,
>  	return err;
>  }
>  
> +#ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES
> +static int fsverity_inode_setsecurity(struct inode *inode,
> +				      const struct fsverity_descriptor *desc)
> +{
> +	return security_inode_setsecurity(inode, FS_VERITY_INODE_SEC_NAME,
> +					  desc->signature,
> +					  le32_to_cpu(desc->sig_size), 0);

Please call it something like FS_VERITY_INODE_BUILTIN_SIG to make it clear that
it's the builtin signature.

> +}
> +#else
> +static inline int fsverity_inode_setsecurity(struct inode *inode,
> +					     const struct fsverity_descriptor *desc)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_IPE_PROP_FS_VERITY*/

The above comment mentions CONFIG_IPE_PROP_FS_VERITY, but it doesn't appear
anywhere else in the patch.

> +struct fsverity_info *fsverity_create_info(struct inode *inode,
>  					   struct fsverity_descriptor *desc)
>  {
>  	struct fsverity_info *vi;
> @@ -242,6 +259,13 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode,
>  		spin_lock_init(&vi->hash_page_init_lock);
>  	}
>  
> +	err = fsverity_inode_setsecurity(inode, desc);
> +	if (err == -EOPNOTSUPP)
> +		err = 0;

What is the "err == -EOPNOTSUPP" case intended to handle?

> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index 90c07573dd77..42f58f4e45d0 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -41,7 +41,9 @@ static struct key *fsverity_keyring;
>   * @sig_size: size of signature in bytes, or 0 if no signature
>   *
>   * If the file includes a signature of its fs-verity file digest, verify it
> - * against the certificates in the fs-verity keyring.
> + * against the certificates in the fs-verity keyring. Note that verification
> + * happens as long as the file's signature exists regardless the state of
> + * fsverity_require_signatures.

Can you please make this mention explicitly that the LSM hook is relying on that
behavior?

- Eric