On Sun, Jan 7, 2024 at 7:59 AM Mimi Zohar <zohar@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@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:
> >
> > ...
> >
> > > > > > > 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).
> > > 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.
--
paul-moore.com
