Re: [RFC PATCH v14 15/19] fsverity: consume builtin signature via LSM hook

[Fan Wu <wufan@linux.microsoft.com>]

On 3/11/2024 7:57 PM, Eric Biggers wrote:
> 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.
Apologies, I must have missed your previous comment. I appreciate your 
feedback and will ensure that the commit message clearly describes what 
the patch does in the next version. Thank you for pointing this out.

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

> 
>> 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?
> 
Yes, I can add more details of IPE support in the built-in signature 
section.

>> 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.
>
Thanks for the suggestion, I will take the name.

>> +}
>> +#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.
>
The CONFIG_IPE_PROP_FS_VERITY KCONFIG switch is introduced in a 
subsequent patch. Merging these two patches might not be the best 
approach. I can switch to using CONFIG_FS_VERITY_BUILTIN_SIGNATURES 
instead, which may be more appropriate for the current context.

>> +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?
>
The -EOPNOTSUPP is designed to signal situations where the called LSM 
hook does not support associating the passed name with a value, but the 
hook call itself does not encounter any errors. Also, -EOPNOTSUPP is the 
default return value of security_inode_setsecurity when no LSM has 
registered the hook. In summary, it indicates that no hook call 
succeeded, but no critical error occurred.

>> 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?
Sure, I can add this info.
-Fan
> 
> - Eric