Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux