| Message ID | 20230801181917.8535-5-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Measuring TPM update counter in IMA | expand |
On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote: > Currently TPM update counter is not available external to the system, > for instance, a remote attestation service. It is a problem because > the service cannot easily determine if the IMA log entries are missing. > The IMA functionality needs to be extended to measure the TPM update > counter from various subsystems in Linux kernel to help detect if > the IMA log entries are missing. > > Implement a function, 'ima_measure_update_counter()' which would retrieve > the TPM update counter using the previously defined function > 'ima_tpm_get_update_counter()'. Format it as a string with the value > "update_counter=<N>;", and measure it using the function > 'ima_measure_critical_data()'. > > The function takes an event name as input, and the update counter value > is measured as part of this event. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> Explicit TPM2 quote commands do not return the quoted PCR values or the pcrCounter value. Define and include a new IMA measurement record containing the pcrCounter, other TPM info, and IMA info in the IMA measurement list to help simplify detecting a trimmed/truncated measurement list. > --- > include/linux/ima.h | 1 + > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 30 insertions(+) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 86b57757c7b1..f15f3a6a4c72 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label, > const char *event_name, > const void *buf, size_t buf_len, > bool hash, u8 *digest, size_t digest_len); > +int ima_measure_update_counter(const char *event_name); > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM > extern void ima_appraise_parse_cmdline(void); > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 4acd0e5a830f..5484bd362237 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -168,6 +168,7 @@ int __init ima_init_digests(void); > int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, > void *lsm_data); > int ima_tpm_get_update_counter(u32 *cpu_update_counter); > +int ima_measure_update_counter(const char *event_name); > > /* > * used to protect h_table and sha_table > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index d66a0a36415e..1bcd45cc5a6a 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label, > } > EXPORT_SYMBOL_GPL(ima_measure_critical_data); > > +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128 > +int ima_measure_update_counter(const char *event_name) > +{ > + int result; > + u32 update_counter = 0; > + char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE]; > + int buf_len; > + > + if (!event_name) > + return -ENOPARAM; > + > + result = ima_tpm_get_update_counter(&update_counter); > + > + if (result != 0) > + return result; > + > + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;", > + update_counter); > + > + buf_len = strlen(buf); > + > + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name, > + buf, buf_len, false, NULL, 0); > The new record should contain everything needed to verify the pcrCounter. For example, each IMA measurement record updates the pcrCounter for each TPM bank enabled. So the number of enabled TPM banks and number of IMA measurements should also be included in this record. Perhaps include a version number as well, so that if we ever want to include other information, we could. Mimi > + > + return result; > +} > +EXPORT_SYMBOL_GPL(ima_measure_update_counter); > + > static int __init init_ima(void) > { > int error;
Thanks for the review Mimi. On 8/3/23 14:42, Mimi Zohar wrote: > On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote: >> Currently TPM update counter is not available external to the system, >> for instance, a remote attestation service. It is a problem because >> the service cannot easily determine if the IMA log entries are missing. >> The IMA functionality needs to be extended to measure the TPM update >> counter from various subsystems in Linux kernel to help detect if >> the IMA log entries are missing. >> >> Implement a function, 'ima_measure_update_counter()' which would retrieve >> the TPM update counter using the previously defined function >> 'ima_tpm_get_update_counter()'. Format it as a string with the value >> "update_counter=<N>;", and measure it using the function >> 'ima_measure_critical_data()'. >> >> The function takes an event name as input, and the update counter value >> is measured as part of this event. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Explicit TPM2 quote commands do not return the quoted PCR values or the > pcrCounter value. Define and include a new IMA measurement record > containing the pcrCounter, other TPM info, and IMA info in the IMA > measurement list to help simplify detecting a trimmed/truncated > measurement list. Sounds good. >> --- >> include/linux/ima.h | 1 + >> security/integrity/ima/ima.h | 1 + >> security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 30 insertions(+) >> >> diff --git a/include/linux/ima.h b/include/linux/ima.h >> index 86b57757c7b1..f15f3a6a4c72 100644 >> --- a/include/linux/ima.h >> +++ b/include/linux/ima.h >> @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label, >> const char *event_name, >> const void *buf, size_t buf_len, >> bool hash, u8 *digest, size_t digest_len); >> +int ima_measure_update_counter(const char *event_name); >> >> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM >> extern void ima_appraise_parse_cmdline(void); >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index 4acd0e5a830f..5484bd362237 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -168,6 +168,7 @@ int __init ima_init_digests(void); >> int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, >> void *lsm_data); >> int ima_tpm_get_update_counter(u32 *cpu_update_counter); >> +int ima_measure_update_counter(const char *event_name); >> >> /* >> * used to protect h_table and sha_table >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index d66a0a36415e..1bcd45cc5a6a 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label, >> } >> EXPORT_SYMBOL_GPL(ima_measure_critical_data); >> >> +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128 >> +int ima_measure_update_counter(const char *event_name) >> +{ >> + int result; >> + u32 update_counter = 0; >> + char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE]; >> + int buf_len; >> + >> + if (!event_name) >> + return -ENOPARAM; >> + >> + result = ima_tpm_get_update_counter(&update_counter); >> + >> + if (result != 0) >> + return result; >> + >> + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;", >> + update_counter); >> + >> + buf_len = strlen(buf); >> + >> + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name, >> + buf, buf_len, false, NULL, 0); >> > The new record should contain everything needed to verify the > pcrCounter. For example, each IMA measurement record updates the > pcrCounter for each TPM bank enabled. So the number of enabled TPM > banks and number of IMA measurements should also be included in this > record. Agreed. That should be valuable information. How does the below format look like for the buf above? version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>; > > Perhaps include a version number as well, so that if we ever want to > include other information, we could. By version number, do you mean kernel_version, or a new version number specific to this record? Or something else? ~Tushar > Mimi > > >> + >> + return result; >> +} >> +EXPORT_SYMBOL_GPL(ima_measure_update_counter); >> + >> static int __init init_ima(void) >> { >> int error;
On Thu, 2023-08-03 at 16:01 -0700, Tushar Sugandhi wrote: > >> + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;", > >> + update_counter); > >> + > >> + buf_len = strlen(buf); > >> + > >> + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name, > >> + buf, buf_len, false, NULL, 0); > >> > > The new record should contain everything needed to verify the > > pcrCounter. For example, each IMA measurement record updates the > > pcrCounter for each TPM bank enabled. So the number of enabled TPM > > banks and number of IMA measurements should also be included in this > > record. > Agreed. That should be valuable information. > How does the below format look like for the buf above? > > version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>; Refer to comment in 5/6. > > Perhaps include a version number as well, so that if we ever want to > > include other information, we could. > By version number, do you mean kernel_version, or a new version > number specific to this record? Or something else? This is a record version type number. The record format shouldn't change, but we should be prepared for it changing. A single number should be fine.
On 8/3/23 18:22, Mimi Zohar wrote: > On Thu, 2023-08-03 at 16:01 -0700, Tushar Sugandhi wrote: >>>> + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;", >>>> + update_counter); >>>> + >>>> + buf_len = strlen(buf); >>>> + >>>> + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name, >>>> + buf, buf_len, false, NULL, 0); >>>> >>> The new record should contain everything needed to verify the >>> pcrCounter. For example, each IMA measurement record updates the >>> pcrCounter for each TPM bank enabled. So the number of enabled TPM >>> banks and number of IMA measurements should also be included in this >>> record. >> Agreed. That should be valuable information. >> How does the below format look like for the buf above? >> >> version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>; > Refer to comment in 5/6. Responded. >>> Perhaps include a version number as well, so that if we ever want to >>> include other information, we could. >> By version number, do you mean kernel_version, or a new version >> number specific to this record? Or something else? > This is a record version type number. The record format shouldn't > change, but we should be prepared for it changing. A single number > should be fine. > Sounds good.
diff --git a/include/linux/ima.h b/include/linux/ima.h index 86b57757c7b1..f15f3a6a4c72 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, bool hash, u8 *digest, size_t digest_len); +int ima_measure_update_counter(const char *event_name); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 4acd0e5a830f..5484bd362237 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -168,6 +168,7 @@ int __init ima_init_digests(void); int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, void *lsm_data); int ima_tpm_get_update_counter(u32 *cpu_update_counter); +int ima_measure_update_counter(const char *event_name); /* * used to protect h_table and sha_table diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index d66a0a36415e..1bcd45cc5a6a 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label, } EXPORT_SYMBOL_GPL(ima_measure_critical_data); +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128 +int ima_measure_update_counter(const char *event_name) +{ + int result; + u32 update_counter = 0; + char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE]; + int buf_len; + + if (!event_name) + return -ENOPARAM; + + result = ima_tpm_get_update_counter(&update_counter); + + if (result != 0) + return result; + + scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;", + update_counter); + + buf_len = strlen(buf); + + result = ima_measure_critical_data("tpm_pcr_update_counter", event_name, + buf, buf_len, false, NULL, 0); + + return result; +} +EXPORT_SYMBOL_GPL(ima_measure_update_counter); + static int __init init_ima(void) { int error;
Currently TPM update counter is not available external to the system, for instance, a remote attestation service. It is a problem because the service cannot easily determine if the IMA log entries are missing. The IMA functionality needs to be extended to measure the TPM update counter from various subsystems in Linux kernel to help detect if the IMA log entries are missing. Implement a function, 'ima_measure_update_counter()' which would retrieve the TPM update counter using the previously defined function 'ima_tpm_get_update_counter()'. Format it as a string with the value "update_counter=<N>;", and measure it using the function 'ima_measure_critical_data()'. The function takes an event name as input, and the update counter value is measured as part of this event. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- include/linux/ima.h | 1 + security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 30 insertions(+)