host key algorithm matching, v4 and v6

[Damien Miller]

On Mon, 23 Nov 2020, Stuart Henderson wrote:

> I have a machine (OpenBSD -current) with the following known_hosts
> entries from various old connections:
> 
> v4_address		ecdsa-sha2-nistp256
> v6_address		ecdsa-sha2-nistp256	(line 1515)
> hostname		ecdsa-sha2-nistp256
> hostname,v4_address	ssh-rsa
> hostname,v4_address	ssh-ed25519		(line 1222)
> 
> I've changed my resolver to hand out v6 addresses before v4 addresses
> (I have done this before - given the above entries it looks like it was
> at a time before ssh-ed25519 keys - then reverted it after an ISP problem
> at some point). Now I get hostkey warnings. It's easy enough to cope with
> the change manually of course, but I wonder if the automated behaviour
> could be improved.

[snip]

> Should order_hostkeyalgs consider the address family and actually prefer
> ecdsa-sha2-nistp256 in this case even though it has a better algorithm
> for the host *name*?

ATM order_hostkeyalgs() doesn't consider the address at all - it only
uses the hostname. I'm not sure of a good heuristic to use here.

> Should this warning be skipped if the algorithm differs? (At least in the
> case where UpdateHostKeys is used?)

I have considered something like the following, that disregards the
CheckHostIP warnings if the key matched the name but didn't match the
address *and* the algorithm is a lower priority in HostkeyAlgorithms.

That being said, I'm not entirely clear on exactly what problem
CheckHostIP is trying to solve. It was added to OpenSSH before the
2.x protocol and thus was written prior to a server potentially having
multiple host keys of different types.

-d

diff --git a/sshconnect.c b/sshconnect.c
index f07d342..c5f63ad 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -827,6 +827,41 @@ other_hostkeys_message(const char *host, const char *ip,
 	return ret;
 }
 
+/* Returns the offset of 's' in a comma-separated list */
+static int
+match_list_offset(const char *list, const char *s)
+{
+	const char *cp;
+	size_t l = strlen(s);
+
+	for (cp = list; cp != NULL; ) {
+		if (strncmp(cp, s, l) == 0 && (cp[l] == ',' || cp[l] == '\0'))
+			return (int)(cp - list);
+		if ((cp = strchr(cp, ',')) == NULL)
+			break;
+		cp++;
+	}
+	return -1;
+}
+
+/*
+ * returns nonzero if the type of key 'a' is lower (less preferred)
+ * than key 'b' on HostkeyAlgorithms.
+ */
+static int
+hostkey_priority_lower(const struct sshkey *a, const struct sshkey *b)
+{
+	int ao, bo;
+
+	if (a == NULL || b == NULL)
+		return 0;
+	ao = match_list_offset(options.hostkeyalgorithms, sshkey_ssh_name(a));
+	bo = match_list_offset(options.hostkeyalgorithms, sshkey_ssh_name(b));
+	if (ao == -1 || bo == -1)
+		return 0;
+	return ao > bo;
+}
+
 /*
  * check whether the supplied host key is valid, return -1 if the key
  * is not valid. user_hostfile[0] will not be updated if 'readonly' is true.
@@ -919,9 +954,15 @@ check_host_key(char *hostname, struct sockaddr *hostaddr, u_short port,
 	if (!want_cert && ip_hostkeys != NULL) {
 		ip_status = check_key_in_hostkeys(ip_hostkeys, host_key,
 		    &ip_found);
-		if (host_status == HOST_CHANGED &&
-		    (ip_status != HOST_CHANGED ||
-		    (ip_found != NULL &&
+		if (ip_status == HOST_CHANGED && host_status == HOST_OK &&
+		    hostkey_priority_lower(ip_found->key, host_found->key)) {
+			debug_f("matching lower-priority key type %s for "
+			    "address %s found at %s:%lu",
+			    sshkey_ssh_name(ip_found->key), ip,
+			    ip_found->file, ip_found->line);
+			ip_status = HOST_NEW;
+		} else if (host_status == HOST_CHANGED &&
+		    (ip_status != HOST_CHANGED || (ip_found != NULL &&
 		    !sshkey_equal(ip_found->key, host_found->key))))
 			host_ip_differ = 1;
 	} else