Skip to content

Conversation

@nicolastemciuc
Copy link
Member

@nicolastemciuc nicolastemciuc commented Jan 31, 2025

Summary

This PR addresses an issue where the t_public.parameters and t_public.unique are incompatible by preventing the gem from raising an exception when this happens and just returning nil as result of TPM:KeyAttestation#key.

Why?

We discovered this issue after upgrading tpm-key_attestation to 0.13.1 in cedarcode/webauthn-ruby#449. During this upgrade, a test failure in the WebAuthn test suite revealed an unsupported edge case: calling the key method of KeyAttestation raises an exception when t_public.parameters and t_public.unique are incompatible.

The exception occurs in the following code:

point = OpenSSL::PKey::EC::Point.new(
group,
bn(ECC_UNCOMPRESSED_POINT_INDICATOR + unique.x.buffer.value + unique.y.buffer.value)
)

This issue went unnoticed before because key_attestation.valid? used to return false (because it failed to validate the signature), preventing key_attestation.key from being called. However, after changes to the signature validation process in #29, valid? now returns true for this scenario, which led the test to fail because the pubArea for this scenario was set up with incompatible parameters and unique attributes (because the coordinates x and y are inconsistent with its curve).

Although we haven’t encountered this scenario in real-world applications, the WebAuthn test suite helped uncover this validation gap.

@nicolastemciuc nicolastemciuc changed the title [WIP] Test when t_public curve_id differs from key curve_id [fix] Handle curve_id mismatch in KeyAttestation Jan 31, 2025
@nicolastemciuc nicolastemciuc marked this pull request as ready for review January 31, 2025 19:26
@nicolastemciuc
Copy link
Member Author

It's worth discussing (and perhaps checking the TPM 2.0 documentation) whether the valid? method in KeyAttestation should return true when key is nil

Previously, valid? returned false in this case:

def valid?
!!key
end

@nicolastemciuc nicolastemciuc changed the title [fix] Handle curve_id mismatch in KeyAttestation Handle incompatibility between parameters and unique in TPublic Feb 3, 2025
@nicolastemciuc nicolastemciuc merged commit 8b9ccbc into cedarcode:master Feb 5, 2025
89 checks passed
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.

2 participants