Bug 2680

Summary:

Regression in server-sig-algs offer in 7.4p1 (Deprecation of SHA1 is not being enforced)

Product:

Portable OpenSSH

Reporter:

Jakub Jelen <jjelen>

Component:

ssh

Assignee:

Damien Miller <djm>

Status:

CLOSED FIXED

Severity:

enhancement

CC:

djm, dtucker, nunojpg

Priority:

Version:

7.4p1

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

2647

Attachments:

Description	Flags
patch	dtucker: ok+

Description Jakub Jelen 2017-02-18 03:13:59 AEDT

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 **);

Comment 1 Darren Tucker 2017-02-21 11:05:33 AEDT

add to list for 7.5

Comment 2 Damien Miller 2017-02-24 14:48:27 AEDT

Created attachment 2947 [details]
patch

Comment 3 Darren Tucker 2017-02-24 14:55:34 AEDT

Comment on attachment 2947 [details]
patch

ok, but would it be clearer if that was a bitmask rather than 3 booleans?

Comment 4 Nuno Goncalves 2017-02-24 17:35:58 AEDT

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?)

Comment 5 Damien Miller 2017-03-10 15:18:09 AEDT

Patch is applied and will be in OpenSSH 7.5

Comment 6 Jakub Jelen 2017-07-20 23:08:56 AEST

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>

Comment 7 Damien Miller 2017-07-21 14:47:46 AEST

(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.

Comment 8 Damien Miller 2017-07-21 15:05:35 AEST

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.

Comment 9 Jakub Jelen 2017-07-21 22:24:17 AEST

(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.

Comment 10 Damien Miller 2021-04-23 14:59:56 AEST

closing resolved bugs as of 8.6p1 release