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

Handle incompatibility between parameters and unique in TPublic #41

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 force-pushed the nt--invalid-public-area-edge-cases branch from 2c57170 to d72d666 Compare February 5, 2025 17:01
@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