Bug 3516

Summary:	ssh-keygen when creating sk fido keys does not create sufficient data for attestation verification.
Product:	Portable OpenSSH	Reporter:	William Brown <william.brown>
Component:	ssh-keygen	Assignee:	Assigned to nobody <unassigned-bugs>
Status:	NEW ---
Severity:	normal	CC:	djm, pedro
Priority:	P5
Version:	9.1p1
Hardware:	All
OS:	All

Description William Brown 2023-01-03 18:29:54 AEDT

An ssh key backed with a fido2 device can be created by ssh-keygen with the command:

ssh-keygen-t ecdsa-sk -f ~/.ssh/id_ecdsa_sk

However in some environments it may be desired to associate an attestation with these keys to perform verification of the supplier of the fido2 device. ssh-keygen allows writing of this attestation with:

ssh-keygen -t ecdsa-sk -O write-attestation=~/.ssh/id_ecdsa_sk.attest -f ~/.ssh/id_ecdsa_sk

This id_ecdsa_sk.attest file is a binary blob generated from 'fill_attestation_blob' per: https://github.com/openssh/openssh-portable/blob/c46f6fed419167c1671e4227459e108036c760f8/ssh-sk.c#L437  with the data populated from https://github.com/openssh/openssh-portable/blob/ff9809fdfd1d9a91067bb14a77d176002edb153c/sk-usbhid.c#L1004 

However, this information is not sufficient for a valid attestation to be performed as the webauthn packed type requires the concatenation of auth_data_bytes and collected_client_data_hash. The verification procedure for "packed" attestations (which are the type generated by most fido2 devices) is documented here: https://w3c.github.io/webauthn/#sctn-packed-attestation

Since the attestation blob does *not* contain collected_client_data_hash it is not possible to validate the attestation provided by ssh-keygen.

In addition, the current design of the blob also may have some issues since the blob can only convey a singe x509 der for attestation cert, when some devices may require a certificate chain in their x5c element. The implementation of libfido2 only exposes the leaf certificate - not the chain. Also worth noting that ed25519 keys do not use the x5c element, but use a seperate field in the attestation statement for the conveyance of their CA. 

In addition the attestation blob lacks the algorithm fields to assist identification of the signature algorithm in use by the authenticator in the attestation. The needed data are all found in the attestation statement object.

As a result, it's likely that the current attestation blob format will need to change:

* Inclusion of the collected_client_data_hash
* replace fido_cred_authdata_ptr with fido_cred_authdata_raw_ptr to prevent a needless cbor array deserialise for attestation callers.
* replacement of attestation cert with the full attestation statement object obtained from fido_cred_attstmt_ptr
* addition of the "format" for the attestation statement type, in this case "packed" to account for future changes in attestation statements.

For libfido2 reference see: https://developers.yubico.com/libfido2/Manuals/fido_cred_attstmt_ptr.html

Without these changes I believe it is currently not possible to validate attestations for fido2 sk keys for ecdsa or ed25519.

Comment 1 Damien Miller 2023-01-05 10:10:51 AEDT

I don't think that's correct - these are not webauthn, but FIDO2 credentials (which is built on top of FIDO2 but adds additional encapsulation). I'm pretty sure that it's possible to verify ssh-keygen's attestation.

Adding Pedro as he knows way more about this than me.

Comment 2 William Brown 2023-01-05 11:10:42 AEDT

The webauthn attestation sections are reflections of their underlying standards in most cases. However, for FIDO2 the attestation format is defined in the Webauthn standard. For FIDO2, and more specifically CTAP2, this is discussed here:

https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-errata-20220621.html#op-makecred-step-rk

"""
Step 19: Generate an attestation statement for the newly-created credential using clientDataHash, taking into account the value of the enterpriseAttestation parameter, if present, as described above in Step 9.

...

attStmt (0x03) ... The attestation statement, as specified in [WebAuthn].
"""

Thus the document and structure I linked is the correct one. 

With this in mind, the lack of a clientDataHash in the attest output created by ssh-keygen means verification of an attestation is not possible as the FIDO2 device itself will be signing the concatenation of authenticatorData and clientDataHash.

Since this will likely constitute a change to the attest blob format that ssh-keygen produces, this is also why I made the other suggestions to altering the format as currently the format as it stands is not able to create or validate ECDAA attestation either.

Comment 3 pedro martelletto 2023-01-05 19:05:46 AEDT

The client data part of the attestation payload can be specified out-of-band through ssh-keygen -O challenge=, https://man.openbsd.org/ssh-keygen#challenge.

Regarding different attestation statement formats, intermediate or root certificates, and other data required to attest a credential: in most cases involving USB or NFC security keys, the format will be "packed" or "fido-u2f", and the root CA published by the vendor of the security key (e.g. https://developers.yubico.com/U2F/yubico-u2f-ca-certs.txt). What is current in place should be enough to satisfy that scenario.

Going forward, we might want to embed the attestation format and the entire attestation statement (fido_cred_fmt() and fido_cred_attstmt_ptr() respectively) in the attestation blob.

Comment 4 William Brown 2023-01-06 10:52:07 AEDT

> The client data part of the attestation payload can be specified out-of-band through ssh-keygen -O challenge=, https://man.openbsd.org/ssh-keygen#challenge.

This doesn't help when the challenge *isn't* specified though, meaning that if attestation is requested, challenge either has to be a mandatory argument and ssh-keygen should error or that the challenge needs to be added to the attestation format so that the attestation can be validated. 

> in most cases involving USB or NFC security keys, the format will be "packed" or "fido-u2f", and the root CA published by the vendor of the security key (e.g. https://developers.yubico.com/U2F/yubico-u2f-ca-certs.txt). What is current in place should be enough to satisfy that scenario.

In packed https://w3c.github.io/webauthn/#sctn-packed-attestation the specification states:

"""
x5c
The elements of this array contain attestnCert and its certificate chain (if any), each encoded in X.509 format. The attestation certificate attestnCert MUST be the first element in the array.
"""

However the current attest format that is created by ssh-keygen does not contain the full x5c chain, it only contains the attestation (leaf) certificate meaning that it is not always true that the vendors CA certificate is sufficient to validate the attestation since the chain may be missing. 


> Going forward, we might want to embed the attestation format and the entire attestation statement (fido_cred_fmt() and fido_cred_attstmt_ptr() respectively) in the attestation blob.

Yes this would resolve the issues I have raised with regard to x5c chains as well as the current inability to validate ecdaa attestations (which do not use x5c).

Comment 5 Damien Miller 2023-01-06 13:08:34 AEDT

> This doesn't help when the challenge *isn't* specified though,
> meaning that if attestation is requested

Attestation without a verifier-specified challenge is pretty worthless, as otherwise there is no guarantee of freshness, or conversely, it would allow replay of prior attestations.

Comment 6 William Brown 2023-01-06 15:50:58 AEDT

(In reply to Damien Miller from comment #5)
> > This doesn't help when the challenge *isn't* specified though,
> > meaning that if attestation is requested
> 
> Attestation without a verifier-specified challenge is pretty
> worthless, as otherwise there is no guarantee of freshness, or
> conversely, it would allow replay of prior attestations.

Then when attestation is requested, it should be an error to also not provide a challenge parameter.