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

[Tushar Sugandhi]

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.