translate and return error codes; retry on bad PIN

Define some well-known error codes in the SK API and pass
them back via ssh-sk-helper.

Use the new "wrong PIN" error code to retry PIN prompting during
ssh-keygen of resident keys.

feedback and ok markus@

6 files changed, 80 insertions, 32 deletions

diff --git a/usr.bin/ssh/sk-api.h b/usr.bin/ssh/sk-api.h
index cff07d264a1..825cb275990 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.5 2019/12/30 09:23:28 djm Exp $ */
+/* $OpenBSD: sk-api.h,v 1.6 2019/12/30 09:24:45 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -30,6 +30,11 @@
 #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
+
 struct sk_enroll_response {
 	uint8_t *public_key;
 	size_t public_key_len;
diff --git a/usr.bin/ssh/sk-usbhid.c b/usr.bin/ssh/sk-usbhid.c
index 694c4135d1a..878c9980c7f 100644
--- a/usr.bin/ssh/sk-usbhid.c
+++ b/usr.bin/ssh/sk-usbhid.c
@@ -61,6 +61,11 @@
 #define	SK_ECDSA		0x00
 #define	SK_ED25519		0x01
 
+/* Error codes */
+#define SSH_SK_ERR_GENERAL		-1
+#define SSH_SK_ERR_UNSUPPORTED		-2
+#define SSH_SK_ERR_PIN_REQUIRED		-3
+
 struct sk_enroll_response {
 	uint8_t *public_key;
 	size_t public_key_len;
@@ -408,6 +413,20 @@ pack_public_key(int alg, const fido_cred_t *cred,
 	}
 }
 
+static int
+fidoerr_to_skerr(int fidoerr)
+{
+	switch (fidoerr) {
+	case FIDO_ERR_UNSUPPORTED_OPTION:
+		return SSH_SK_ERR_UNSUPPORTED;
+	case FIDO_ERR_PIN_REQUIRED:
+	case FIDO_ERR_PIN_INVALID:
+		return SSH_SK_ERR_PIN_REQUIRED;
+	default:
+		return -1;
+	}
+}
+
 int
 sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
     const char *application, uint8_t flags, const char *pin,
@@ -420,7 +439,7 @@ sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
 	struct sk_enroll_response *response = NULL;
 	size_t len;
 	int cose_alg;
-	int ret = -1;
+	int ret = SSH_SK_ERR_GENERAL;
 	int r;
 	char *device = NULL;
 
@@ -487,8 +506,9 @@ sk_enroll(int alg, const uint8_t *challenge, size_t challenge_len,
 		skdebug(__func__, "fido_dev_open: %s", fido_strerr(r));
 		goto out;
 	}
-	if ((r = fido_dev_make_cred(dev, cred, NULL)) != FIDO_OK) {
+	if ((r = fido_dev_make_cred(dev, cred, pin)) != FIDO_OK) {
 		skdebug(__func__, "fido_dev_make_cred: %s", fido_strerr(r));
+		ret = fidoerr_to_skerr(r);
 		goto out;
 	}
 	if (fido_cred_x5c_ptr(cred) != NULL) {
@@ -653,7 +673,7 @@ sk_sign(int alg, const uint8_t *message, size_t message_len,
 	fido_assert_t *assert = NULL;
 	fido_dev_t *dev = NULL;
 	struct sk_sign_response *response = NULL;
-	int ret = -1;
+	int ret = SSH_SK_ERR_GENERAL;
 	int r;
 
 #ifdef SK_DEBUG
@@ -732,7 +752,7 @@ static int
 read_rks(const char *devpath, const char *pin,
     struct sk_resident_key ***rksp, size_t *nrksp)
 {
-	int r = -1;
+	int ret = SSH_SK_ERR_GENERAL, r = -1;
 	fido_dev_t *dev = NULL;
 	fido_credman_metadata_t *metadata = NULL;
 	fido_credman_rp_t *rp = NULL;
@@ -743,16 +763,15 @@ read_rks(const char *devpath, const char *pin,
 
 	if ((dev = fido_dev_new()) == NULL) {
 		skdebug(__func__, "fido_dev_new failed");
-		return -1;
+		return ret;
 	}
 	if ((r = fido_dev_open(dev, devpath)) != FIDO_OK) {
 		skdebug(__func__, "fido_dev_open %s failed: %s",
 		    devpath, fido_strerr(r));
 		fido_dev_free(&dev);
-		return -1;
+		return ret;
 	}
 	if ((metadata = fido_credman_metadata_new()) == NULL) {
-		r = -1;
 		skdebug(__func__, "alloc failed");
 		goto out;
 	}
@@ -761,7 +780,7 @@ read_rks(const char *devpath, const char *pin,
 		if (r == FIDO_ERR_INVALID_COMMAND) {
 			skdebug(__func__, "device %s does not support "
 			    "resident keys", devpath);
-			r = 0;
+			ret = 0;
 			goto out;
 		}
 		skdebug(__func__, "get metadata for %s failed: %s",
@@ -772,7 +791,6 @@ read_rks(const char *devpath, const char *pin,
 	    (unsigned long long)fido_credman_rk_existing(metadata),
 	    (unsigned long long)fido_credman_rk_remaining(metadata));
 	if ((rp = fido_credman_rp_new()) == NULL) {
-		r = -1;
 		skdebug(__func__, "alloc rp failed");
 		goto out;
 	}
@@ -797,7 +815,6 @@ read_rks(const char *devpath, const char *pin,
 
 		fido_credman_rk_free(&rk);
 		if ((rk = fido_credman_rk_new()) == NULL) {
-			r = -1;
 			skdebug(__func__, "alloc rk failed");
 			goto out;
 		}
@@ -827,7 +844,6 @@ read_rks(const char *devpath, const char *pin,
 			    fido_cred_id_len(cred))) == NULL ||
 			    (srk->application = strdup(fido_credman_rp_id(rp,
 			    i))) == NULL) {
-				r = -1;
 				skdebug(__func__, "alloc sk_resident_key");
 				goto out;
 			}
@@ -858,7 +874,6 @@ read_rks(const char *devpath, const char *pin,
 			/* append */
 			if ((tmp = recallocarray(*rksp, *nrksp, (*nrksp) + 1,
 			    sizeof(**rksp))) == NULL) {
-				r = -1;
 				skdebug(__func__, "alloc rksp");
 				goto out;
 			}
@@ -868,7 +883,7 @@ read_rks(const char *devpath, const char *pin,
 		}
 	}
 	/* Success */
-	r = 0;
+	ret = 0;
  out:
 	if (srk != NULL) {
 		free(srk->application);
@@ -881,14 +896,14 @@ read_rks(const char *devpath, const char *pin,
 	fido_dev_close(dev);
 	fido_dev_free(&dev);
 	fido_credman_metadata_free(&metadata);
-	return r;
+	return ret;
 }
 
 int
 sk_load_resident_keys(const char *pin,
     struct sk_resident_key ***rksp, size_t *nrksp)
 {
-	int r = -1;
+	int ret = SSH_SK_ERR_GENERAL, r = -1;
 	fido_dev_info_t *devlist = NULL;
 	size_t i, ndev = 0, nrks = 0;
 	const fido_dev_info_t *di;
@@ -920,7 +935,7 @@ sk_load_resident_keys(const char *pin,
 		}
 	}
 	/* success */
-	r = 0;
+	ret = 0;
 	*rksp = rks;
 	*nrksp = nrks;
 	rks = NULL;
@@ -934,6 +949,6 @@ sk_load_resident_keys(const char *pin,
 	}
 	free(rks);
 	fido_dev_info_free(&devlist, MAX_FIDO_DEVICES);
-	return r;
+	return ret;
 }
 
diff --git a/usr.bin/ssh/ssh-keygen.c b/usr.bin/ssh/ssh-keygen.c
index 9c49eb81490..f4b87df2d4f 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.378 2019/12/30 09:23:28 djm Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.379 2019/12/30 09:24:45 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -3340,15 +3340,25 @@ main(int argc, char **argv)
 	switch (type) {
 	case KEY_ECDSA_SK:
 	case KEY_ED25519_SK:
-		if (!quiet) {
-			printf("You may need to touch your security key "
-			    "to authorize key generation.\n");
+		passphrase1 = NULL;
+		for (i = 0 ; i < 3; i++) {
+			if (!quiet) {
+				printf("You may need to touch your security "
+				    "key to authorize key generation.\n");
+			}
+			fflush(stdout);
+			r = sshsk_enroll(type, sk_provider,
+			    cert_key_id == NULL ? "ssh:" : cert_key_id,
+			    sk_flags, passphrase1, NULL, &private, NULL);
+			if (r == 0)
+				break;
+			if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
+				exit(1); /* error message already printed */
+			passphrase1 = read_passphrase("Enter PIN for security "
+			    "key: ", RP_ALLOW_STDIN);
 		}
-		fflush(stdout);
-		if (sshsk_enroll(type, sk_provider,
-		    cert_key_id == NULL ? "ssh:" : cert_key_id,
-		    sk_flags, NULL, NULL, &private, NULL) != 0)
-			exit(1); /* error message already printed */
+		if (i > 3)
+			fatal("Too many incorrect PINs");
 		break;
 	default:
 		if ((r = sshkey_generate(type, bits, &private)) != 0)
diff --git a/usr.bin/ssh/ssh-sk.c b/usr.bin/ssh/ssh-sk.c
index 8d66d641a75..275a6919dc2 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.22 2019/12/30 09:24:03 djm Exp $ */
+/* $OpenBSD: ssh-sk.c,v 1.23 2019/12/30 09:24:45 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -317,6 +317,20 @@ sshsk_key_from_response(int alg, const char *application, uint8_t flags,
 	return r;
 }
 
+static int
+skerr_to_ssherr(int skerr)
+{
+	switch (skerr) {
+	case SSH_SK_ERR_UNSUPPORTED:
+		return SSH_ERR_FEATURE_UNSUPPORTED;
+	case SSH_SK_ERR_PIN_REQUIRED:
+		return SSH_ERR_KEY_WRONG_PASSPHRASE;
+	case SSH_SK_ERR_GENERAL:
+	default:
+		return SSH_ERR_INVALID_FORMAT;
+	}
+}
+
 int
 sshsk_enroll(int type, const char *provider_path, const char *application,
     uint8_t flags, const char *pin, struct sshbuf *challenge_buf,
@@ -388,7 +402,7 @@ sshsk_enroll(int type, const char *provider_path, const char *application,
 	    flags, pin, &resp)) != 0) {
 		error("Security key provider \"%s\" returned failure %d",
 		    provider_path, r);
-		r = SSH_ERR_INVALID_FORMAT; /* XXX error codes in API? */
+		r = skerr_to_ssherr(r);
 		goto out;
 	}
 
@@ -551,6 +565,7 @@ sshsk_sign(const char *provider_path, struct sshkey *key,
 	    sshbuf_ptr(key->sk_key_handle), sshbuf_len(key->sk_key_handle),
 	    key->sk_flags, pin, &resp)) != 0) {
 		debug("%s: sk_sign failed with code %d", __func__, r);
+		r = skerr_to_ssherr(r);
 		goto out;
 	}
 	/* Assemble signature */
@@ -647,7 +662,7 @@ sshsk_load_resident(const char *provider_path, const char *pin,
 	if ((r = skp->sk_load_resident_keys(pin, &rks, &nrks)) != 0) {
 		error("Security key provider \"%s\" returned failure %d",
 		    provider_path, r);
-		r = SSH_ERR_INVALID_FORMAT; /* XXX error codes in API? */
+		r = skerr_to_ssherr(r);
 		goto out;
 	}
 	for (i = 0; i < nrks; i++) {
diff --git a/usr.bin/ssh/ssherr.c b/usr.bin/ssh/ssherr.c
index 8ad3d575019..38974f51b8d 100644
--- a/usr.bin/ssh/ssherr.c
+++ b/usr.bin/ssh/ssherr.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ssherr.c,v 1.8 2018/07/03 11:39:54 djm Exp $	*/
+/*	$OpenBSD: ssherr.c,v 1.9 2019/12/30 09:24:45 djm Exp $	*/
 /*
  * Copyright (c) 2011 Damien Miller
  *
@@ -141,6 +141,8 @@ ssh_err(int n)
 		return "number is too large";
 	case SSH_ERR_SIGN_ALG_UNSUPPORTED:
 		return "signature algorithm not supported";
+	case SSH_ERR_FEATURE_UNSUPPORTED:
+		return "requested feature not supported";
 	default:
 		return "unknown error";
 	}
diff --git a/usr.bin/ssh/ssherr.h b/usr.bin/ssh/ssherr.h
index 348da5a2086..520bd496463 100644
--- a/usr.bin/ssh/ssherr.h
+++ b/usr.bin/ssh/ssherr.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ssherr.h,v 1.6 2018/07/03 11:39:54 djm Exp $	*/
+/*	$OpenBSD: ssherr.h,v 1.7 2019/12/30 09:24:45 djm Exp $	*/
 /*
  * Copyright (c) 2011 Damien Miller
  *
@@ -80,6 +80,7 @@
 #define SSH_ERR_KEY_LENGTH			-56
 #define SSH_ERR_NUMBER_TOO_LARGE		-57
 #define SSH_ERR_SIGN_ALG_UNSUPPORTED		-58
+#define SSH_ERR_FEATURE_UNSUPPORTED		-59
 
 /* Translate a numeric error code to a human-readable error string */
 const char *ssh_err(int n);