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

[Tushar Sugandhi]

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 at linux.microsoft.com>
>> ---
>>   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