when writing an attestation blob for a FIDO key, record all the data

needed to verify the attestation. Previously we were missing the
"authenticator data" that is included in the signature.

spotted by Ian Haken
feedback Pedro Martelletto and Ian Haken; ok markus@

6 files changed, 96 insertions, 118 deletions

diff --git a/usr.bin/ssh/PROTOCOL.u2f b/usr.bin/ssh/PROTOCOL.u2f
index 5b8a06277c5..f8ca56b11c8 100644
--- a/usr.bin/ssh/PROTOCOL.u2f
+++ b/usr.bin/ssh/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		authenticator data (CBOR encoded)
+	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
@@ -267,87 +277,15 @@ regress testing. For this reason, OpenSSH shall support a dynamically-
 loaded middleware libraries to communicate with security keys, but offer
 support for the common case of USB HID security keys internally.
 
-The middleware library need only expose a handful of functions:
-
-	#define SSH_SK_VERSION_MAJOR		0x00050000 /* API version */
-	#define SSH_SK_VERSION_MAJOR_MASK	0xffff0000
-
-	/* Flags */
-	#define SSH_SK_USER_PRESENCE_REQD	0x01
-	#define SSH_SK_USER_VERIFICATION_REQD	0x04
-	#define SSH_SK_RESIDENT_KEY		0x20
-
-	/* Algs */
-	#define SSH_SK_ECDSA                   0x00
-	#define SSH_SK_ED25519                 0x01
-
-	/* Error codes */
-	#define SSH_SK_ERR_GENERAL		-1
-	#define SSH_SK_ERR_UNSUPPORTED		-2
-	#define SSH_SK_ERR_PIN_REQUIRED		-3
-	#define SSH_SK_ERR_DEVICE_NOT_FOUND	-4
-
-	struct sk_enroll_response {
-		uint8_t *public_key;
-		size_t public_key_len;
-		uint8_t *key_handle;
-		size_t key_handle_len;
-		uint8_t *signature;
-		size_t signature_len;
-		uint8_t *attestation_cert;
-		size_t attestation_cert_len;
-	};
-
-	struct sk_sign_response {
-		uint8_t flags;
-		uint32_t counter;
-		uint8_t *sig_r;
-		size_t sig_r_len;
-		uint8_t *sig_s;
-		size_t sig_s_len;
-	};
-
-	struct sk_resident_key {
-		uint32_t alg;
-		size_t slot;
-		char *application;
-		struct sk_enroll_response key;
-	};
-
-	struct sk_option {
-		char *name;
-		char *value;
-		uint8_t important;
-	};
-
-	/* Return the version of the middleware API */
-	uint32_t sk_api_version(void);
-
-	/* Enroll a U2F key (private key generation) */
-	int sk_enroll(uint32_t alg,
-	    const uint8_t *challenge, size_t challenge_len,
-	    const char *application, uint8_t flags, const char *pin,
-	    struct sk_option **options,
-	    struct sk_enroll_response **enroll_response);
-
-	/* Sign a challenge */
-	int sk_sign(uint32_t alg, const uint8_t *message, size_t message_len,
-	    const char *application,
-	    const uint8_t *key_handle, size_t key_handle_len,
-	    uint8_t flags, const char *pin, struct sk_option **options,
-	    struct sk_sign_response **sign_response);
-
-	/* Enumerate all resident keys */
-	int sk_load_resident_keys(const char *pin, struct sk_option **options,
-	    struct sk_resident_key ***rks, size_t *nrks);
-
-The SSH_SK_VERSION_MAJOR should be incremented for each incompatible
+The middleware library need only expose a handful of functions and
+numbers listed in sk-api.h. Included in the defined numbers is a
+SSH_SK_VERSION_MAJOR that should be incremented for each incompatible
 API change.
 
-The options may be used to pass miscellaneous options to the middleware
-as a NULL-terminated array of pointers to struct sk_option. The middleware
-may ignore unsupported or unknown options unless the "important" flag is
-set, in which case it should return failure if an unsupported option is
+miscellaneous options may be passed to the middleware as a NULL-
+terminated array of pointers to struct sk_option. The middleware may
+ignore unsupported or unknown options unless the "required" flag is set,
+in which case it should return failure if an unsupported option is
 requested.
 
 At present the following options names are supported:
@@ -368,4 +306,4 @@ In OpenSSH, the middleware will be invoked by using a similar mechanism to
 ssh-pkcs11-helper to provide address-space containment of the
 middleware from ssh-agent.
 
-$OpenBSD: PROTOCOL.u2f,v 1.25 2020/08/31 00:17:41 djm Exp $
+$OpenBSD: PROTOCOL.u2f,v 1.26 2020/09/09 03:08:01 djm Exp $
diff --git a/usr.bin/ssh/sk-api.h b/usr.bin/ssh/sk-api.h
index 6bf7c092875..40cdc834a04 100644
--- a/usr.bin/ssh/sk-api.h
+++ b/usr.bin/ssh/sk-api.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: sk-api.h,v 1.10 2020/08/27 01:08:19 djm Exp $ */
+/* $OpenBSD: sk-api.h,v 1.11 2020/09/09 03:08:01 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -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/usr.bin/ssh/sk-usbhid.c b/usr.bin/ssh/sk-usbhid.c
index 9dde5c10a26..878ce965111 100644
--- a/usr.bin/ssh/sk-usbhid.c
+++ b/usr.bin/ssh/sk-usbhid.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sk-usbhid.c,v 1.25 2020/08/31 00:17:41 djm Exp $ */
+/* $OpenBSD: sk-usbhid.c,v 1.26 2020/09/09 03:08:01 djm Exp $ */
 /*
  * Copyright (c) 2019 Markus Friedl
  * Copyright (c) 2020 Pedro Martelletto
@@ -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/usr.bin/ssh/ssh-keygen.1 b/usr.bin/ssh/ssh-keygen.1
index c51a0d9c8b5..3ae596caac8 100644
--- a/usr.bin/ssh/ssh-keygen.1
+++ b/usr.bin/ssh/ssh-keygen.1
@@ -1,4 +1,4 @@
-.\"	$OpenBSD: ssh-keygen.1,v 1.208 2020/08/27 06:15:22 jmc Exp $
+.\"	$OpenBSD: ssh-keygen.1,v 1.209 2020/09/09 03:08:01 djm Exp $
 .\"
 .\" Author: Tatu Ylonen <ylo@cs.hut.fi>
 .\" Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -35,7 +35,7 @@
 .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd $Mdocdate: August 27 2020 $
+.Dd $Mdocdate: September 9 2020 $
 .Dt SSH-KEYGEN 1
 .Os
 .Sh NAME
@@ -520,9 +520,10 @@ Not all FIDO tokens support this option.
 Currently PIN authentication is the only supported verification method,
 but other methods may be supported in the future.
 .It Cm write-attestation Ns = Ns Ar path
-May be used at key generation time to record the attestation certificate
+May be used at key generation time to record the attestation data
 returned from FIDO tokens during key generation.
-By default this information is discarded.
+Please note that this information is potentially sensitive.
+By default, this information is discarded.
 .El
 .Pp
 The
diff --git a/usr.bin/ssh/ssh-keygen.c b/usr.bin/ssh/ssh-keygen.c
index 0e169a2aaac..703a5fb2102 100644
--- a/usr.bin/ssh/ssh-keygen.c
+++ b/usr.bin/ssh/ssh-keygen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.419 2020/08/27 09:46:04 djm Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.420 2020/09/09 03:08:01 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -3050,6 +3050,27 @@ do_download_sk(const char *skprovider, const char *device)
 }
 
 static void
+save_attestation(struct sshbuf *attest, const char *path)
+{
+	mode_t omask;
+	int r;
+
+	if (path == NULL)
+		return; /* nothing to do */
+	if (attest == NULL || sshbuf_len(attest) == 0)
+		fatal("Enrollment did not return attestation data");
+	omask = umask(077);
+	r = sshbuf_write_file(path, attest);
+	umask(omask);
+	if (r != 0)
+		fatal("Unable to write attestation data \"%s\": %s", path,
+		    ssh_err(r));
+	if (!quiet)
+		printf("Your FIDO attestation certificate has been saved in "
+		    "%s\n", path);
+}
+
+static void
 usage(void)
 {
 	fprintf(stderr,
@@ -3115,7 +3136,7 @@ main(int argc, char **argv)
 	unsigned long long cert_serial = 0;
 	char *identity_comment = NULL, *ca_key_path = NULL, **opts = NULL;
 	char *sk_application = NULL, *sk_device = NULL, *sk_user = NULL;
-	char *sk_attestaion_path = NULL;
+	char *sk_attestation_path = NULL;
 	struct sshbuf *challenge = NULL, *attest = NULL;
 	size_t i, nopts = 0;
 	u_int32_t bits = 0;
@@ -3568,7 +3589,7 @@ main(int argc, char **argv)
 				}
 			} else if (strncasecmp(opts[i],
 			    "write-attestation=", 18) == 0) {
-				sk_attestaion_path = opts[i] + 18;
+				sk_attestation_path = opts[i] + 18;
 			} else if (strncasecmp(opts[i],
 			    "application=", 12) == 0) {
 				sk_application = xstrdup(opts[i] + 12);
@@ -3690,20 +3711,9 @@ main(int argc, char **argv)
 		free(fp);
 	}
 
-	if (sk_attestaion_path != NULL) {
-		if (attest == NULL || sshbuf_len(attest) == 0) {
-			fatal("Enrollment did not return attestation "
-			    "certificate");
-		}
-		if ((r = sshbuf_write_file(sk_attestaion_path, attest)) != 0) {
-			fatal("Unable to write attestation certificate "
-			    "\"%s\": %s", sk_attestaion_path, ssh_err(r));
-		}
-		if (!quiet) {
-			printf("Your FIDO attestation certificate has been "
-			    "saved in %s\n", sk_attestaion_path);
-		}
-	}
+	if (sk_attestation_path != NULL)
+		save_attestation(attest, sk_attestation_path);
+
 	sshbuf_free(attest);
 	sshkey_free(public);
 
diff --git a/usr.bin/ssh/ssh-sk.c b/usr.bin/ssh/ssh-sk.c
index 6a2422f2bba..1262bce4b97 100644
--- a/usr.bin/ssh/ssh-sk.c
+++ b/usr.bin/ssh/ssh-sk.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-sk.c,v 1.31 2020/08/27 01:08:19 djm Exp $ */
+/* $OpenBSD: ssh-sk.c,v 1.32 2020/09/09 03:08:02 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -164,6 +164,7 @@ sshsk_free_enroll_response(struct sk_enroll_response *r)
 	freezero(r->public_key, r->public_key_len);
 	freezero(r->signature, r->signature_len);
 	freezero(r->attestation_cert, r->attestation_cert_len);
+	freezero(r->authdata, r->authdata_len);
 	freezero(r, sizeof(*r));
 }
 
@@ -409,6 +410,31 @@ make_options(const char *device, const char *user_id,
 	return ret;
 }
 
+
+static int
+fill_attestation_blob(const 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 +522,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_blob(resp, attest)) != 0)
+		goto out;
+
 	/* success */
 	*keyp = key;
 	key = NULL; /* transferred */