Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ot_certs] add creator pubkey ID (serial number) to UDS certificate #21314

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

timothytrippel
Copy link
Contributor

This adds a utility function to compute attestation key IDs that are used in the "serial number" and "subject key ID" fields of attestation certificates. The attestation key ID is derived from an attestation public key using a KMAC KDF operation as described in NIST SP 800-56C.

Additionally, this updates the UDS certificate generation flow in the personalization firmware to include the creator pubkey ID field in the TBD certificate generation.

@timothytrippel timothytrippel requested a review from a team as a code owner February 12, 2024 22:29
@timothytrippel timothytrippel requested review from jadephilipoom and removed request for a team February 12, 2024 22:29
@jadephilipoom
Copy link
Contributor

This adds a utility function to compute attestation key IDs that are used in the "serial number" and "subject key ID" fields of attestation certificates. The attestation key ID is derived from an attestation public key using a KMAC KDF operation as described in NIST SP 800-56C.

A question about this setup: why does the attestation key ID need to come from something as heavy-duty as KMAC-KDF? The X.509 RFC suggests simply hashing the public key and using the hash as the ID. (It suggests SHA-1, but it also says any method that generates a unique ID is fine; I'd suggest SHA-256 given that's probably fastest on OT.)

Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am no expert on the crypto part but overall it looks good.

uint8_t creator_pub_key_id[kCertKeyIdSizeInBytes] = {0};

// Generate the UDS key ID.
uint32_t creator_pub_key_id[kOtCertPubkeyIdSizeIn32BitWords];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan of the uint32_t * to uint8_t * casting. I wonder if this should be done in ot_cert_gen_key_id to have a well-defined and self-contained implementation of this hash since this is endian-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cryptolib API mostly operates on 32-bit words. To not violate strict aliasing we can only alias with a a char type, i.e., unsigned char *. The cryptolib API requires the caller allocate space for the return value. Aliasing outside of ot_cert_gen_key_id enables all these things to hold. I am not sure I follow your concerns regarding how this would impact endianness? Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I was just pointing out that the meaning of the cast from uint32_t * to unsigned char* depends on the endianness and is therefore a bit implicit for something which is quite important but as long as we have some tests to make sure that this produces the correct result, this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has come up a few times with cryptolib. The crux of the issue is that while bytes are less ambiguous, words are safer from SCA attacks and faster, since dealing with more bits at a time obscures side-channel signal and the hardware registers mostly use words.

Like Tim said above, the current status is that the cryptolib mostly uses word buffers, but it always arranges the word buffers so that casting them directly to byte buffers with unsigned char * on Ibex always produces the right byte stream.

@timothytrippel
Copy link
Contributor Author

timothytrippel commented Feb 13, 2024

This adds a utility function to compute attestation key IDs that are used in the "serial number" and "subject key ID" fields of attestation certificates. The attestation key ID is derived from an attestation public key using a KMAC KDF operation as described in NIST SP 800-56C.

A question about this setup: why does the attestation key ID need to come from something as heavy-duty as KMAC-KDF? The X.509 RFC suggests simply hashing the public key and using the hash as the ID. (It suggests SHA-1, but it also says any method that generates a unique ID is fine; I'd suggest SHA-256 given that's probably fastest on OT.)

Admittedly, KMAC was chosen here simply because it is what was specified in the OT Certificates specification document. It seems the rationale there was this block was hardened against SCA/Fault attacks (unlike the HMAC block).

I think another choice for KMAC was the output size. The serial number field is 160 bits (convenient for a SHA1 or a cSHAKE KMAC).

Upon further reflection, I am not sure this matters much for the serial number / subject key id certificate fields, as these can easily be verified out of band, since they are based on the public key, during certificate endorsement. I could just truncate the HMAC to 160bits? Wdyt @jadephilipoom @moidx ?

@pamaury
Copy link
Contributor

pamaury commented Feb 14, 2024

Admittedly, KMAC was chosen here simply because it is what was specified in the OT Certificates specification document. It seems the rationale there was this block was hardened against SCA/Fault attacks (unlike the HMAC block).

I think another choice for KMAC was the output size. The serial number field is 160 bits (convenient for a SHA1 or a cSHAKE KMAC).

Upon further reflection, I am not sure this matters much for the serial number / subject key id certificate fields, as these can easily be verified out of band, since they are based on the public key, during certificate endorsement. I could just truncate the HMAC to 160bits? Wdyt @jadephilipoom @moidx ?

My understanding was that the key ID is something that anyone can derive from the public key anyway (provided they know the formula) so not sure the hardening brings anything. Is the time difference between HMAC and KMAC significant? After all, the attestation is not done very frequently.

@timothytrippel
Copy link
Contributor Author

Yes, agreed @pamaury, I think the same applies for truncating an HMAC, just that collision resistance is reduced. But that should not matter much here I don't think.

@jadephilipoom
Copy link
Contributor

Upon further reflection, I am not sure this matters much for the serial number / subject key id certificate fields, as these can easily be verified out of band, since they are based on the public key, during certificate endorsement. I could just truncate the HMAC to 160bits? Wdyt @jadephilipoom @moidx ?

The language in the X.509 RFC specifically prefers hash functions[0], so I'd be in favor of using SHA-256 or SHA3-256 and truncating to 160 bits, rather than either HMAC or KMAC. My understanding is that the key identifier doesn't really need to be cryptographically secure, just deterministically based on the key so it can be used to distinguish certificates.

[0] I'm looking at this passage here from RFC 5280:

Two common methods for generating key
identifiers from the public key are described in Section 4.2.1.2.
Where a key identifier has not been previously established, this
specification RECOMMENDS use of one of these methods for generating
keyIdentifiers or use of a similar method that uses a different hash
algorithm. Where a key identifier has been previously established,
the CA SHOULD use the previously established identifier.

The parameter names for the cert builder function matched the parameter
names for the TbsCert builder function which was slightly misleading.

Signed-off-by: Tim Trippel <[email protected]>
This updates the UDS cert generation code to add the creator pubkey ID,
which is generated via a truncated SHA256 operation over the public key
itself. The creator pubkey ID becomes the serial number for the UDS
certificate.

Signed-off-by: Tim Trippel <[email protected]>
@timothytrippel
Copy link
Contributor Author

Sounds sensible to me. I updated the PR to reflect the suggested changes. LMK what you think @pamaury @jadephilipoom .

@timothytrippel timothytrippel merged commit d866ebe into lowRISC:master Feb 15, 2024
32 checks passed
@timothytrippel timothytrippel deleted the ft-perso-certgen-pt2 branch February 15, 2024 08:29
@pamaury
Copy link
Contributor

pamaury commented Feb 15, 2024

This has been merged but I am generally in favor of following the specification closely if possible so using HMAC sounds like a good idea to me.

@jadephilipoom
Copy link
Contributor

Post-merge LGTM! (Looks like GitHub doesn't let you explicitly approve after merge anymore.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants