openssh unix dev - Sep 2020 - Incomplete attestation data for FIDO2 SKs?

I was recently looking at verifying the attestation data
(ssh-sk-attest-v00) for a SK key, but I believe the data saved in this
structure is insufficient for completing verification of the attestation.
While the structure has enough information for U2F devices, FIDO2 devices
sign their attestation over a richer "authData" blob [1] (concatenated
with
the challenge hash). The authData blob contains data not derivable from the
public/private key, such as a signature counter and the device's AAGUID. As
I understand it, the attestation structure should probably persist the
entire authData blob to enable validation of the attestation. (This is
really only getting into support for verifying "packed" attestation
statements. Figuring out what to extract and persist is likely even more
nuanced for other formats, but I'm not terribly inclined to go there
myself.)

Is there something I'm missing that would enable verification of the
attestation signature for FIDO2 devices, or is this a correct assessment
that the ssh-sk-attest-v00 file saved from ssh-keygen would not be enough?

[1] https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#sctn-attestation

On Fri, 4 Sep 2020, Ian Haken wrote:
> I was recently looking at verifying the attestation data
> (ssh-sk-attest-v00) for a SK key, but I believe the data saved in this
> structure is insufficient for completing verification of the attestation.
> While the structure has enough information for U2F devices, FIDO2 devices
> sign their attestation over a richer "authData" blob [1]
(concatenated with
> the challenge hash). The authData blob contains data not derivable from the
> public/private key, such as a signature counter and the device's
AAGUID. As
> I understand it, the attestation structure should probably persist the
> entire authData blob to enable validation of the attestation. (This is
> really only getting into support for verifying "packed"
attestation
> statements. Figuring out what to extract and persist is likely even more
> nuanced for other formats, but I'm not terribly inclined to go there
> myself.)
> 
> Is there something I'm missing that would enable verification of the
> attestation signature for FIDO2 devices, or is this a correct assessment
> that the ssh-sk-attest-v00 file saved from ssh-keygen would not be enough?
> 
> [1] https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#sctn-attestation
I haven't tried to parse the attestation blob for FIDO2 devices,
so it's entirely possible that it doesn't work.

We rely on libfido2 to obtain the attestation cert from the token,
so if it has functions that let us get at the rest of what we need
then it wouldn't be too much work to append/prepend that extra
information to the blob.

Pedro, do you have any insight here?

-d

On Mon, 7 Sep 2020, pedro martelletto wrote:
> > I haven't tried to parse the attestation blob for FIDO2 devices,
> > so it's entirely possible that it doesn't work.
> > 
> > We rely on libfido2 to obtain the attestation cert from the token,
> > so if it has functions that let us get at the rest of what we need
> > then it wouldn't be too much work to append/prepend that extra
> > information to the blob.
> > 
> > Pedro, do you have any insight here?
> > 
> > -d
> 
> I agree with Ian -- we need to persist the contents of
> fido_cred_authdata_ptr(), or an external verifier won't be able to form
a
> complete picture of what was signed.
> 
> Alternatively, we could persist only the AAGUID and the signature counter,
and
> let the verifier reconstruct authdata. Since authdata isn't strictly
bounded
> (its last constituent part is a CBOR map of variable content and length),
> reconstructing it can get tricky.
Thanks for the pointers - I think the attached patch should do it.

-d


-------------- next part --------------
diff --git a/PROTOCOL.u2f b/PROTOCOL.u2f
index 5b8a062..f11494c 100644
--- a/PROTOCOL.u2f
+++ b/PROTOCOL.u2f
@@ -154,6 +154,16 @@ by trusted hardware before it will issue a certificate. To
support this
 case, OpenSSH optionally allows retaining the attestation information
 at the time of key generation. It will take the following format:
 
+	string		"ssh-sk-attest-v01"
+	string		attestation certificate
+	string		enrollment signature
+	string		authenicator data
+	uint32		reserved flags
+	string		reserved string
+
+A previous version of this format, emitted prior to OpenSSH 8.4 omitted
+the authenticator data.
+
 	string		"ssh-sk-attest-v00"
 	string		attestation certificate
 	string		enrollment signature
diff --git a/sk-api.h b/sk-api.h
index 6bf7c09..7d40679 100644
--- a/sk-api.h
+++ b/sk-api.h
@@ -45,6 +45,8 @@ struct sk_enroll_response {
 	size_t signature_len;
 	uint8_t *attestation_cert;
 	size_t attestation_cert_len;
+	uint8_t *authdata;
+	size_t authdata_len;
 };
 
 struct sk_sign_response {
@@ -70,7 +72,7 @@ struct sk_option {
 	uint8_t required;
 };
 
-#define SSH_SK_VERSION_MAJOR		0x00060000 /* current API version */
+#define SSH_SK_VERSION_MAJOR		0x00070000 /* current API version */
 #define SSH_SK_VERSION_MAJOR_MASK	0xffff0000
 
 /* Return the version of the middleware API */
diff --git a/sk-usbhid.c b/sk-usbhid.c
index 9dde5c1..e36930e 100644
--- a/sk-usbhid.c
+++ b/sk-usbhid.c
@@ -781,6 +781,16 @@ sk_enroll(uint32_t alg, const uint8_t *challenge, size_t
challenge_len,
 		memcpy(response->attestation_cert, ptr, len);
 		response->attestation_cert_len = len;
 	}
+	if ((ptr = fido_cred_authdata_ptr(cred)) != NULL) {
+		len = fido_cred_authdata_len(cred);
+		debug3("%s: authdata len=%zu", __func__, len);
+		if ((response->authdata = calloc(1, len)) == NULL) {
+			skdebug(__func__, "calloc authdata failed");
+			goto out;
+		}
+		memcpy(response->authdata, ptr, len);
+		response->authdata_len = len;
+	}
 	*enroll_response = response;
 	response = NULL;
 	ret = 0;
@@ -791,6 +801,7 @@ sk_enroll(uint32_t alg, const uint8_t *challenge, size_t
challenge_len,
 		free(response->key_handle);
 		free(response->signature);
 		free(response->attestation_cert);
+		free(response->authdata);
 		free(response);
 	}
 	sk_close(sk);
diff --git a/ssh-sk.c b/ssh-sk.c
index 6a2422f..cd1645a 100644
--- a/ssh-sk.c
+++ b/ssh-sk.c
@@ -409,6 +409,30 @@ make_options(const char *device, const char *user_id,
 	return ret;
 }
 
+
+static int
+fill_attestation_cert(struct sk_enroll_response *resp, struct sshbuf *attest)
+{
+	int r;
+
+	if (attest == NULL)
+		return 0; /* nothing to do */
+	if ((r = sshbuf_put_cstring(attest, "ssh-sk-attest-v01")) != 0 ||
+	    (r = sshbuf_put_string(attest,
+	    resp->attestation_cert, resp->attestation_cert_len)) != 0 ||
+	    (r = sshbuf_put_string(attest,
+	    resp->signature, resp->signature_len)) != 0 ||
+	    (r = sshbuf_put_string(attest,
+	    resp->authdata, resp->authdata_len)) != 0 ||
+	    (r = sshbuf_put_u32(attest, 0)) != 0 || /* resvd flags */
+	    (r = sshbuf_put_string(attest, NULL, 0)) != 0 /* resvd */) {
+		error("%s: buffer error: %s", __func__, ssh_err(r));
+		return r;
+	}
+	/* success */
+	return 0;
+}
+
 int
 sshsk_enroll(int type, const char *provider_path, const char *device,
     const char *application, const char *userid, uint8_t flags,
@@ -496,19 +520,9 @@ sshsk_enroll(int type, const char *provider_path, const
char *device,
 		goto out;
 
 	/* Optionally fill in the attestation information */
-	if (attest != NULL) {
-		if ((r = sshbuf_put_cstring(attest,
-		    "ssh-sk-attest-v00")) != 0 ||
-		    (r = sshbuf_put_string(attest,
-		    resp->attestation_cert, resp->attestation_cert_len)) != 0 ||
-		    (r = sshbuf_put_string(attest,
-		    resp->signature, resp->signature_len)) != 0 ||
-		    (r = sshbuf_put_u32(attest, 0)) != 0 || /* resvd flags */
-		    (r = sshbuf_put_string(attest, NULL, 0)) != 0 /* resvd */) {
-			error("%s: buffer error: %s", __func__, ssh_err(r));
-			goto out;
-		}
-	}
+	if ((r = fill_attestation_cert(resp, attest)) != 0)
+		goto out;
+
 	/* success */
 	*keyp = key;
 	key = NULL; /* transferred */

Thanks folks! This looks like it's exactly what I was looking for. As
I'm
pulling the thread on this, one word of warning on the
fido_cred_authdata_ptr method. The following is mentioned libfido2 docs
[1]: "The authenticator data returned by fido_cred_authdata_ptr() is a
CBOR-encoded byte string, as obtained from the authenticator." This is a
bit unfortunate since it's the CBOR-decoded data over which the attestation
signature is computed (concatenated with the challenge hash). And of course
you would also want to CBOR-decode the byte string before parsing the auth
data structure. I just opened a question [2] on the libfido2 GH page to ask
if there shouldn't be an API to return the CBOR-decoded data instead since
really that's what you would want for any uses of the function.

Basically, I think the openssh docs might also want to clarify that the
"ssh-sk-attest-v01" structure similarly has "authenticator
data" as a
CBOR-encoded byte array (since customers would need to decode it to verify
attestation), or else you may want to just CBOR-decode the output of
fido_cred_authdata_ptr in sk-usbhid.c, at least until libfido2 (hopefully)
follows up on my question and provides a convenience method for getting
that decoded value directly.

[1]
https://developers.yubico.com/libfido2/Manuals/fido_cred_authdata_ptr.html
[2] https://github.com/Yubico/libfido2/issues/212

On Mon, Sep 7, 2020 at 5:31 PM Damien Miller <djm at mindrot.org> wrote:
> On Mon, 7 Sep 2020, pedro martelletto wrote:
>
> > > I haven't tried to parse the attestation blob for FIDO2
devices,
> > > so it's entirely possible that it doesn't work.
> > >
> > > We rely on libfido2 to obtain the attestation cert from the
token,
> > > so if it has functions that let us get at the rest of what we
need
> > > then it wouldn't be too much work to append/prepend that
extra
> > > information to the blob.
> > >
> > > Pedro, do you have any insight here?
> > >
> > > -d
> >
> > I agree with Ian -- we need to persist the contents of
> > fido_cred_authdata_ptr(), or an external verifier won't be able to
form a
> > complete picture of what was signed.
> >
> > Alternatively, we could persist only the AAGUID and the signature
> counter, and
> > let the verifier reconstruct authdata. Since authdata isn't
strictly
> bounded
> > (its last constituent part is a CBOR map of variable content and
length),
> > reconstructing it can get tricky.
>
> Thanks for the pointers - I think the attached patch should do it.
>
> -d
>
>
>