On 8/3/23 18:18, Mimi Zohar wrote:
On Thu, 2023-08-03 at 16:34 -0700, Tushar Sugandhi wrote:
+++ b/security/integrity/ima/ima_init.c
@@ -154,5 +154,8 @@ int __init ima_init(void)
UTS_RELEASE, strlen(UTS_RELEASE), false,
NULL, 0);
+ /* Measures TPM update counter at ima_init */
+ ima_measure_update_counter("ima_init_tpm_update_counter");
+
With "ima_policy=critical_data" on the boot command line, the IMA
measurement list record looks like:
6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b
Please change the "ima_init_tpm_update_counter" to something shorter
and the hex encoded ascii string and pcr counter to something readable.
I believe you are seeing the above line in ascill_runtime_measurements log.
Yes, the ascii_runtime_measurements are suppose to be readable to the
end user.
We were passing string literals to 'buf' param in
ima_measure_critical_data().
I believe we need to convert them first.
The ascii logging format is consistent with other event data for
critical_data event e.g. kernel_version.
Then you got it wrong.
I see. I will fix it for tpm in this patch series.
I think I should spin up another series to fix it for
selinux, kernel info, DM etc.
10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf
sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version
362e332e302d7263332b
10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf
sha1:d8c187524412f74a961f2051a9529c009e700337
ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
Entries in the binary runtime measurements look readable to me.
You've inverted the meaning of the ascii and binary runtime measurement
lists. For comparison look at the ima-ng/ima-sig records.
Yup. Agreed.
ima_init_tpm_update_counter update_counter=130;
...
kexec_load_tpm_update_counte rupdate_counter=133;
Please let me know if you still want me to change the format.
OI course the ascii measurement list should be human readable.
Yup. I will make the changes as I mentioned above.
Perhaps name this critical-data "tpm" and "tpm-info", similar to the
From patch 4/6:
+ result = ima_measure_critical_data("tpm_pcr_update_counter",
event_name,
+ buf, buf_len, false, NULL, 0);
The critical_data event_label value is currently set to
"tpm_pcr_update_counter".
Why is the string so long? Long strings or variables don't make the
code or logs more understandable. Please shorten both the strings and
variables.
Agreed. I will name this "tpm" and "tpm-info" or something similar.
I can rename event_label to "tpm-info", so that the admins can filter the
event in IMA policy based on the label if needed.
The new record needs to be self containd and verifiable. The
additional info I suggested were just examples. Please take the time
to consider what needs to be included in the new record. Decide
whether this is a TPM security critical data record. Only then, decide
on the naming.
Yes. I will explore what other potential attributes can be added to
this record.
And I'll share them here for the community feedback.
