Issue #421 has been updated by Michał Żygowski. Related to Feature #420: Use standard format of TPM event log added ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1163 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Sergii Dmytruk. If there are no objections, I'll use an API like this instead: ``` struct tpm_digest { const uint8_t *hash; size_t len; enum vb2_hash_algorithm hash_type; }; /** * Ask vboot for a digest and extend a TPM PCR with it. * @param pcr sets the pcr index * @param digests An array of digests terminated by an entry with VB2_HASH_NONE * @param name sets additional info where the digest comes from * @return TPM_SUCCESS on success. If not a tpm error is returned */ uint32_t tpm_extend_pcr(int pcr, const tpm_digest *digests, const char *name); ``` Passing in length and arrays which must be kept in sync seems to make things needlessly complicated. ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1182 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Sergii Dmytruk.

Can you explain what use case you have that requires you to use multiple algorithms?

I'll let Krystian and Michał correct me, but I'm not sure if we have an actual need for multiple algorithms right away. The API change is probably motivated by the fact that agile format supports multiple algorithms unlike the coreboot-specific one.

And why is it not enough to just call tpm_extend_pcr() several times, once for each algorithm?

This won't work well with `tcpa_log_add_table_entry()`, which will then add a log entry per algorithm. `tpm_extend_pcr()` would be updated for consistency here, but it's not strictly necessary. ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1184 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Krystian Hebel. Julius Werner wrote in #note-4:

Can you explain what use case you have that requires you to use multiple algorithms?

This allows for greater flexibility, where multiple coexisting programs may have different expectations, e.g. one is old enough to not know anything but SHA1, and another that considers SHA1 not secure enough.

And why is it not enough to just call tpm_extend_pcr() several times, once for each algorithm?

This would call `tcpa_log_add_table_entry()` (or its corresponding new version), which would create multiple entries. This is not allowed by specification [1], 10.1.6:

For each Hash algorithm enumerated in the TCG_PCClientPCREvent entry, there SHALL be a corresponding digest in all TCG_PCR_EVENT2 structures.

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_... ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1191 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Sergii Dmytruk. Julius Werner wrote in #note-8:

are you planning to make a bunch of Kconfigs to select this

vboot2 supports 4 matching hashes, so 4 corresponding options.

Basically, I understand that the log format *allows* multiple algorithms, and that's fine. And I'm also not saying that we can never expand it to allow logging multiple algorithms if a real need comes up in the future. I'm just saying there's no need to make things any more complicated than they need to be right now and implement support for a bunch of stuff in the lower level APIs that the higher level APIs wouldn't actually be using yet. If your goal for the time being is just to support the new log format, why don't you just do that in a way where tpm_extend_pcr() always creates a log entry with exactly one algorithm? If we ever get to the point where we actually need to log multiple algorithms somewhere we can still expand that later.

This approach will require returning to these changes, understanding them and spec again, changing, testing and reviewing changes again and only then adding a second/third algorithm. This can even prevent such development in the future by making the cost of change high enough. I think we might as well implement agile format properly (no fixed-size buffers in structures, any number of algorithms) right away. Lower level API is called in two places, both of which need to loop over an array of enabled algorithms, which doesn't complicate the code much, just generalizes it. ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1201 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Sergii Dmytruk. Julius Werner wrote in #note-10:

I still feel strongly that we shouldn't overcomplicate APIs

It would actually simplify the API by making parameter lists shorter and input data better grouped.

increase maintenance burden by implementing options that nobody has a use case for right now and likely never will.

`skiboot` writes both SHA1 and SHA256 hashes to TPM2 log. I didn't count it as a use case because so far we were using TPM1.2, but it does show existence of logs with multiple hashes in the wild. ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1208 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Krystian Hebel.

I don't know what skiboot is... is that coreboot? Do they have a real use case for having both hashes in the log or is it just another bootloader where someone decided "might as well write all the hashes in advance just because the spec technically allows for it"?

My question is: is there any user of coreboot right now who would actually turn on multiple hashes for production purposes because otherwise something they need doesn't work for them?

Skiboot is a payload currently used by OpenPOWER systems [1], like QEMU POWER9 or Talos II that is slowly being upstreamed [2]. With additional changes both to format of log created by coreboot and to the payload itself (latter breaks TPM2.0 logs), it could be persuaded to work with one hash, we did that as PoC for our setup that uses TPM1.2 (due to supply chain issues and low ability of I2C TPMs in general). However, instead of following existing standards, be it TCG or coreboot, such approach creates yet another one. Having the ability to use more than one would make transition to TPM2.0 easier, if not no-op. Since we are going to have to change event log generation code anyway, we want to do it properly, instead of putting another half-measure in place. So no, there are no users of coreboot that would use it right now, but there will (hopefully) soon be. As this is a change that will impact many platforms, we want to push it upstream sooner rather than later, leaving as much time for review and testing as possible. [1] https://github.com/coreboot/coreboot/tree/master/payloads/external/skiboot [2] https://review.coreboot.org/q/topic:talos-2 ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1217 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account

Issue #421 has been updated by Michał Żygowski. There may not be a strong need to have multiple hashes in the log entries yet. I have also recently spotted Intel fTPMs that can have only one PCR bank active at a time, so only discrete TPMs are capable of having multiple PCR banks active. Let's stick with a single hash algorithm in the entries for now. If needed we may add additional algorithms later indeed. ---------------------------------------- Cleanup #421: Change API of functions taking hash as an argument https://ticket.coreboot.org/issues/421#change-1343 * Author: Krystian Hebel * Status: New * Priority: Normal * Target version: none * Start date: 2022-10-12 ---------------------------------------- All existing functions that take a digest as an input assume that only one hashing algorithm is used at a time. Crypto agile format entry can (and should) log every used PCR bank in one entry for a given measurement. To make it work, some of the arguments must be changed, e.g.: - pass number of algorithms used; - instead of algorithm ID, pass a pointer to array of such IDs, with size equal to above; - instead of hash, pass a pointer to array of hashes, with size and order as above. -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: https://ticket.coreboot.org/my/account