ssh: probable bug in ssh -current

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

ssh: probable bug in ssh -current

Aham Brahmasmi
Namaste misc,

Overview:
In -current (#625), the ssh client is asking the user to accept updated
server host keys after every successful connection. No host keys have
actually been updated at the server side.

Setup:
Consider a server (-current #625) which uses host certificates. The
server's ed25519 host public key is signed by a host certificate
authority.

Consider a client (-current #625) which is configured to use only
ed25519-cert and ed25519 HostKeyAlgorithms. In its known hosts file, the
client has
@cert-authority <server> ssh-ed25519 <host_ca_public_key>
<server> ssh-ed25519 <server_ed25519_host_public_key>.

Bug:
When the client connects to the server, they use the ed25519-cert to
establish the connection. After the ssh session is established, the
server sends the "[hidden email]" message with the server's
ed25519 host public key.

This results in the client looping over the keys in known hosts file,
and deciding that the @cert-authority host certificate authority public
key is "deprecated", because it was not sent by the server [1]. The
client then informs the user:
"
The server has updated its host keys.
These changes were verified by the server's existing trusted key.
Deprecating obsolete hostkey: ED25519 SHA256:<host_ca_public_key_fingerprint>
Accept updated hostkeys? (yes/no):
"

In reality, no host key has been added/removed on the server. Repeat. No
host key has been added/removed on the server.

Now, even when the user types yes in response to the above prompt, the
user is presented with this prompt again after the next successful
login.

This is because when the user types yes, there is a check in the
host_delete function in src/usr.bin/ssh/hostfile.c, which prevents the
@cert-authority keys from being deleted [2].

goto Bug: after every successful login.

And if I am not wrong, this "ask" happened due to the recent changes in
the default setting of UpdateHostKeys option [3][4].

(It is only when users are repeatedly prompted with the same question by
ssh, some get annoyed, few irritated, one complains. And because we had
not changed any keys, and updated everyone+dog to the recent -current
because of the smtpd CVE, it set alarm bells ringing so as to whether
"someone" had actually changed the keys.)

Dhanyavaad,
ab
[1] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/clientloop.c?rev=1.338&content-type=text/x-cvsweb-markup
[2] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/hostfile.c?rev=1.77&content-type=text/x-cvsweb-markup
[3] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readconf.c.diff?r1=1.322&r2=1.323&f=h
[4] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readconf.c.diff?r1=1.323&r2=1.324&f=h
---------|---------|---------|---------|---------|---------|---------|--

Reply | Threaded
Open this post in threaded view
|

Re: ssh: probable bug in ssh -current

Damien Miller
On Fri, 31 Jan 2020, Aham Brahmasmi wrote:

> Bug:
> When the client connects to the server, they use the ed25519-cert to
> establish the connection. After the ssh session is established, the
> server sends the "[hidden email]" message with the server's
> ed25519 host public key.
>
> This results in the client looping over the keys in known hosts file,
> and deciding that the @cert-authority host certificate authority public
> key is "deprecated", because it was not sent by the server [1]. The
> client then informs the user:
> "
> The server has updated its host keys.
> These changes were verified by the server's existing trusted key.
> Deprecating obsolete hostkey: ED25519 SHA256:<host_ca_public_key_fingerprint>
> Accept updated hostkeys? (yes/no):
> "

Could you plesse try this patch?


Index: clientloop.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/clientloop.c,v
retrieving revision 1.338
diff -u -p -r1.338 clientloop.c
--- clientloop.c 30 Jan 2020 07:20:57 -0000 1.338
+++ clientloop.c 3 Feb 2020 02:27:18 -0000
@@ -1856,13 +1859,25 @@ hostkeys_find(struct hostkey_foreach_lin
 
  /* Mark off keys we've already seen for this host */
  for (i = 0; i < ctx->nkeys; i++) {
- if (sshkey_equal(l->key, ctx->keys[i])) {
+ if ((l->marker & MRK_CA) != 0) {
+ if (!sshkey_is_cert(ctx->keys[i]))
+ continue;
+ if (!sshkey_equal(ctx->keys[i]->cert->signature_key,
+    l->key))
+ continue;
+ debug3("%s: found %s CA key at %s:%ld", __func__,
+    sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
+ ctx->keys_seen[i] = 1;
+ return 0;
+ } else if (sshkey_equal(l->key, ctx->keys[i])) {
  debug3("%s: found %s key at %s:%ld", __func__,
     sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
  ctx->keys_seen[i] = 1;
  return 0;
  }
  }
+ if ((l->marker & MRK_REVOKE) != 0)
+ return 0;
  /* This line contained a key that not offered by the server */
  debug3("%s: deprecated %s key at %s:%ld", __func__,
     sshkey_ssh_name(l->key), l->path, l->linenum);
@@ -1961,10 +1976,11 @@ update_known_hosts(struct hostkeys_updat
  if (stat(options.user_hostfiles[i], &sb) != 0) {
  if (errno == ENOENT) {
  debug("%s: known hosts file %s does not exist",
-    __func__, strerror(errno));
+    __func__, options.user_hostfiles[i]);
  } else {
- error("%s: known hosts file %s inaccessible",
-    __func__, strerror(errno));
+ error("%s: known hosts file %s inaccessible: "
+    "%s", __func__, options.user_hostfiles[i],
+    strerror(errno));
  }
  continue;
  }

Reply | Threaded
Open this post in threaded view
|

Re: ssh: probable bug in ssh -current

Aham Brahmasmi
> Sent: Monday, February 03, 2020 at 2:28 AM
> From: "Damien Miller" <[hidden email]>
> To: "Aham Brahmasmi" <[hidden email]>
> Cc: Misc <[hidden email]>
> Subject: Re: ssh: probable bug in ssh -current
>
> On Fri, 31 Jan 2020, Aham Brahmasmi wrote:
>
> > Bug:
> > When the client connects to the server, they use the ed25519-cert to
> > establish the connection. After the ssh session is established, the
> > server sends the "[hidden email]" message with the server's
> > ed25519 host public key.
> >
> > This results in the client looping over the keys in known hosts file,
> > and deciding that the @cert-authority host certificate authority public
> > key is "deprecated", because it was not sent by the server [1]. The
> > client then informs the user:
> > "
> > The server has updated its host keys.
> > These changes were verified by the server's existing trusted key.
> > Deprecating obsolete hostkey: ED25519 SHA256:<host_ca_public_key_fingerprint>
> > Accept updated hostkeys? (yes/no):
> > "
>
> Could you plesse try this patch?
>
>
> Index: clientloop.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/clientloop.c,v
> retrieving revision 1.338
> diff -u -p -r1.338 clientloop.c
> --- clientloop.c 30 Jan 2020 07:20:57 -0000 1.338
> +++ clientloop.c 3 Feb 2020 02:27:18 -0000
> @@ -1856,13 +1859,25 @@ hostkeys_find(struct hostkey_foreach_lin
>
>   /* Mark off keys we've already seen for this host */
>   for (i = 0; i < ctx->nkeys; i++) {
> - if (sshkey_equal(l->key, ctx->keys[i])) {
> + if ((l->marker & MRK_CA) != 0) {
> + if (!sshkey_is_cert(ctx->keys[i]))
> + continue;
> + if (!sshkey_equal(ctx->keys[i]->cert->signature_key,
> +    l->key))
> + continue;
> + debug3("%s: found %s CA key at %s:%ld", __func__,
> +    sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
> + ctx->keys_seen[i] = 1;
> + return 0;
> + } else if (sshkey_equal(l->key, ctx->keys[i])) {
>   debug3("%s: found %s key at %s:%ld", __func__,
>      sshkey_ssh_name(ctx->keys[i]), l->path, l->linenum);
>   ctx->keys_seen[i] = 1;
>   return 0;
>   }
>   }
> + if ((l->marker & MRK_REVOKE) != 0)
> + return 0;
>   /* This line contained a key that not offered by the server */
>   debug3("%s: deprecated %s key at %s:%ld", __func__,
>      sshkey_ssh_name(l->key), l->path, l->linenum);
> @@ -1961,10 +1976,11 @@ update_known_hosts(struct hostkeys_updat
>   if (stat(options.user_hostfiles[i], &sb) != 0) {
>   if (errno == ENOENT) {
>   debug("%s: known hosts file %s does not exist",
> -    __func__, strerror(errno));
> +    __func__, options.user_hostfiles[i]);
>   } else {
> - error("%s: known hosts file %s inaccessible",
> -    __func__, strerror(errno));
> + error("%s: known hosts file %s inaccessible: "
> +    "%s", __func__, options.user_hostfiles[i],
> +    strerror(errno));
>   }
>   continue;
>   }
>

Namaste Damien,

Thank you very much for the patch. I apologize for the delay in my
response.

Applying the patch as is on -current (clientloop.c, v 1.340) did not
resolve the problem. In fact, the patched ssh client tried to
simultaneously add as well as delete the ssh-ed25519 key present in line
2.

As a result, I tried to read and execute the code. Here is what I
understood:

1) The changes to the strings in the error and debug functions work
correctly.

2) I think we may benefit from changing the lines in the patch:
                if ((l->marker & MRK_CA) != 0) {
to
                if (l->marker == MRK_CA) {
and
        if ((l->marker & MRK_REVOKE) != 0)
to
        if (l->marker == MRK_REVOKE)

I base this on what I saw in record_hostkey function in hostfile.c [1].
In case we do intend to perform bit-wise and operations, could you
please share the rationale behind it?

3) Now, with the above change, given that the known_hosts contains:
@cert-authority <server> ssh-ed25519 <host_ca_public_key>
<server> ssh-ed25519 <server_ed25519_host_public_key>

the following is what happens during the first iteration of
hostkeys_find function in the patched clientloop.c (1.340 + patch +
above change):

the execution enters the following if block and the continue is
executed:
                        if (!sshkey_is_cert(ctx->keys[i]))
                                continue;
The sshkey_is_cert function is in the sshkey.c file [2].
...
int
sshkey_type_is_cert(int type)
{
        const struct keytype *kt;

        for (kt = keytypes; kt->type != -1; kt++) {
                if (kt->type == type)
                        return kt->cert;
        }
        return 0;
}
...
int
sshkey_is_cert(const struct sshkey *k)
{
        if (k == NULL)
                return 0;
        return sshkey_type_is_cert(k->type);
}
...
During the execution, this sshkey_is_cert function returns 0. This is
because the argument key is of type=3 which implies "KEY_ED25519"
according to the enum sshkey_types in file sshkey.h [3].
...
/* Key types */
enum sshkey_types {
        KEY_RSA,
        KEY_DSA,
        KEY_ECDSA,
        KEY_ED25519,
        KEY_RSA_CERT,
        KEY_DSA_CERT,
        KEY_ECDSA_CERT,
        KEY_ED25519_CERT,
        KEY_XMSS,
        KEY_XMSS_CERT,
        KEY_ECDSA_SK,
        KEY_ECDSA_SK_CERT,
        KEY_ED25519_SK,
        KEY_ED25519_SK_CERT,
        KEY_UNSPEC
};
...
The enum keytypes is defined in file sshkey.c [2] as
...
/* Supported key types */
struct keytype {
        const char *name;
        const char *shortname;
        const char *sigalg;
        int type;
        int nid;
        int cert;
        int sigonly;
};
static const struct keytype keytypes[] = {
        { "ssh-ed25519", "ED25519", NULL, KEY_ED25519, 0, 0, 0 },
        { "[hidden email]", "ED25519-CERT", NULL,
            KEY_ED25519_CERT, 0, 1, 0 },
...

Since "ssh-ed25519" (KEY_ED25519) has cert=0, the sshkey_is_cert
function returns 0.

Now, the notify_hostkeys function in the file sshd.c [4] contains
...
        for (i = nkeys = 0; i < options.num_host_key_files; i++) {
                key = get_hostkey_public_by_index(i, ssh);
                if (key == NULL || key->type == KEY_UNSPEC ||
                    sshkey_is_cert(key))
                        continue;
...
which implies that the ssh daemon does not seem to send the ssh
host certificate keys as part of the hostkeys-00 message.

Further, the client_input_hostkeys function in the file clientloop.c [5]
contains
...
                /* Skip certs */
                if (sshkey_is_cert(key)) {
                        debug3("%s: %s key is a certificate; skipping",
                            __func__, sshkey_ssh_name(key));
                        continue;
                }
...
which implies that the ssh client seems to be skipping the ssh host
certificate keys.

So, there does not seem to be any way to satisfy the sshkey_is_cert
function check in the patch, because from what I understand, the server
does not seem to send the certificate keys and the client skips the
certificate keys before iterating through the known_hosts file.

I am out of ideas beyond this. I am sorry. If there is a stupid mistake
that I am making here, please let me know.

Dhanyavaad,
ab
[1] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/hostfile.c?rev=1.77&content-type=text/x-cvsweb-markup
[2] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/sshkey.c?rev=1.99&content-type=text/x-cvsweb-markup
[3] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/sshkey.h?rev=1.44&content-type=text/x-cvsweb-markup
[4] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/sshd.c?rev=1.549&content-type=text/x-cvsweb-markup
[5] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/clientloop.c?rev=1.340&content-type=text/x-cvsweb-markup
---------|---------|---------|---------|---------|---------|---------|--