upstream: Improve strictness and control over RSA-SHA2 signature

In ssh, when an agent fails to return a RSA-SHA2 signature when
requested and falls back to RSA-SHA1 instead, retry the signature to
ensure that the public key algorithm sent in the SSH_MSG_USERAUTH
matches the one in the signature itself.

In sshd, strictly enforce that the public key algorithm sent in the
SSH_MSG_USERAUTH message matches what appears in the signature.

Make the sshd_config PubkeyAcceptedKeyTypes and
HostbasedAcceptedKeyTypes options control accepted signature algorithms
(previously they selected supported key types). This allows these
options to ban RSA-SHA1 in favour of RSA-SHA2.

Add new signature algorithms "rsa-sha2-256-cert-v01@openssh.com" and
"rsa-sha2-512-cert-v01@openssh.com" to force use of RSA-SHA2 signatures
with certificate keys.

feedback and ok markus@

OpenBSD-Commit-ID: c6e9f6d45eed8962ad502d315d7eaef32c419dde

1 files changed, 216 insertions, 129 deletions

diff --git a/sshconnect2.c b/sshconnect2.c
index d8ae6eb3a..920376408 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sshconnect2.c,v 1.271 2018/06/26 02:02:36 djm Exp $ */
+/* $OpenBSD: sshconnect2.c,v 1.272 2018/07/03 11:39:54 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  * Copyright (c) 2008 Damien Miller.  All rights reserved.
@@ -315,7 +315,7 @@ int	input_gssapi_errtok(int, u_int32_t, struct ssh *);
 
 void    userauth(Authctxt *, char *);
 
-static int sign_and_send_pubkey(Authctxt *, Identity *);
+static int sign_and_send_pubkey(struct ssh *ssh, Authctxt *, Identity *);
 static void pubkey_prepare(Authctxt *);
 static void pubkey_cleanup(Authctxt *);
 static void pubkey_reset(Authctxt *);
@@ -619,7 +619,7 @@ input_userauth_pk_ok(int type, u_int32_t seq, struct ssh *ssh)
          */
         TAILQ_FOREACH_REVERSE(id, &authctxt->keys, idlist, next) {
                 if (key_equal(key, id->key)) {
-                        sent = sign_and_send_pubkey(authctxt, id);
+                        sent = sign_and_send_pubkey(ssh, authctxt, id);
                         break;
                 }
         }
@@ -986,73 +986,80 @@ input_userauth_passwd_changereq(int type, u_int32_t seqnr, struct ssh *ssh)
         return 0;
 }
 
-static const char *
-key_sign_encode(const struct sshkey *key)
-{
-        struct ssh *ssh = active_state;
-
-        if (key->type == KEY_RSA) {
-                switch (ssh->kex->rsa_sha2) {
-                case 256:
-                        return "rsa-sha2-256";
-                case 512:
-                        return "rsa-sha2-512";
-                }
-        }
-        return key_ssh_name(key);
-}
-
 /*
- * Some agents will return ssh-rsa signatures when asked to make a
- * rsa-sha2-* signature. Check what they actually gave back and warn the
- * user if the agent has returned an unexpected type.
+ * Select an algorithm for publickey signatures.
+ * Returns algorithm (caller must free) or NULL if no mutual algorithm found.
+ *
+ * Call with ssh==NULL to ignore server-sig-algs extension list and
+ * only attempt with the key's base signature type.
  */
-static int
-check_sigtype(const struct sshkey *key, const u_char *sig, size_t len)
+static char *
+key_sig_algorithm(struct ssh *ssh, const struct sshkey *key)
 {
-        int r;
-        char *sigtype = NULL;
-        const char *alg = key_sign_encode(key);
+        char *allowed, *oallowed, *cp, *alg = NULL;
 
-        if (sshkey_is_cert(key))
-                return 0;
-        if ((r = sshkey_sigtype(sig, len, &sigtype)) != 0)
-                return r;
-        if (strcmp(sigtype, alg) != 0) {
-                logit("warning: agent returned different signature type %s "
-                    "(expected %s)", sigtype, alg);
-        }
-        free(sigtype);
-        /* Incorrect signature types aren't an error ... yet */
-        return 0;
+        /*
+         * The signature algorithm will only differ from the key algorithm
+         * for RSA keys/certs and when the server advertises support for
+         * newer (SHA2) algorithms.
+         */
+        if (ssh == NULL || ssh->kex->server_sig_algs == NULL ||
+            (key->type != KEY_RSA && key->type != KEY_RSA_CERT)) {
+                /* Filter base key signature alg against our configuration */
+                return match_list(key_ssh_name(key),
+                    options.pubkey_key_types, NULL);
+        }
+
+        /*
+         * For RSA keys/certs, since these might have a different sig type:
+         * find the first entry in PubkeyAcceptedKeyTypes of the right type
+         * that also appears in the supported signature algorithms list from
+         * the server.
+         */
+        oallowed = allowed = xstrdup(options.pubkey_key_types);
+        while ((cp = strsep(&allowed, ",")) != NULL) {
+                if (sshkey_type_from_name(cp) != key->type)
+                        continue;
+                alg = match_list(cp, ssh->kex->server_sig_algs, NULL);
+                if (alg != NULL)
+                        break;
+        }
+        free(oallowed);
+        return alg;
 }
 
 static int
 identity_sign(struct identity *id, u_char **sigp, size_t *lenp,
-    const u_char *data, size_t datalen, u_int compat)
+    const u_char *data, size_t datalen, u_int compat, const char *alg)
 {
         struct sshkey *prv;
         int r;
 
-        /* the agent supports this key */
+        /* The agent supports this key. */
         if (id->key != NULL && id->agent_fd != -1) {
-                if ((r = ssh_agent_sign(id->agent_fd, id->key, sigp, lenp,
-                    data, datalen, key_sign_encode(id->key), compat)) != 0 ||
-                    (r = check_sigtype(id->key, *sigp, *lenp)) != 0)
-                        return r;
-                return 0;
+                return ssh_agent_sign(id->agent_fd, id->key, sigp, lenp,
+                    data, datalen, alg, compat);
         }
 
         /*
-         * we have already loaded the private key or
-         * the private key is stored in external hardware
+         * We have already loaded the private key or the private key is
+         * stored in external hardware.
          */
         if (id->key != NULL &&
-            (id->isprivate || (id->key->flags & SSHKEY_FLAG_EXT)))
-                return (sshkey_sign(id->key, sigp, lenp, data, datalen,
-                    key_sign_encode(id->key), compat));
+            (id->isprivate || (id->key->flags & SSHKEY_FLAG_EXT))) {
+                if ((r = sshkey_sign(id->key, sigp, lenp, data, datalen,
+                    alg, compat)) != 0)
+                        return r;
+                /*
+                 * PKCS#11 tokens may not support all signature algorithms,
+                 * so check what we get back.
+                 */
+                if ((r = sshkey_check_sigtype(*sigp, *lenp, alg)) != 0)
+                        return r;
+                return 0;
+        }
 
-        /* load the private key from the file */
+        /* Load the private key from the file. */
         if ((prv = load_identity_file(id)) == NULL)
                 return SSH_ERR_KEY_NOT_FOUND;
         if (id->key != NULL && !sshkey_equal_public(prv, id->key)) {
@@ -1060,8 +1067,7 @@ identity_sign(struct identity *id, u_char **sigp, size_t *lenp,
                    __func__, id->filename);
                 return SSH_ERR_KEY_NOT_FOUND;
         }
-        r = sshkey_sign(prv, sigp, lenp, data, datalen,
-            key_sign_encode(prv), compat);
+        r = sshkey_sign(prv, sigp, lenp, data, datalen, alg, compat);
         sshkey_free(prv);
         return r;
 }
@@ -1086,57 +1092,35 @@ id_filename_matches(Identity *id, Identity *private_id)
 }
 
 static int
-sign_and_send_pubkey(Authctxt *authctxt, Identity *id)
+sign_and_send_pubkey(struct ssh *ssh, Authctxt *authctxt, Identity *id)
 {
-        Buffer b;
-        Identity *private_id;
-        u_char *blob, *signature;
-        size_t slen;
-        u_int bloblen, skip = 0;
-        int matched, ret = -1, have_sig = 1;
-        char *fp;
+        struct sshbuf *b = NULL;
+        Identity *private_id, *sign_id = NULL;
+        u_char *signature = NULL;
+        size_t slen = 0, skip = 0;
+        int r, fallback_sigtype, sent = 0;
+        char *alg = NULL, *fp = NULL;
+        const char *loc = "";
 
         if ((fp = sshkey_fingerprint(id->key, options.fingerprint_hash,
             SSH_FP_DEFAULT)) == NULL)
                 return 0;
-        debug3("%s: %s %s", __func__, key_type(id->key), fp);
-        free(fp);
 
-        if (key_to_blob(id->key, &blob, &bloblen) == 0) {
-                /* we cannot handle this key */
-                debug3("sign_and_send_pubkey: cannot handle key");
-                return 0;
-        }
-        /* data to be signed */
-        buffer_init(&b);
-        if (datafellows & SSH_OLD_SESSIONID) {
-                buffer_append(&b, session_id2, session_id2_len);
-                skip = session_id2_len;
-        } else {
-                buffer_put_string(&b, session_id2, session_id2_len);
-                skip = buffer_len(&b);
-        }
-        buffer_put_char(&b, SSH2_MSG_USERAUTH_REQUEST);
-        buffer_put_cstring(&b, authctxt->server_user);
-        buffer_put_cstring(&b, authctxt->service);
-        buffer_put_cstring(&b, authctxt->method->name);
-        buffer_put_char(&b, have_sig);
-        buffer_put_cstring(&b, key_sign_encode(id->key));
-        buffer_put_string(&b, blob, bloblen);
+        debug3("%s: %s %s", __func__, sshkey_type(id->key), fp);
 
         /*
          * If the key is an certificate, try to find a matching private key
          * and use it to complete the signature.
          * If no such private key exists, fall back to trying the certificate
          * key itself in case it has a private half already loaded.
+         * This will try to set sign_id to the private key that will perform
+         * the signature.
          */
-        if (key_is_cert(id->key)) {
-                matched = 0;
+        if (sshkey_is_cert(id->key)) {
                 TAILQ_FOREACH(private_id, &authctxt->keys, next) {
                         if (sshkey_equal_public(id->key, private_id->key) &&
                             id->key->type != private_id->key->type) {
-                                id = private_id;
-                                matched = 1;
+                                sign_id = private_id;
                                 break;
                         }
                 }
@@ -1147,18 +1131,18 @@ sign_and_send_pubkey(Authctxt *authctxt, Identity *id)
                  * of keeping just a private key file and public
                  * certificate on disk.
                  */
-                if (!matched && !id->isprivate && id->agent_fd == -1 &&
+                if (sign_id == NULL &&
+                    !id->isprivate && id->agent_fd == -1 &&
                     (id->key->flags & SSHKEY_FLAG_EXT) == 0) {
                         TAILQ_FOREACH(private_id, &authctxt->keys, next) {
                                 if (private_id->key == NULL &&
                                     id_filename_matches(id, private_id)) {
-                                        id = private_id;
-                                        matched = 1;
+                                        sign_id = private_id;
                                         break;
                                 }
                         }
                 }
-                if (matched) {
+                if (sign_id != NULL) {
                         debug2("%s: using private key \"%s\"%s for "
                             "certificate", __func__, id->filename,
                             id->agent_fd != -1 ? " from agent" : "");
@@ -1168,51 +1152,121 @@ sign_and_send_pubkey(Authctxt *authctxt, Identity *id)
                 }
         }
 
-        /* generate signature */
-        ret = identity_sign(id, &signature, &slen,
-            buffer_ptr(&b), buffer_len(&b), datafellows);
-        if (ret != 0) {
-                if (ret != SSH_ERR_KEY_NOT_FOUND)
-                        error("%s: signing failed: %s", __func__, ssh_err(ret));
-                free(blob);
-                buffer_free(&b);
-                return 0;
+        /*
+         * If the above didn't select another identity to do the signing
+         * then default to the one we started with.
+         */
+        if (sign_id == NULL)
+                sign_id = id;
+
+        /* assemble and sign data */
+        for (fallback_sigtype = 0; fallback_sigtype <= 1; fallback_sigtype++) {
+                free(alg);
+                slen = 0;
+                signature = NULL;
+                if ((alg = key_sig_algorithm(fallback_sigtype ? NULL : ssh,
+                    id->key)) == NULL) {
+                        error("%s: no mutual signature supported", __func__);
+                        goto out;
+                }
+                debug3("%s: signing using %s", __func__, alg);
+
+                sshbuf_free(b);
+                if ((b = sshbuf_new()) == NULL)
+                        fatal("%s: sshbuf_new failed", __func__);
+                if (datafellows & SSH_OLD_SESSIONID) {
+                        if ((r = sshbuf_put(b, session_id2,
+                            session_id2_len)) != 0) {
+                                fatal("%s: sshbuf_put: %s",
+                                    __func__, ssh_err(r));
+                        }
+                } else {
+                        if ((r = sshbuf_put_string(b, session_id2,
+                            session_id2_len)) != 0) {
+                                fatal("%s: sshbuf_put_string: %s",
+                                    __func__, ssh_err(r));
+                        }
+                }
+                skip = buffer_len(b);
+                if ((r = sshbuf_put_u8(b, SSH2_MSG_USERAUTH_REQUEST)) != 0 ||
+                    (r = sshbuf_put_cstring(b, authctxt->server_user)) != 0 ||
+                    (r = sshbuf_put_cstring(b, authctxt->service)) != 0 ||
+                    (r = sshbuf_put_cstring(b, authctxt->method->name)) != 0 ||
+                    (r = sshbuf_put_u8(b, 1)) != 0 ||
+                    (r = sshbuf_put_cstring(b, alg)) != 0 ||
+                    (r = sshkey_puts(id->key, b)) != 0) {
+                        fatal("%s: assemble signed data: %s",
+                            __func__, ssh_err(r));
+                }
+
+                /* generate signature */
+                r = identity_sign(sign_id, &signature, &slen,
+                    sshbuf_ptr(b), sshbuf_len(b), datafellows, alg);
+                if (r == 0)
+                        break;
+                else if (r == SSH_ERR_KEY_NOT_FOUND)
+                        goto out; /* soft failure */
+                else if (r == SSH_ERR_SIGN_ALG_UNSUPPORTED &&
+                    !fallback_sigtype) {
+                        if (sign_id->agent_fd != -1)
+                                loc = "agent ";
+                        else if ((sign_id->key->flags & SSHKEY_FLAG_EXT) != 0)
+                                loc = "token ";
+                        logit("%skey %s %s returned incorrect signature type",
+                            loc, sshkey_type(id->key), fp);
+                        continue;
+                }
+                error("%s: signing failed: %s", __func__, ssh_err(r));
+                goto out;
         }
-#ifdef DEBUG_PK
-        buffer_dump(&b);
-#endif
-        free(blob);
+        if (slen == 0 || signature == NULL) /* shouldn't happen */
+                fatal("%s: no signature", __func__);
 
         /* append signature */
-        buffer_put_string(&b, signature, slen);
-        free(signature);
+        if ((r = sshbuf_put_string(b, signature, slen)) != 0)
+                fatal("%s: append signature: %s", __func__, ssh_err(r));
 
+#ifdef DEBUG_PK
+        sshbuf_dump(b, stderr);
+#endif
         /* skip session id and packet type */
-        if (buffer_len(&b) < skip + 1)
-                fatal("userauth_pubkey: internal error");
-        buffer_consume(&b, skip + 1);
+        if ((r = sshbuf_consume(b, skip + 1)) != 0)
+                fatal("%s: consume: %s", __func__, ssh_err(r));
 
         /* put remaining data from buffer into packet */
-        packet_start(SSH2_MSG_USERAUTH_REQUEST);
-        packet_put_raw(buffer_ptr(&b), buffer_len(&b));
-        buffer_free(&b);
-        packet_send();
+        if ((r = sshpkt_start(ssh, SSH2_MSG_USERAUTH_REQUEST)) != 0 ||
+            (r = sshpkt_putb(ssh, b)) != 0 ||
+            (r = sshpkt_send(ssh)) != 0)
+                fatal("%s: enqueue request: %s", __func__, ssh_err(r));
 
-        return 1;
+        /* success */
+        sent = 1;
+
+ out:
+        free(fp);
+        free(alg);
+        sshbuf_free(b);
+        freezero(signature, slen);
+        return sent;
 }
 
 static int
-send_pubkey_test(Authctxt *authctxt, Identity *id)
+send_pubkey_test(struct ssh *ssh, Authctxt *authctxt, Identity *id)
 {
-        u_char *blob;
+        u_char *blob = NULL;
         u_int bloblen, have_sig = 0;
+        char *alg = NULL;
+        int sent = 0;
 
-        debug3("send_pubkey_test");
+        if ((alg = key_sig_algorithm(ssh, id->key)) == NULL) {
+                debug("%s: no mutual signature algorithm", __func__);
+                goto out;
+        }
 
         if (key_to_blob(id->key, &blob, &bloblen) == 0) {
                 /* we cannot handle this key */
-                debug3("send_pubkey_test: cannot handle key");
-                return 0;
+                debug3("%s: cannot handle key", __func__);
+                goto out;
         }
         /* register callback for USERAUTH_PK_OK message */
         dispatch_set(SSH2_MSG_USERAUTH_PK_OK, &input_userauth_pk_ok);
@@ -1222,11 +1276,15 @@ send_pubkey_test(Authctxt *authctxt, Identity *id)
         packet_put_cstring(authctxt->service);
         packet_put_cstring(authctxt->method->name);
         packet_put_char(have_sig);
-        packet_put_cstring(key_sign_encode(id->key));
+        packet_put_cstring(alg);
         packet_put_string(blob, bloblen);
-        free(blob);
         packet_send();
-        return 1;
+        /* success */
+        sent = 1;
+out:
+        free(alg);
+        free(blob);
+        return sent;
 }
 
 static struct sshkey *
@@ -1295,6 +1353,36 @@ load_identity_file(Identity *id)
         return private;
 }
 
+static int
+key_type_allowed_by_config(struct sshkey *key)
+{
+        if (match_pattern_list(sshkey_ssh_name(key),
+            options.pubkey_key_types, 0) == 1)
+                return 1;
+
+        /* RSA keys/certs might be allowed by alternate signature types */
+        switch (key->type) {
+        case KEY_RSA:
+                if (match_pattern_list("rsa-sha2-512",
+                    options.pubkey_key_types, 0) == 1)
+                        return 1;
+                if (match_pattern_list("rsa-sha2-256",
+                    options.pubkey_key_types, 0) == 1)
+                        return 1;
+                break;
+        case KEY_RSA_CERT:
+                if (match_pattern_list("rsa-sha2-512-cert-v01@openssh.com",
+                    options.pubkey_key_types, 0) == 1)
+                        return 1;
+                if (match_pattern_list("rsa-sha2-256-cert-v01@openssh.com",
+                    options.pubkey_key_types, 0) == 1)
+                        return 1;
+                break;
+        }
+        return 0;
+}
+
+
 /*
  * try keys in the following order:
  *      1. certificates listed in the config file
@@ -1419,9 +1507,7 @@ pubkey_prepare(Authctxt *authctxt)
         }
         /* finally, filter by PubkeyAcceptedKeyTypes */
         TAILQ_FOREACH_SAFE(id, preferred, next, id2) {
-                if (id->key != NULL &&
-                    match_pattern_list(sshkey_ssh_name(id->key),
-                    options.pubkey_key_types, 0) != 1) {
+                if (id->key != NULL && !key_type_allowed_by_config(key)) {
                         debug("Skipping %s key %s - "
                             "not in PubkeyAcceptedKeyTypes",
                             sshkey_ssh_name(id->key), id->filename);
@@ -1479,6 +1565,7 @@ try_identity(Identity *id)
 int
 userauth_pubkey(Authctxt *authctxt)
 {
+        struct ssh *ssh = active_state; /* XXX */
         Identity *id;
         int sent = 0;
         char *fp;
@@ -1506,7 +1593,7 @@ userauth_pubkey(Authctxt *authctxt)
                                 debug("Offering public key: %s %s %s",
                                     sshkey_type(id->key), fp, id->filename);
                                 free(fp);
-                                sent = send_pubkey_test(authctxt, id);
+                                sent = send_pubkey_test(ssh, authctxt, id);
                         }
                 } else {
                         debug("Trying private key: %s", id->filename);
@@ -1514,7 +1601,7 @@ userauth_pubkey(Authctxt *authctxt)
                         if (id->key != NULL) {
                                 if (try_identity(id)) {
                                         id->isprivate = 1;
-                                        sent = sign_and_send_pubkey(
+                                        sent = sign_and_send_pubkey(ssh,
                                             authctxt, id);
                                 }
                                 key_free(id->key);
@@ -1735,7 +1822,7 @@ ssh_keysign(struct sshkey *key, u_char **sigp, size_t *lenp,
 int
 userauth_hostbased(Authctxt *authctxt)
 {
-        struct ssh *ssh = active_state;
+        struct ssh *ssh = active_state; /* XXX */
         struct sshkey *private = NULL;
         struct sshbuf *b = NULL;
         u_char *sig = NULL, *keyblob = NULL;