Incomplete attestation data for FIDO2 SKs?

[Damien Miller]

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 */