Re: [PATCH v19 15/20] fsverity: expose verified fsverity built-in signatures to LSMs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx>

---
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




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux