Re: [RFC V2] IMA Log Snapshotting Design Proposal

[Mimi Zohar <zohar@linux.ibm.com>]

On Tue, 2023-11-28 at 20:06 -0500, Paul Moore wrote:
> On Tue, Nov 28, 2023 at 7:09 AM Mimi Zohar <zohar@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@linux.ibm.com> wrote:
> > > > On Wed, 2023-11-22 at 09:22 -0500, Paul Moore wrote:
> 
> ...
> 
> > > If we are going to have a record count, I imagine it would also be
> > > helpful to maintain a securityfs file with the total size (in bytes)
> > > of the in-memory measurement log.  In fact, I suspect this will
> > > probably be more useful for those who wish to manage the size of the
> > > measurement log.
> >
> > A running number of bytes needed for carrying the measurement list
> > across kexec already exists.  This value would be affected when the
> > measurement list is trimmed.
> 
> There we go, it should be trivial to export that information via securityfs.
> 
> > > > Defining other IMA securityfs files like
> > > > how many times the measurement list has been trimmed might be
> > > > beneficial as well.
> > >
> > > I have no objection to that.  Would a total record count, i.e. a value
> > > that doesn't reset on a snapshot event, be more useful here?
> >
> > <securityfs>/ima/runtime_measurements_count already exports the total
> > number of measurement records.
> 
> I guess the question is would you want 'runtime_measurements_count' to
> reflect the current/trimmed log size or would you want it to reflect
> hthe measurements since the initial cold boot?  Presumably we would
> want to add another securityfs file to handle the case not covered by
> 'runtime_measurements_count'.

Right.  <securityfs>/ima/runtime_measurements_count is defined as the
total number of measurements since boot.  When the measurement list is
carried across kexec, it is the number of measurements since cold boot.

A new securityfs file should be defined for the current number of in
kernel memory records.  Unless the measurement list has been trimmed,
this should be the same as the runtime_measurements_count.

> 
> > > > 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.  For ease of review, this new critical-
data record should be a separate patch set from trimming the
measurement list.

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);

-- 
thanks,

Mimi