Cross-filled from the mailing list to get some more attention: The server-sig algorithms changed with commit 130f5d before: server-sig-algs=<rsa-sha2-256,rsa-sha2-512> after: server-sig-algs=<ssh-ed25519,ssh-rsa,ssh-dss,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521> http://lists.mindrot.org/pipermail/openssh-unix-dev/2017-February/035785.html Can we get that fixed? It prevents using the new signatures and falls back unconditionally to sha1. ---------------------- original email ------------------------------ The side effect of this bug is that my "problem" originally reported disappeared from 7.3p1 to 7.4p1. It was fixed by properly supporting rsa-sha2-256 from OpenSC (my pkcs11 lib) side, but during tests we found out that 7.4p1 was not using rsa-sha2-256 anymore. Bug was introduced with commit: https://github.com/openssh/openssh-portable/commit/130f5df4fa37cace8c079dccb690e5cafbf00751. Due to: https://bugzilla.mindrot.org/show_bug.cgi?id=2547 From this commit rsa-sha2-256 and rsa-sha2-512 are no longer offered so all is downgraded to rsa-sha. A fix applied at current master could be: diff --git a/kex.c b/kex.c index a30dabe..13bb9aa 100644 --- a/kex.c +++ b/kex.c @@ -348,7 +348,7 @@ kex_send_ext_info(struct ssh *ssh) int r; char *algs; - if ((algs = sshkey_alg_list(0, 1, ',')) == NULL) + if ((algs = sshkey_alg_list(0, 1, 1, ',')) == NULL) return SSH_ERR_ALLOC_FAIL; if ((r = sshpkt_start(ssh, SSH2_MSG_EXT_INFO)) != 0 || (r = sshpkt_put_u32(ssh, 1)) != 0 || diff --git a/ssh.c b/ssh.c index ee0b16d..edef335 100644 --- a/ssh.c +++ b/ssh.c @@ -684,11 +684,11 @@ main(int ac, char **av) else if (strcmp(optarg, "kex") == 0) cp = kex_alg_list('\n'); else if (strcmp(optarg, "key") == 0) - cp = sshkey_alg_list(0, 0, '\n'); + cp = sshkey_alg_list(0, 0, 0, '\n'); else if (strcmp(optarg, "key-cert") == 0) - cp = sshkey_alg_list(1, 0, '\n'); + cp = sshkey_alg_list(1, 0, 0, '\n'); else if (strcmp(optarg, "key-plain") == 0) - cp = sshkey_alg_list(0, 1, '\n'); + cp = sshkey_alg_list(0, 1, 0, '\n'); else if (strcmp(optarg, "protocol-version") == 0) { #ifdef WITH_SSH1 cp = xstrdup("1\n2"); diff --git a/sshkey.c b/sshkey.c index 31710e5..1c5dfdb 100644 --- a/sshkey.c +++ b/sshkey.c @@ -195,14 +195,16 @@ sshkey_ecdsa_nid_from_name(const char *name) } char * -sshkey_alg_list(int certs_only, int plain_only, char sep) +sshkey_alg_list(int certs_only, int plain_only, int sigonly_also, char sep) { char *tmp, *ret = NULL; size_t nlen, rlen = 0; const struct keytype *kt; for (kt = keytypes; kt->type != -1; kt++) { - if (kt->name == NULL || kt->sigonly) + if (kt->name == NULL) + continue; + if (!sigonly_also && kt->sigonly) continue; if ((certs_only && !kt->cert) || (plain_only && kt->cert)) continue; diff --git a/sshkey.h b/sshkey.h index f393638..6a3ff2f 100644 --- a/sshkey.h +++ b/sshkey.h @@ -156,7 +156,7 @@ int sshkey_ec_validate_private(const EC_KEY *); const char *sshkey_ssh_name(const struct sshkey *); const char *sshkey_ssh_name_plain(const struct sshkey *); int sshkey_names_valid2(const char *, int); -char *sshkey_alg_list(int, int, char); +char *sshkey_alg_list(int, int, int, char); int sshkey_from_blob(const u_char *, size_t, struct sshkey **); int sshkey_fromb(struct sshbuf *, struct sshkey **);
add to list for 7.5
Created attachment 2947 [details] patch
Comment on attachment 2947 [details] patch ok, but would it be clearer if that was a bitmask rather than 3 booleans?
I'm the patch author. I agree a bitmask is preferred, but I'm unsure of the names to give the constants. The bools currently are: int certs_only int plain_only int sigonly_also Maybe the constants could be in a additive model as: PLAIN CERTS SIGN (or HASH_EXT?)
Patch is applied and will be in OpenSSH 7.5
Although the patch looks reasonable and I considered it as a resolved issue, it is not as the current master (openssh 7.5) still reports: debug1: kex_input_ext_info: server-sig-algs=<ssh-ed25519,ssh-rsa,rsa-sha2-256,rsa-sha2-512,ssh-dss,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,null> The problem is in the order of the checks in the condition "!include_sigonly && kt->sigonly". With the following patch I can see the correct list offered by the server again: diff --git a/sshkey.c b/sshkey.c --- a/sshkey.c +++ b/sshkey.c @@ -203,7 +203,7 @@ sshkey_alg_list(int certs_only, int plain_only, int include_sigonly, char sep) for (kt = keytypes; kt->type != -1; kt++) { if (kt->name == NULL) continue; - if (!include_sigonly && kt->sigonly) + if (include_sigonly && !kt->sigonly) continue; if ((certs_only && !kt->cert) || (plain_only && kt->cert)) continue; The correct list: debug1: kex_input_ext_info: server-sig-algs=<rsa-sha2-256,rsa-sha2-512>
(In reply to Jakub Jelen from comment #6) > Although the patch looks reasonable and I considered it as a > resolved issue, it is not as the current master (openssh 7.5) still > reports: > > debug1: kex_input_ext_info: > server-sig-algs=<ssh-ed25519,ssh-rsa,rsa-sha2-256,rsa-sha2-512,ssh- > dss,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,null> That's AFAIK what it's supposed to be, excepting the "null" at the end of the list - where does that come from? > The correct list: > > debug1: kex_input_ext_info: > server-sig-algs=<rsa-sha2-256,rsa-sha2-512> Doesn't list non-RSA signature algorithms. Per https://tools.ietf.org/html/draft-ietf-curdle-ssh-ext-info-10 : > This extension is sent by the server, and contains a list of public > key algorithms that the server is able to process as part of a > "publickey" authentication request. That doesn't limit the contents to just new signature algorithms. We don't currently provide a knob to disable SHA1 signtures, but feel free to file another bug to request it and I'll try to get it done before 7.6.
Though there at least one error in the contents of server-sig-algs: we shouldn't offer ssh-dss when we're unwilling to offer a ssh-dss hostkey (true by default). I'll look at filtering the contents for that.
(In reply to Damien Miller from comment #7) > (In reply to Jakub Jelen from comment #6) > > Although the patch looks reasonable and I considered it as a > > resolved issue, it is not as the current master (openssh 7.5) still > > reports: > > > > debug1: kex_input_ext_info: > > server-sig-algs=<ssh-ed25519,ssh-rsa,rsa-sha2-256,rsa-sha2-512,ssh- > > dss,ecdsa-sha2-nistp256,ecdsa-sha2-nistp384,ecdsa-sha2-nistp521,null> > > That's AFAIK what it's supposed to be, excepting the "null" at the > end of the list - where does that come from? That is gssapi key exchange. Sorry for confusion. > > The correct list: > > > > debug1: kex_input_ext_info: > > server-sig-algs=<rsa-sha2-256,rsa-sha2-512> > > Doesn't list non-RSA signature algorithms. Per > https://tools.ietf.org/html/draft-ietf-curdle-ssh-ext-info-10 : > > > This extension is sent by the server, and contains a list of public > > key algorithms that the server is able to process as part of a > > "publickey" authentication request. > > That doesn't limit the contents to just new signature algorithms. Ok. So it was a change from the initial implementation. Thanks for a clarification. But I am wondering what is the point of of listing all the algorithms that are already defined by the standard in extension. They are ignored by OpenSSH at least. > We don't currently provide a knob to disable SHA1 signtures, but > feel free to file another bug to request it and I'll try to get it > done before 7.6. I will do if it is the time already (it was not some time ago). > Though there at least one error in the contents of server-sig-algs: we shouldn't offer ssh-dss when we're unwilling to offer a ssh-dss hostkey (true by default). That is one of the thing I things why it is bogus to list all supported pkalgs, when they are already negotiated. Closing again, since it looks like it is correct according to the draft. I will fill separate bugs for the other issues.
closing resolved bugs as of 8.6p1 release