From 43ce96427b76c4918e39af654e2fc9ee18d5d478 Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Mon, 30 Dec 2019 09:24:45 +0000 Subject: upstream: 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@ OpenBSD-Commit-ID: 9663c6a2bb7a0bc8deaccc6c30d9a2983b481620 --- sk-api.h | 7 ++++++- sk-usbhid.c | 49 ++++++++++++++++++++++++++++++++----------------- ssh-keygen.c | 32 +++++++++++++++++++++----------- ssh-sk.c | 21 ++++++++++++++++++--- ssherr.c | 4 +++- ssherr.h | 3 ++- 6 files changed, 82 insertions(+), 34 deletions(-) diff --git a/sk-api.h b/sk-api.h index 4f9f43ee6..dc786d556 100644 --- a/sk-api.h +++ b/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 * @@ -32,6 +32,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/sk-usbhid.c b/sk-usbhid.c index 54ce0bddf..22a4c5df5 100644 --- a/sk-usbhid.c +++ b/sk-usbhid.c @@ -65,6 +65,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; @@ -412,6 +417,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, @@ -424,7 +443,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; @@ -491,8 +510,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) { @@ -657,7 +677,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 @@ -736,7 +756,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; @@ -747,16 +767,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; } @@ -765,7 +784,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", @@ -776,7 +795,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; } @@ -801,7 +819,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; } @@ -831,7 +848,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; } @@ -862,7 +878,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; } @@ -872,7 +887,7 @@ read_rks(const char *devpath, const char *pin, } } /* Success */ - r = 0; + ret = 0; out: if (srk != NULL) { free(srk->application); @@ -885,14 +900,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; @@ -924,7 +939,7 @@ sk_load_resident_keys(const char *pin, } } /* success */ - r = 0; + ret = 0; *rksp = rks; *nrksp = nrks; rks = NULL; @@ -938,7 +953,7 @@ sk_load_resident_keys(const char *pin, } free(rks); fido_dev_info_free(&devlist, MAX_FIDO_DEVICES); - return r; + return ret; } #endif /* ENABLE_SK_INTERNAL */ diff --git a/ssh-keygen.c b/ssh-keygen.c index 79e2e92b5..696891e0e 100644 --- a/ssh-keygen.c +++ b/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 * Copyright (c) 1994 Tatu Ylonen , Espoo, Finland @@ -3361,16 +3361,26 @@ 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"); - } - 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 */ - break; + 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); + } + if (i > 3) + fatal("Too many incorrect PINs"); + break; default: if ((r = sshkey_generate(type, bits, &private)) != 0) fatal("sshkey_generate failed"); diff --git a/ssh-sk.c b/ssh-sk.c index e1fb72cfc..b1d0d6c58 100644 --- a/ssh-sk.c +++ b/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 * @@ -325,6 +325,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, @@ -396,7 +410,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; } @@ -559,6 +573,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 */ @@ -655,7 +670,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/ssherr.c b/ssherr.c index 8ad3d5750..38974f51b 100644 --- a/ssherr.c +++ b/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/ssherr.h b/ssherr.h index 348da5a20..520bd4964 100644 --- a/ssherr.h +++ b/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); -- cgit v1.2.3