Message-ID: <aed4ed7d-9464-458a-9cc4-5d89ee9d8bb6@linux.microsoft.com>
Date: Wed, 29 May 2024 20:38:41 -0700
From: Fan Wu <wufan@...ux.microsoft.com>
To: Eric Biggers <ebiggers@...nel.org>, Paul Moore <paul@...l-moore.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org, serge@...lyn.com,
 tytso@....edu, axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
 mpatocka@...hat.com, eparis@...hat.com, linux-doc@...r.kernel.org,
 linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
 fsverity@...ts.linux.dev, linux-block@...r.kernel.org,
 dm-devel@...ts.linux.dev, audit@...r.kernel.org,
 linux-kernel@...r.kernel.org, Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v19 15/20] fsverity: expose verified fsverity built-in
 signatures to LSMs



On 5/29/2024 8:06 PM, Eric Biggers wrote:
> On Wed, May 29, 2024 at 09:46:57PM -0400, Paul Moore wrote:
>> On Fri, May 24, 2024 at 4:46 PM Fan Wu <wufan@...ux.microsoft.com> wrote:
>>>
>>> This patch enhances fsverity's capabilities to support both integrity and
>>> authenticity protection by introducing the exposure of built-in
>>> signatures through a new LSM hook. This functionality allows LSMs,
>>> e.g. IPE, to enforce policies based on the authenticity and integrity of
>>> files, specifically focusing on built-in fsverity signatures. It enables
>>> a policy enforcement layer within LSMs for fsverity, offering granular
>>> control over the usage of authenticity claims. For instance, a policy
>>> could be established to permit the execution of all files with verified
>>> built-in fsverity signatures while restricting kernel module loading
>>> from specified fsverity files via fsverity digests.
>>>
>>> The introduction of a security_inode_setintegrity() hook call within
>>> fsverity's workflow ensures that the verified built-in signature of a file
>>> is exposed to LSMs. This enables LSMs to recognize and label fsverity files
>>> that contain a verified built-in fsverity signature. This hook is invoked
>>> subsequent to the fsverity_verify_signature() process, guaranteeing the
>>> signature's verification against fsverity's keyring. This mechanism is
>>> crucial for maintaining system security, as it operates in kernel space,
>>> effectively thwarting attempts by malicious binaries to bypass user space
>>> stack interactions.
>>>
>>> The second to last commit in this patch set will add a link to the IPE
>>> documentation in fsverity.rst.
>>>
>>> Signed-off-by: Deven Bowers <deven.desai@...ux.microsoft.com>
>>> Signed-off-by: Fan Wu <wufan@...ux.microsoft.com>
>>>
>>> ---
>>> v1-v6:
>>>    + Not present
>>>
>>> v7:
>>>    Introduced
>>>
>>> v8:
>>>    + Split fs/verity/ changes and security/ changes into separate patches
>>>    + Change signature of fsverity_create_info to accept non-const inode
>>>    + Change signature of fsverity_verify_signature to accept non-const inode
>>>    + Don't cast-away const from inode.
>>>    + Digest functionality dropped in favor of:
>>>      ("fs-verity: define a function to return the integrity protected
>>>        file digest")
>>>    + Reworded commit description and title to match changes.
>>>    + Fix a bug wherein no LSM implements the particular fsverity @name
>>>      (or LSM is disabled), and returns -EOPNOTSUPP, causing errors.
>>>
>>> v9:
>>>    + No changes
>>>
>>> v10:
>>>    + Rename the signature blob key
>>>    + Cleanup redundant code
>>>    + Make the hook call depends on CONFIG_FS_VERITY_BUILTIN_SIGNATURES
>>>
>>> v11:
>>>    + No changes
>>>
>>> v12:
>>>    + Add constification to the hook call
>>>
>>> v13:
>>>    + No changes
>>>
>>> v14:
>>>    + Add doc/comment to built-in signature verification
>>>
>>> v15:
>>>    + Add more docs related to IPE
>>>    + Switch the hook call to security_inode_setintegrity()
>>>
>>> v16:
>>>    + Explicitly mention "fsverity builtin signatures" in the commit
>>>      message
>>>    + Amend documentation in fsverity.rst
>>>    + Fix format issue
>>>    + Change enum name
>>>
>>> v17:
>>>    + Fix various documentation issues
>>>    + Use new enum name LSM_INT_FSVERITY_BUILTINSIG_VALID
>>>
>>> v18:
>>>    + Fix typos
>>>    + Move the inode_setintegrity hook call into fsverity_verify_signature()
>>>
>>> v19:
>>>    + Cleanup code w.r.t inode_setintegrity hook refactoring
>>> ---
>>>   Documentation/filesystems/fsverity.rst | 23 +++++++++++++++++++++--
>>>   fs/verity/signature.c                  | 18 +++++++++++++++++-
>>>   include/linux/security.h               |  1 +
>>>   3 files changed, 39 insertions(+), 3 deletions(-)
>>
>> Eric, can you give this patch in particular a look to make sure you
>> are okay with everything?  I believe Fan has addressed all of your
>> previous comments and it would be nice to have your Ack/Review tag if
>> you are okay with the current revision.
> 
> Sorry, I've just gotten a bit tired of finding so many basic issues in this
> patchset even after years of revisions.
> 
> This patch in particular is finally looking better.  There are a couple issues
> that I still see.  (BTW, you're welcome to review it too to help find these
> things, given that you seem to have an interest in getting this landed...):
> 
>> +	err = security_inode_setintegrity(inode,
>> +					  LSM_INT_FSVERITY_BUILTINSIG_VALID,
>> +					  signature,
>> +					  le32_to_cpu(sig_size));
> 
> This is doing le32_to_cpu() on a variable of type size_t, which will do the
> wrong thing on big endian systems and will generate a 'sparse' warning.
> 
Sorry for the mistake. As sig_size is already converted in open.c, there 
is indeed no need to call this function again. I will remove this 
unnecessary conversion.

> Also, the commit message still incorrectly claims that this patch allows
> "restricting kernel module loading from specified fsverity files via fsverity
> digests".  As I said before (sigh...), this is not correct as that can be done
> without this patch.
> 
> - Eric

As for the commit message, my intention was to provide an example of a 
policy that with the patch IPE can enforce, not to claim that this 
specific restriction requires the patch. However, I will remove it as it 
seems to be causing confusion.
-Fan

Powered by blists - more mailing lists