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

[Paul Moore]

On Wed, Nov 1, 2023 at 10:54 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxx> wrote:
> > > On Oct  4, 2023 Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> 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@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
> > > > ---
> > > >  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@xxxxxxxxxxxxxxxxxxx/
>
> 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/

Hopefully Fan can comment on that, unless I'm forgetting some
implementation details it seems like a reasonable request.

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

While I think the commit description does provide a reasonable summary
of IPE as it relates to fsverify, I agree that the specifics of the
changes presented in the patch are lacking.  Fan, could you update the
commit description to add a paragraph explaining what IPE would do in
the security_inode_setsecurity() hook called from within
fsverity_inode_setsecurity()?

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

Fan can surely elaborate on this more, but my understanding is that
IPE can help provide the missing authorization piece.

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

We are getting a bit beyond the integration of IPE and fsverity so I
don't want to spend a lot of time here, but IPE and IMA work quite a
bit differently as they serve different purposes.  IPE provides a file
authorization mechanism that grants access based on the specified
policy and the file's underlying backing store; IPE does not measure
files like IMA to provide additional integrity checking, it relies on
the storage medium's integrity mechanisms.

I have no doubt Fan could provide a much better summary if needed, and
of course there are the documentation patches in the patchset too.

-- 
paul-moore.com