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
