| Message ID | 20230801181917.8535-6-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: > 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. > > Signed-off-by: Tushar Sugandhi <tusharsu@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. Perhaps name this critical-data "tpm" and "tpm-info", similar to the SELinux "selinux" and "selinux-state". Then again, if this is TPM critical-data we should rethink what other info should be included. > 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.
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@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
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. > The ascii logging format is consistent with other event data for > critical_data event e.g. kernel_version. Then you got it wrong. > 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. > 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. > > 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. > 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.
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.
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"); + 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); + } /**
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. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima_init.c | 3 +++ security/integrity/ima/ima_main.c | 1 + 2 files changed, 4 insertions(+)