Re: [RFC V2] IMA Log Snapshotting Design Proposal

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

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:
> 
> ...
> 
> > > Okay, we are starting to get closer, but I'm still missing the part
> > > where you say "if you do X, Y, and Z, I'll accept and merge the
> > > solution."  Can you be more explicit about what approach(es) you would
> > > be willing to accept upstream?
> >
> > Included with what is wanted/needed is an explanation as to my concerns
> > with the existing proposal.
> >
> > First we need to differentiate between kernel and uhserspace
> > requirements.  (The "snapshotting" design proposal intermixes them.)
> >
> > From the kernel persective, the Log Snapshotting Design proposal "B.1
> > Goals" is very nice, but once the measurement list can be trimmed it is
> > really irrelevant.  Userspace can do whatever it wants with the
> > measurement list records.  So instead of paying lip service to what
> > should be done, just call it as it is - trimming the measurement list.
> 
> Fair enough.  I personally think it is nice to have a brief discussion
> of how userspace might use a kernel feature, but if you prefer to drop
> that part of the design doc I doubt anyone will object very strongly.
> 
> > From the kernel perspective there needs to be a method of trimming N
> > number of records from the head of the measurement list.  In addition
> > to the existing securityfs "runtime measurement list",  defining a new
> > securityfs file containing the current count of in memory measurement
> > records would be beneficial.
> 
> I imagine that should be trivial to implement and I can't imagine
> there being any objection to that.
> 
> 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.

...

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

> 
> > Of course properly document the integrity
> > implications and repercussions of the new Kconfig that allows trimming
> > the measurement list.
> 
> Of course.
> 
> > Defining a simple "trim" marker measurement record would be a visual
> > indication that the measurement list has been trimmed.  I might even
> > have compared it to the "boot_aggregate".  However, the proposed marker
> > based on TPM PCRs requires pausing extending the measurement list.
> 
> ...
> 
> > 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). 

> 
> > The new critical-data record and trimming the measurement list should
> > be disjoint features.  If the first record after trimming the
> > measurement list should be the critical-data record, then trim the
> > measurement list up to that point.
> 
> I disagree about the snapshot_aggregate record being disjoint from the
> measurement log, but I suspect Tushar and Sush are willing to forgo
> the snapshot_aggregate if that is a blocker from your perspective.

> Once again, the main goal is the ability to manage the size of the
> measurement log; while having a snapshot_aggregate that can be used to
> establish a root of trust similar to the boot_aggregate is nice, it is
> not a MUST have.

The problem isn't the "snapshot_aggregate" critical data record per-se, 
but pausing adding measurements to the IMA measurement list and
extending the PCR to calculate it.

(Perhaps including other information, like the number of IMA
measurements before or after reading each TPM PCR read, would eliminate
the need for pausing the measurement list.)

> > From a userspace perspective, trimming the measurement list is a major
> change and will break existing attestation requests, unless the change
> is transparent.  Removing "snapshots"/"shards" will of course break
> attestation requests.  Refer to Stefan's suggestions:
>https://lore.kernel.org/linux-integrity/1ed2d72c-4cb2-48b3-bb0f-b0877fc1e9ca@linux.ibm.com/ > 
> You will note that Sush and I replied to Stefan two weeks ago.

Yes, I saw.  This might be a good place, as you suggested, "to have a
brief discussion
of how userspace might use a kernel feature".  Perhaps rename this
thread to differentiate it from the kernel design.

-- 
thanks,

Mimi