On Tue, 2023-11-28 at 20:06 -0500, Paul Moore wrote:
> On Tue, Nov 28, 2023 at 7:09 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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
