upstream: improve the error message for u2f enrollment errors by

making ssh-keygen be solely responsible for printing the error message and
convertint some more common error responses from the middleware to a useful
ssherr.h status code. more detail remains visible via -v of course.

also remove indepedent copy of sk-api.h declarations in sk-usbhid.c
and just include it.

feedback & ok markus@

OpenBSD-Commit-ID: a4a8ffa870d9a3e0cfd76544bcdeef5c9fb1f1bb

diff --git a/PROTOCOL.u2f b/PROTOCOL.u2f
index fd0cd0de0abfa37288c95dc104716fd3ef7135e0..58f75ba285dd06ba0de658084538f89b3134afd1 100644 (file)
--- a/PROTOCOL.u2f
+++ b/PROTOCOL.u2f
@@ -249,6 +249,7 @@ The middleware library need only expose a handful of functions:
        #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;

diff --git a/sk-api.h b/sk-api.h
index 93d6a1229decdcf2d54fe647f56c42d918beb0fe..170fd4470b0fa6e40025ec9114b67e8f08d7d6ad 100644 (file)
--- a/sk-api.h
+++ b/sk-api.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: sk-api.h,v 1.7 2020/01/06 02:00:46 djm Exp $ */
+/* $OpenBSD: sk-api.h,v 1.8 2020/01/25 23:13:09 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -36,6 +36,7 @@
 #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;

diff --git a/sk-usbhid.c b/sk-usbhid.c
index cf783e205f69b14e59db83dffaa56a727bbed1c9..2148e1d79b284385c54f345b61169a173e3e929d 100644 (file)
--- a/sk-usbhid.c
+++ b/sk-usbhid.c
@@ -37,9 +37,19 @@
 #include <fido/credman.h>
 
 #ifndef SK_STANDALONE
-#include "log.h"
-#include "xmalloc.h"
-#endif
+# include "log.h"
+# include "xmalloc.h"
+/*
+ * If building as part of OpenSSH, then rename exported functions.
+ * This must be done before including sk-api.h.
+ */
+# define sk_api_version                ssh_sk_api_version
+# define sk_enroll             ssh_sk_enroll
+# define sk_sign               ssh_sk_sign
+# define sk_load_resident_keys ssh_sk_load_resident_keys
+#endif /* !SK_STANDALONE */
+
+#include "sk-api.h"
 
 /* #define SK_DEBUG 1 */
 
@@ -54,63 +64,6 @@
        } while (0)
 #endif
 
-#define SK_VERSION_MAJOR       0x00040000 /* current API version */
-
-/* Flags */
-#define SK_USER_PRESENCE_REQD          0x01
-#define SK_USER_VERIFICATION_REQD      0x04
-#define SK_RESIDENT_KEY                        0x20
-
-/* Algs */
-#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;
-       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 required;
-};
-
-/* If building as part of OpenSSH, then rename exported functions */
-#if !defined(SK_STANDALONE)
-#define sk_api_version         ssh_sk_api_version
-#define sk_enroll              ssh_sk_enroll
-#define sk_sign                        ssh_sk_sign
-#define sk_load_resident_keys  ssh_sk_load_resident_keys
-#endif
-
 /* Return the version of the middleware API */
 uint32_t sk_api_version(void);
 
@@ -161,7 +114,7 @@ skdebug(const char *func, const char *fmt, ...)
 uint32_t
 sk_api_version(void)
 {
-       return SK_VERSION_MAJOR;
+       return SSH_SK_VERSION_MAJOR;
 }
 
 /* Select the first identified FIDO device attached to the system */
@@ -426,10 +379,10 @@ pack_public_key(uint32_t alg, const fido_cred_t *cred,
 {
        switch(alg) {
 #ifdef WITH_OPENSSL
-       case SK_ECDSA:
+       case SSH_SK_ECDSA:
                return pack_public_key_ecdsa(cred, response);
 #endif /* WITH_OPENSSL */
-       case SK_ED25519:
+       case SSH_SK_ED25519:
                return pack_public_key_ed25519(cred, response);
        default:
                return -1;
@@ -441,6 +394,7 @@ fidoerr_to_skerr(int fidoerr)
 {
        switch (fidoerr) {
        case FIDO_ERR_UNSUPPORTED_OPTION:
+       case FIDO_ERR_UNSUPPORTED_ALGORITHM:
                return SSH_SK_ERR_UNSUPPORTED;
        case FIDO_ERR_PIN_REQUIRED:
        case FIDO_ERR_PIN_INVALID:
@@ -516,11 +470,11 @@ sk_enroll(uint32_t alg, const uint8_t *challenge, size_t challenge_len,
        *enroll_response = NULL;
        switch(alg) {
 #ifdef WITH_OPENSSL
-       case SK_ECDSA:
+       case SSH_SK_ECDSA:
                cose_alg = COSE_ES256;
                break;
 #endif /* WITH_OPENSSL */
-       case SK_ED25519:
+       case SSH_SK_ED25519:
                cose_alg = COSE_EDDSA;
                break;
        default:
@@ -528,6 +482,7 @@ sk_enroll(uint32_t alg, const uint8_t *challenge, size_t challenge_len,
                goto out;
        }
        if (device == NULL && (device = pick_first_device()) == NULL) {
+               ret = SSH_SK_ERR_DEVICE_NOT_FOUND;
                skdebug(__func__, "pick_first_device failed");
                goto out;
        }
@@ -546,7 +501,7 @@ sk_enroll(uint32_t alg, const uint8_t *challenge, size_t challenge_len,
                    fido_strerr(r));
                goto out;
        }
-       if ((r = fido_cred_set_rk(cred, (flags & SK_RESIDENT_KEY) != 0 ?
+       if ((r = fido_cred_set_rk(cred, (flags & SSH_SK_RESIDENT_KEY) != 0 ?
            FIDO_OPT_TRUE : FIDO_OPT_OMIT)) != FIDO_OK) {
                skdebug(__func__, "fido_cred_set_rk: %s", fido_strerr(r));
                goto out;
@@ -717,10 +672,10 @@ pack_sig(uint32_t  alg, fido_assert_t *assert,
 {
        switch(alg) {
 #ifdef WITH_OPENSSL
-       case SK_ECDSA:
+       case SSH_SK_ECDSA:
                return pack_sig_ecdsa(assert, response);
 #endif /* WITH_OPENSSL */
-       case SK_ED25519:
+       case SSH_SK_ED25519:
                return pack_sig_ed25519(assert, response);
        default:
                return -1;
@@ -804,7 +759,7 @@ sk_sign(uint32_t alg, const uint8_t *message, size_t message_len,
                goto out;
        }
        if ((r = fido_assert_set_up(assert,
-           (flags & SK_USER_PRESENCE_REQD) ?
+           (flags & SSH_SK_USER_PRESENCE_REQD) ?
            FIDO_OPT_TRUE : FIDO_OPT_FALSE)) != FIDO_OK) {
                skdebug(__func__, "fido_assert_set_up: %s", fido_strerr(r));
                goto out;
@@ -951,15 +906,15 @@ read_rks(const char *devpath, const char *pin,
 
                        switch (fido_cred_type(cred)) {
                        case COSE_ES256:
-                               srk->alg = SK_ECDSA;
+                               srk->alg = SSH_SK_ECDSA;
                                break;
                        case COSE_EDDSA:
-                               srk->alg = SK_ED25519;
+                               srk->alg = SSH_SK_ED25519;
                                break;
                        default:
                                skdebug(__func__, "unsupported key type %d",
                                    fido_cred_type(cred));
-                               goto out;
+                               goto out; /* XXX free rk and continue */
                        }
 
                        if ((r = pack_public_key(srk->alg, cred,

diff --git a/ssh-keygen.c b/ssh-keygen.c
index 29013a20f15a980e72aabcb21805d2e7b18fe26a..8df55f2c275d871c21bb6ea61d003f8df5e8cad5 100644 (file)
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.393 2020/01/25 23:02:13 djm Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.394 2020/01/25 23:13:09 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -3579,7 +3579,7 @@ main(int argc, char **argv)
                        if (r == 0)
                                break;
                        if (r != SSH_ERR_KEY_WRONG_PASSPHRASE)
-                               exit(1); /* error message already printed */
+                               fatal("Key enrollment failed: %s", ssh_err(r));
                        if (passphrase != NULL)
                                freezero(passphrase, strlen(passphrase));
                        passphrase = read_passphrase("Enter PIN for security "

diff --git a/ssh-sk-helper.c b/ssh-sk-helper.c
index a4be9d369638ba7252a30974568d02191b8ef70b..2f93ad716b920b27e4dbbfaa4195133711df6d29 100644 (file)
--- a/ssh-sk-helper.c
+++ b/ssh-sk-helper.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-sk-helper.c,v 1.8 2020/01/10 23:43:26 djm Exp $ */
+/* $OpenBSD: ssh-sk-helper.c,v 1.9 2020/01/25 23:13:09 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -63,7 +63,7 @@ reply_error(int r, char *fmt, ...)
        va_start(ap, fmt);
        xvasprintf(&msg, fmt, ap);
        va_end(ap);
-       error("%s: %s", __progname, msg);
+       debug("%s: %s", __progname, msg);
        free(msg);
 
        if (r >= 0)

diff --git a/ssh-sk.c b/ssh-sk.c
index 3f5eed62d22835f782ea213e76b80a0d608e67e6..a8d4de832b053923c229b69c0858c99142910332 100644 (file)
--- a/ssh-sk.c
+++ b/ssh-sk.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-sk.c,v 1.24 2020/01/06 02:00:47 djm Exp $ */
+/* $OpenBSD: ssh-sk.c,v 1.25 2020/01/25 23:13:09 djm Exp $ */
 /*
  * Copyright (c) 2019 Google LLC
  *
@@ -338,6 +338,8 @@ skerr_to_ssherr(int skerr)
                return SSH_ERR_FEATURE_UNSUPPORTED;
        case SSH_SK_ERR_PIN_REQUIRED:
                return SSH_ERR_KEY_WRONG_PASSPHRASE;
+       case SSH_SK_ERR_DEVICE_NOT_FOUND:
+               return SSH_ERR_DEVICE_NOT_FOUND;
        case SSH_SK_ERR_GENERAL:
        default:
                return SSH_ERR_INVALID_FORMAT;
@@ -490,7 +492,7 @@ sshsk_enroll(int type, const char *provider_path, const char *device,
        /* enroll key */
        if ((r = skp->sk_enroll(alg, challenge, challenge_len, application,
            flags, pin, opts, &resp)) != 0) {
-               error("Security key provider \"%s\" returned failure %d",
+               debug("%s: provider \"%s\" returned failure %d", __func__,
                    provider_path, r);
                r = skerr_to_ssherr(r);
                goto out;

diff --git a/ssherr.c b/ssherr.c
index 38974f51b8d0dd4ba7642e5d49d732ce02eb4a55..bd954aadd72934e61c7b8ba11f08e588e522be60 100644 (file)
--- a/ssherr.c
+++ b/ssherr.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ssherr.c,v 1.9 2019/12/30 09:24:45 djm Exp $  */
+/*     $OpenBSD: ssherr.c,v 1.10 2020/01/25 23:13:09 djm Exp $ */
 /*
  * Copyright (c) 2011 Damien Miller
  *
@@ -143,6 +143,8 @@ ssh_err(int n)
                return "signature algorithm not supported";
        case SSH_ERR_FEATURE_UNSUPPORTED:
                return "requested feature not supported";
+       case SSH_ERR_DEVICE_NOT_FOUND:
+               return "device not found";
        default:
                return "unknown error";
        }

diff --git a/ssherr.h b/ssherr.h
index 520bd496463c63a47e901d92f3a1038412e32c78..085e752744d8f16cca2aad87c88bd84e6ba69319 100644 (file)
--- a/ssherr.h
+++ b/ssherr.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ssherr.h,v 1.7 2019/12/30 09:24:45 djm Exp $  */
+/*     $OpenBSD: ssherr.h,v 1.8 2020/01/25 23:13:09 djm Exp $  */
 /*
  * Copyright (c) 2011 Damien Miller
  *
@@ -81,6 +81,7 @@
 #define SSH_ERR_NUMBER_TOO_LARGE               -57
 #define SSH_ERR_SIGN_ALG_UNSUPPORTED           -58
 #define SSH_ERR_FEATURE_UNSUPPORTED            -59
+#define SSH_ERR_DEVICE_NOT_FOUND               -60
 
 /* Translate a numeric error code to a human-readable error string */
 const char *ssh_err(int n);