openssh bugs - Feb 2017 - [Bug 2680] New: Regression in server-sig-algs offer in 7.4p1 (Deprecation of SHA1 is not being enforced)

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

            Bug ID: 2680
           Summary: Regression in server-sig-algs offer in 7.4p1
                    (Deprecation of SHA1 is not being enforced)
           Product: Portable OpenSSH
           Version: 7.4p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Nuno Goncalves <nunojpg at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nunojpg at gmail.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2647
                 CC|                            |dtucker at zip.com.au

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> ---
add to list for 7.5


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org
             Status|NEW                         |ASSIGNED
   Attachment #2947|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Created attachment 2947
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2947&action=edit
patch

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2947|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2947
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2947
patch

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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

--- Comment #4 from Nuno Goncalves <nunojpg at gmail.com> ---
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?)

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Patch is applied and will be in OpenSSH 7.5

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #6 from Jakub Jelen <jjelen at redhat.com> ---
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>

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
(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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Jakub Jelen <jjelen at redhat.com> ---
(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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2680

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.