On 8/3/23 15:15, Mimi Zohar wrote:
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
IMA log entries can be lost due to a variety of causes, such as code bugs
or error conditions, leading to a mismatch between TPM PCRs and
the IMA log. Measuring TPM PCR update counter during ima_init would
provide a baseline counter for the number of times the TPM PCRs are
updated. The remote attestation service can compare this baseline
counter with a subsequent measured one (e.g., post-kexec soft-boot) to
identify if there are any lost IMA log events.
Measure the TPM update counter at ima init.
No need for separate patches for one line changes like this. Either
merge patches 5/6 and 6/6 or all three 4/6, 5/6, 6/6 together.
Sounds good.
I will merge 4/6, 5/6, 6/6 together.
Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
---
security/integrity/ima/ima_init.c | 3 +++
security/integrity/ima/ima_main.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 63979aefc95f..9bb18d6c2fd6 100644
--- a/security/integrity/ima/ima_init.c
+++ 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.
The ascii logging format is consistent with other event data for
critical_data event e.g. kernel_version.
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.
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.
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".
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.
As you know, event_label doesn't appear in IMA log, it can appear in IMA
policy.
Whereas event_name appears in IMA log.
I was thinking of using event_name to identify when was the info captured.
(e.g. ima_init, kexec_load, or at some other event in future).
We can either do
(a)
event_label = "tpm-info" event_name = "tpm-info-ima-init" |
"tpm-info-kexec-load" | ...
-or-
(b)
event_label = "tpm" event_name = "tpm-info"
and event_data to describe the where/when this info was captured.
e.g.
version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;event=kexec_load;
Let me know if you would prefer option (a) or (b) or something else.
SELinux "selinux" and "selinux-state". Then again, if this is TPM
critical-data we should rethink what other info should be included.
As you suggested in Patch 4/6, I will add version, number of enabled pcr
banks,
pcrUpdateCounter, and num_ima_measurements. I think we should include
the TPM
version as well (1 v/s 2).
Please let me know if you think of any other attribute to record.
return rc;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1bcd45cc5a6a..93357c245e82 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
NULL, false, NULL, 0);
fdput(f);
+
}
/**
Unnecessary change.
oops. Thanks for catching. Will fix.
~Tushar
