openssh bugs - Jan 2023 - [Bug 3516] New: ssh-keygen when creating sk fido keys does not create sufficient data for attestation verification.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

            Bug ID: 3516
           Summary: ssh-keygen when creating sk fido keys does not create
                    sufficient data for attestation verification.
           Product: Portable OpenSSH
           Version: 9.1p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh-keygen
          Assignee: unassigned-bugs at mindrot.org
          Reporter: william.brown at suse.com

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org,
                   |                            |pedro at ambientworks.net

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

--- Comment #2 from William Brown <william.brown at suse.com> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

--- Comment #3 from pedro martelletto <pedro at ambientworks.net> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

--- Comment #4 from William Brown <william.brown at suse.com>
---
> 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).

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

--- Comment #5 from Damien Miller <djm at mindrot.org>
---
> 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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3516

--- Comment #6 from William Brown <william.brown at suse.com> ---
(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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.