Re: [PATCH v3 0/9] bpf-lsm: Extend interoperability with IMA

[KP Singh]

On Thu, Mar 3, 2022 at 5:30 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
>
> On Thu, 2022-03-03 at 17:17 +0100, KP Singh wrote:
> > On Thu, Mar 3, 2022 at 5:05 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> > >
> > > [Cc'ing Florent, Kees]
> > >
> > > Hi Alexei,
> > >
> > > On Wed, 2022-03-02 at 14:20 -0800, Alexei Starovoitov wrote:
> > > > On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
> > > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > > implementation of integrity-focused LSMs based on eBPF.
> > > > >
> > > > > Patch 1 fixes some style issues.
> > > > >
> > > > > Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the
> > > > > measurement capability of IMA without needing to setup a policy in IMA
> > > > > (those LSMs might implement the policy capability themselves).
> > > > >
> > > > > Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
> > > > >
> > > > > Changelog
> > > > >
> > > > > v2:
> > > > > - Add better description to patch 1 (suggested by Shuah)
> > > > > - Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
> > > > > - Move declaration of bpf_ima_file_hash() at the end (suggested by
> > > > >   Yonghong)
> > > > > - Add tests to check if the digest has been recalculated
> > > > > - Add deny test for bpf_kernel_read_file()
> > > > > - Add description to tests
> > > > >
> > > > > v1:
> > > > > - Modify ima_file_hash() only and allow the usage of the function with the
> > > > >   modified behavior by eBPF-based LSMs through the new function
> > > > >   bpf_ima_file_hash() (suggested by Mimi)
> > > > > - Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash()
> > > > >   and bpf_ima_file_hash() can be called inside the implementation of
> > > > >   eBPF-based LSMs for this hook
> > > > >
> > > > > Roberto Sassu (9):
> > > > >   ima: Fix documentation-related warnings in ima_main.c
> > > > >   ima: Always return a file measurement in ima_file_hash()
> > > > >   bpf-lsm: Introduce new helper bpf_ima_file_hash()
> > > > >   selftests/bpf: Move sample generation code to ima_test_common()
> > > > >   selftests/bpf: Add test for bpf_ima_file_hash()
> > > > >   selftests/bpf: Check if the digest is refreshed after a file write
> > > > >   bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable
> > > > >   selftests/bpf: Add test for bpf_lsm_kernel_read_file()
> > > > >   selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA
> > > > >     policy
> > > >
> > > > We have to land this set through bpf-next.
> > > > Please get the Acks for patches 1 and 2, so we can proceed.
> > >
> >
> > Hi Mimi,
> >
> > > Each year in the LSS integrity status update talk, I've mentioned the
> > > eBPF integrity gaps.  I finally reached out to KP, Florent Revest, Kees
> >
> > Thanks for bringing this up and it's very timely because we have been
> > having discussion around eBPF program signing and delineating that
> > from eBPF program integrity use-cases.
> >
> > My plan is to travel to LSS (travel and visa permitting) and we can discuss
> > it more there.
> >
> > If you prefer we can also discuss it before in one of the BPF office hours:
> >
> > https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0AejEveU/edit#gid=0
>
> Sounds good.
>
> >
> > > and others, letting them know that I'm concerned about the eBPF module
> > > integrity gaps.  True there is a difference between signing the eBPF
> > > source modules versus the eBPF generated output, but IMA could at least
> > > verify the integrity of the source eBPF modules making sure they are
> > > measured, the module hash audited, and are properly signed.
> > >
> > > Before expanding the ima_file_hash() or ima_inode_hash() usage, I'd
> > > appreciate someone adding the IMA support to measure, appraise, and
> > > audit eBPF modules.  I realize that closing the eBPF integrity gaps is
> > > orthogonal to this patch set, but this patch set is not only extending
> >
> > This really is orthogonal and IMHO it does not seem rational to block this
> > patchset on it.
> >
> > > the ima_file_hash()/ima_inode_hash() usage, but will be used to
> > > circumvent IMA.  As per usual, IMA is policy based, allowing those
> >
> > I don't think they are being used to circumvent IMA but for totally
> > different use-cases (e.g. as a data point for detecting attacks).
> >
> >
> > > interested in eBPF module integrity to define IMA policy rules.
>
> That might be true for your usecase, but not Roberto's.  From the cover
> letter above, Roberto was honest in saying:
>
> Patches 2-6 give the ability to eBPF-based LSMs to take advantage of
> the measurement capability of IMA without needing to setup a policy in
> IMA (those LSMs might implement the policy capability themselves).

Currently to use the helper bpf_ima_inode_hash in LSM progs
one needs to have a basic IMA policy in place just to get a hash,
even if one does not need to use the whole feature-set provided by IMA.
These patches would be quite beneficial for this user-journey.

Even Robert's use case is to implement IMA policies in BPF this is still
fundamentally different from IMA doing integrity measurement for BPF
and blocking this patch-set on the latter does not seem rational and
I don't see how implementing integrity for BPF would avoid your
concerns.



>
> --
> thanks,
>
> Mimi
>