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

diff --git a/sk-api.h b/sk-api.h
index 4f9f43ee6dacccc7d538c256a2b62abfc74d3d5b..dc786d556d880ac2f6d035575150a5c665b86a0a 100644 (file)
--- 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 54ce0bddfd0762d57f6156aaebd619a1b8009e1d..22a4c5df5f3131cec0f7a482660afd62449a8cfa 100644 (file)
--- 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 79e2e92b51486836696eaeb3caf65f83a56ebb46..696891e0e76f7920f709e3bcd094a06d400ea868 100644 (file)
--- 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 <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, 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 e1fb72cfcaf67d3043140d6b950b491e83a0d926..b1d0d6c585f6bbff4a7bd83026dfc302ecc92a3e 100644 (file)
--- 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 8ad3d575019094e26bdcccaab26babb2b86f676e..38974f51b8d0dd4ba7642e5d49d732ce02eb4a55 100644 (file)
--- 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 348da5a208650732735583538d8cdb265ad943ba..520bd496463c63a47e901d92f3a1038412e32c78 100644 (file)
--- 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);