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

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Nov 01, 2023 at 08:40:06PM -0400, Paul Moore wrote:
> On Mon, Oct 23, 2023 at 11:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Oct  4, 2023 Fan Wu <wufan@linux.microsoft.com> 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>
> > > ---
> > > 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
> > > ---
> > >  fs/verity/fsverity_private.h |  2 +-
> > >  fs/verity/open.c             | 26 +++++++++++++++++++++++++-
> > >  include/linux/fsverity.h     |  2 ++
> > >  3 files changed, 28 insertions(+), 2 deletions(-)
> >
> > We need an ACK from some VFS folks on this.
> 
> Eric and/or Ted, can we get either an ACK or some feedback on this patch?
> 
> For reference, the full patchset can be found on lore at the link below:
> 
> https://lore.kernel.org/linux-security-module/1696457386-3010-1-git-send-email-wufan@linux.microsoft.com/

Well, technically I already gave some (minor) feedback on this exact patch, and
it's not yet been addressed:
https://lore.kernel.org/linux-security-module/20231005022707.GA1688@quark.localdomain/

Of course, it would also be nice if the commit message mentioned what the patch
actually does.

At a higher level, I've said before, I'm not super happy about the use of
fsverity builtin signatures growing.  (For some of the reasons why, see the
guidance in the fsverity documentation at
https://docs.kernel.org/filesystems/fsverity.html#built-in-signature-verification)
That being said, if the people who are doing the broader review of IPE believe
this is how its fsverity integration should work, I can live with that; I don't
intend to block the IPE patchset if enough people want it to be merged.  I've
really been hoping to see engagement with the people involved in IMA, as IPE
basically duplicates/replaces IMA.  But I haven't seen that, so maybe things
need to move on without them.

- Eric