upstream: 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@

OpenBSD-Commit-ID: 8439896e63792b2db99c6065dd9a45eabbdb7e0a

diff --git a/PROTOCOL.u2f b/PROTOCOL.u2f
index 5b8a06277c50c8ef4925724fb1035da4afb1f8f6..f8ca56b11c8cfc76adcedf813205e7d00171f913 100644 (file)
--- a/PROTOCOL.u2f
+++ b/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/sk-api.h b/sk-api.h
index cc32cd4cceb2714648429c0c905b8338160eaf26..df17ca54020c344a5f99a0bad90b3b8febd1b2af 100644 (file)
--- a/sk-api.h
+++ b/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
  *
@@ -47,6 +47,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 {
@@ -72,7 +74,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/sk-usbhid.c b/sk-usbhid.c
index de85b2cb3d9bac69139e018c70f7ecad740ff732..007c596447ff9e6bd746eaaa8cbcec0fd9f31eac 100644 (file)
--- a/sk-usbhid.c
+++ b/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
@@ -822,6 +822,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;
@@ -832,6 +842,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/ssh-keygen.1 b/ssh-keygen.1
index c51a0d9c8b57d6822d2a5ab093256cc7511964e5..3ae596caac82ae1c66f3af975928e4dbe4f7ab41 100644 (file)
--- a/ssh-keygen.1
+++ b/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/ssh-keygen.c b/ssh-keygen.c
index 64cee4de04906934ff6cac78015371014119b2f1..a12b79a5606dfa7af13cfc454feb2c900ec332c9 100644 (file)
--- a/ssh-keygen.c
+++ b/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
@@ -3071,6 +3071,27 @@ do_download_sk(const char *skprovider, const char *device)
        return ret;
 }
 
+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)
 {
@@ -3137,7 +3158,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;
@@ -3593,7 +3614,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);
@@ -3715,20 +3736,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/ssh-sk.c b/ssh-sk.c
index 89478aff005c2b5be9b222e5cc514e13c8165478..1455df635ff843c526ecd91ea52d12aa1b9d4b55 100644 (file)
--- a/ssh-sk.c
+++ b/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
  *
@@ -174,6 +174,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));
 }
 
@@ -419,6 +420,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,
@@ -506,19 +532,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 */