[RFC V2] IMA Log Snapshotting Design Proposal

[Paul Moore]

On Mon, Jan 8, 2024 at 6:48 AM Mimi Zohar <zohar at linux.ibm.com> wrote:
> On Sun, 2024-01-07 at 21:58 -0500, Paul Moore wrote:
> > On Sun, Jan 7, 2024 at 7:59 AM Mimi Zohar <zohar at linux.ibm.com> wrote:
> > > On Sat, 2024-01-06 at 18:27 -0500, Paul Moore wrote:
> > > > On Tue, Nov 28, 2023 at 9:07 PM Mimi Zohar <zohar at linux.ibm.com> wrote:
> > > > > On Tue, 2023-11-28 at 20:06 -0500, Paul Moore wrote:
> > > > > > On Tue, Nov 28, 2023 at 7:09 AM Mimi Zohar <zohar at linux.ibm.com> wrote:
> > > > > > > On Mon, 2023-11-27 at 17:16 -0500, Paul Moore wrote:
> > > > > > > > On Mon, Nov 27, 2023 at 12:08 PM Mimi Zohar <zohar at linux.ibm.com> wrote:
> > > > > > > > > On Wed, 2023-11-22 at 09:22 -0500, Paul Moore wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > > Before defining a new critical-data record, we need to decide whether
> > > > > > > > > it is really necessary or if it is redundant.  If we define a new
> > > > > > > > > "critical-data" record, can it be defined such that it doesn't require
> > > > > > > > > pausing extending the measurement list?  For example, a new simple
> > > > > > > > > visual critical-data record could contain the number of records (e.g.
> > > > > > > > > <securityfs>/ima/runtime_measurements_count) up to that point.
> > > > > > > >
> > > > > > > > What if the snapshot_aggregate was a hash of the measurement log
> > > > > > > > starting with either the boot_aggregate or the latest
> > > > > > > > snapshot_aggregate and ending on the record before the new
> > > > > > > > snapshot_aggregate?  The performance impact at snapshot time should be
> > > > > > > > minimal as the hash can be incrementally updated as new records are
> > > > > > > > added to the measurement list.  While the hash wouldn't capture the
> > > > > > > > TPM state, it would allow some crude verification when reassembling
> > > > > > > > the log.  If one could bear the cost of a TPM signing operation, the
> > > > > > > > log digest could be signed by the TPM.
> > > > > > >
> > > > > > > Other critical data is calculated, before calling
> > > > > > > ima_measure_critical_data(), which adds the record to the measurement
> > > > > > > list and extends the TPM PCR.
> > > > > > >
> > > > > > > Signing the hash shouldn't be an issue if it behaves like other
> > > > > > > critical data.
> > > > > > >
> > > > > > > In addition to the hash, consider including other information in the
> > > > > > > new critical data record (e.g. total number of measurement records, the
> > > > > > > number of measurements included in the hash, the number of times the
> > > > > > > measurement list was trimmed, etc).
> > > > > >
> > > > > > It would be nice if you could provide an explicit list of what you
> > > > > > would want hashed into a snapshot_aggregate record; the above is
> > > > > > close, but it is still a little hand-wavy.  I'm just trying to reduce
> > > > > > the back-n-forth :)
> > > > >
> > > > > What is being defined here is the first IMA critical-data record, which
> > > > > really requires some thought.
> > > >
> > > > My thinking has always been that taking a hash of the current
> > > > measurement log up to the snapshot point would be a nice
> > > > snapshot_aggregate measurement, but I'm not heavily invested in that.
> > > > To me it is more important that we find something we can all agree on,
> > > > perhaps reluctantly, so we can move forward with a solution.
> > > >
> > > > > For ease of review, this new critical-
> > > > > data record should be a separate patch set from trimming the
> > > > > measurement list.
> > > >
> > > > I see the two as linked, but if you prefer them as separate then so be
> > > > it.  Once again, the important part is to move forward with a
> > > > solution, I'm not overly bothered if it arrives in multiple pieces
> > > > instead of one.
> > >
> > > Trimming the IMA measurement list could be used in conjunction with the new IMA
> > > critical data record or independently.  Both options should be supported.
> > >
> > > 1. trim N number of records from the head of the in kernel IMA measurement list
> > > 2. intermittently include the new IMA critical data record based on some trigger
> > > 3. trim the measurement list up to the (first/last/Nth) IMA critical data record
> > >
> > > Since the two features could be used independently of each other, there is no
> > > reason to upstream them as a single patch set.  It just makes it harder to
> > > review.
> >
> > I don't see much point in recording a snapshot aggregate if you aren't
> > doing a snapshot, but it's not harmful in any way, so sure, go for it.
> > Like I said earlier, as long as the functionality is there, I don't
> > think anyone cares too much how it gets into the kernel (although
> > Tushar and Sush should comment from the perspective).
>
> Paul, there are two features:
> - trimming the measurement list
> - defining and including an IMA critical data record
>
> The original design doc combined these two features making them an "atomic"
> operation and referred to it as a snapshot.  At the time the term "snapshot" was
> an appropriate term for the IMA critical record.  Now not so much.
>
> These are two separate, independent features.  Trimming the measurement list
> should not be dependent on the IMA critical data record.  The IMA critical data
> record should not be dependent on trimming the measurement list.  Trimming the
> measurement list up to the (first/last/Nth) critical data record should be
> optional.

Mimi, do you keep missing the part in my replies where I mention that
I don't really care as long as the trimming and aggregate both make it
into the kernel?  I don't agree with your assertion that the two
pieces of functionality are independent, but it doesn't really matter
as long as the functionality is in place, userspace can be made to do
what Tushar and Sush need to do.

> > > > > As I'm sure you're aware, SElinux defines two critical-data records.
> > > > > From security/selinux/ima.c:
> > > > >
> > > > >         ima_measure_critical_data("selinux", "selinux-state",
> > > > >                                   state_str, strlen(state_str), false,
> > > > >                                   NULL, 0);
> > > > >
> > > > >         ima_measure_critical_data("selinux", "selinux-policy-hash",
> > > > >                                   policy, policy_len, true,
> > > > >                                   NULL, 0);
> > > >
> > > > Yep, but there is far more to this than SELinux.
> > >
> > > Only if you conflate the two features.
> >
> > If that is a clever retort, you'll need to elaborate a bit as it
> > doesn't make much sense to me.  The IMA measurement log snapshot is
> > independent from SELinux; the only connection is that yes, IMA does
> > measure SELinux "things" but that is no different from any other
> > system attribute that is measured by IMA.
>
> The IMA critical data record should not be that different or more difficult,
> than the SELinux critical data record.  Only if you conflate the two features
> being discussed - trimming the IMA measurement list and the IMA critical data
> record - does it become "far more".

I still don't understand why you've brought SELinux into this, it
makes no sense from my perspective as the two are "separate,
independent features".  Are you trying to draw some odd parallel
between snapshotting and IMA/SELinux?  I guess I'd suggest just
dropping this comparison and focusing on more concrete things like
"the aggregate should contain X, Y, and Z", etc. as that is something
we can all understand and use as a starting point to continue to make
forward progress.

-- 
paul-moore.com