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

Must assertions with attestation include attested credential data? #1792

Closed
emlun opened this issue Sep 1, 2022 · 3 comments
Closed

Must assertions with attestation include attested credential data? #1792

emlun opened this issue Sep 1, 2022 · 3 comments

Comments

@emlun
Copy link
Member

emlun commented Sep 1, 2022

In PR #1663, we're about to add the capability to return attestation statements with assertions. It is currently not explicit whether authenticatorData should contain attested credential data or not when there's an attestation statement with the assertion. This is likely significant for the security of the attestation, so I think it's worth documenting the decision and rationale somewhere more visible than PR review comments.

I believe that if assertions with attestation do not include attested credential data in authenticatorData, then attestation statements can be forged by transplanting the attestation signature onto a different key. Please verify my reasoning below.

Setup

Let's assume that assertions with attestation do not include attested credential data in authenticatorData.

packed attestations sign over authData || hash, so if the public key isn't in authData, then there is no signature chain from the attestation key to the credential key. Both keys will have signed the same data, and the RP already knows the credential public key.

Let's assume that an attacker has a credential registered with some RP. The credential was registered without attestation, and with a signature count of zero.

Forging a packed attestation

In practice, the client has control of all parts of authData || hash except the signature counter. An attacker can forge an attestation statement as follows:

  1. Invoke makeCredential on a compliant authenticator to obtain a credential ID. Discard any other results.
  2. Invoke getAssertion on the authenticator with that credential ID, and request packed attestation in the assertion. The authenticator will sign over rpIdHash || flags || signCount || hash with its attestation key. The client controls rpIdHash and hash, and flags is essentially a constant. flags and signCount are also echoed back to the attacker with the assertion.
  3. Generate an assertion over the same rpIdHash || flags || signCount || hash.
  4. Append the attestation statement from the authenticator.

The attacker has now generated an assertion

  • whose signature has signed over rpIdHash || flags || signCount || hash
  • whose attestationObject contains a sig that has signed over rpIdHash || flags || signCount || hash
  • whose credential private key is not the same as the credential private key that was created by the authenticator.

Thus the attacker has successfully transplanted the attestation statement onto a key outside the authenticator's control, allowing the attacker to spoof any compliant authenticator.

Conclusion

authenticatorData must include attested credential data when the assertion includes an attestation statement. Otherwise, packed attestations can be forged.

@emlun
Copy link
Member Author

emlun commented Sep 1, 2022

...ah, I see now that this is made unambiguous further down, in the RP assertion verification procedure:

    1. Verify that the AT bit in the flags field of authData is set, indicating that attested credential data is included.

Still, I think it should be explicit in the authenticator operations as well. And now we have this issue as documentation of what exactly goes wrong if authenticators implement this incorrectly.

@Firstyear
Copy link
Contributor

Complete agree @emlun, the last thing we want is forging this.

@nadalin nadalin added this to the L3-WD-01 milestone Sep 7, 2022
@emlun
Copy link
Member Author

emlun commented Sep 12, 2022

Addressed in #1663 (comment), see commit d671894d.

@emlun emlun closed this as completed Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants