upstream: repair PubkeyAcceptedKeyTypes (and friends) after RSA

signature work - returns ability to add/remove/specify algorithms by
wildcard.

Algorithm lists are now fully expanded when the server/client configs
are finalised, so errors are reported early and the config dumps
(e.g. "ssh -G ...") now list the actual algorithms selected.

Clarify that, while wildcards are accepted in algorithm lists, they
aren't full pattern-lists that support negation.

(lots of) feedback, ok markus@

OpenBSD-Commit-ID: a8894c5c81f399a002f02ff4fe6b4fa46b1f3207

10 files changed, 187 insertions, 67 deletions

diff --git a/compat.c b/compat.c
index 8335f2a9..d8fd6eaf 100644
--- a/compat.c
+++ b/compat.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: compat.c,v 1.109 2018/07/03 11:42:12 djm Exp $ */
+/* $OpenBSD: compat.c,v 1.110 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl.  All rights reserved.
  *
@@ -190,8 +190,8 @@ compat_cipher_proposal(char *cipher_prop)
 	if (!(datafellows & SSH_BUG_BIGENDIANAES))
 		return cipher_prop;
 	debug2("%s: original cipher proposal: %s", __func__, cipher_prop);
-	if ((cipher_prop = match_filter_list(cipher_prop, "aes*")) == NULL)
-		fatal("match_filter_list failed");
+	if ((cipher_prop = match_filter_blacklist(cipher_prop, "aes*")) == NULL)
+		fatal("match_filter_blacklist failed");
 	debug2("%s: compat cipher proposal: %s", __func__, cipher_prop);
 	if (*cipher_prop == '\0')
 		fatal("No supported ciphers found");
@@ -204,8 +204,8 @@ compat_pkalg_proposal(char *pkalg_prop)
 	if (!(datafellows & SSH_BUG_RSASIGMD5))
 		return pkalg_prop;
 	debug2("%s: original public key proposal: %s", __func__, pkalg_prop);
-	if ((pkalg_prop = match_filter_list(pkalg_prop, "ssh-rsa")) == NULL)
-		fatal("match_filter_list failed");
+	if ((pkalg_prop = match_filter_blacklist(pkalg_prop, "ssh-rsa")) == NULL)
+		fatal("match_filter_blacklist failed");
 	debug2("%s: compat public key proposal: %s", __func__, pkalg_prop);
 	if (*pkalg_prop == '\0')
 		fatal("No supported PK algorithms found");
@@ -219,14 +219,14 @@ compat_kex_proposal(char *p)
 		return p;
 	debug2("%s: original KEX proposal: %s", __func__, p);
 	if ((datafellows & SSH_BUG_CURVE25519PAD) != 0)
-		if ((p = match_filter_list(p,
+		if ((p = match_filter_blacklist(p,
 		    "curve25519-sha256@libssh.org")) == NULL)
-			fatal("match_filter_list failed");
+			fatal("match_filter_blacklist failed");
 	if ((datafellows & SSH_OLD_DHGEX) != 0) {
-		if ((p = match_filter_list(p,
+		if ((p = match_filter_blacklist(p,
 		    "diffie-hellman-group-exchange-sha256,"
 		    "diffie-hellman-group-exchange-sha1")) == NULL)
-			fatal("match_filter_list failed");
+			fatal("match_filter_blacklist failed");
 	}
 	debug2("%s: compat KEX proposal: %s", __func__, p);
 	if (*p == '\0')
diff --git a/kex.c b/kex.c
index d0a5f1b6..2fd052e9 100644
--- a/kex.c
+++ b/kex.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.c,v 1.137 2018/07/03 11:39:54 djm Exp $ */
+/* $OpenBSD: kex.c,v 1.138 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
  *
@@ -174,7 +174,7 @@ kex_names_cat(const char *a, const char *b)
 	size_t len;
 
 	if (a == NULL || *a == '\0')
-		return NULL;
+		return strdup(b);
 	if (b == NULL || *b == '\0')
 		return strdup(a);
 	if (strlen(b) > 1024*1024)
@@ -209,27 +209,88 @@ kex_names_cat(const char *a, const char *b)
  * specified names should be removed.
  */
 int
-kex_assemble_names(const char *def, char **list)
+kex_assemble_names(char **listp, const char *def, const char *all)
 {
-	char *ret;
+	char *cp, *tmp, *patterns;
+	char *list = NULL, *ret = NULL, *matching = NULL, *opatterns = NULL;
+	int r = SSH_ERR_INTERNAL_ERROR;
 
-	if (list == NULL || *list == NULL || **list == '\0') {
-		*list = strdup(def);
+	if (listp == NULL || *listp == NULL || **listp == '\0') {
+		if ((*listp = strdup(def)) == NULL)
+			return SSH_ERR_ALLOC_FAIL;
 		return 0;
 	}
-	if (**list == '+') {
-		if ((ret = kex_names_cat(def, *list + 1)) == NULL)
-			return SSH_ERR_ALLOC_FAIL;
-		free(*list);
-		*list = ret;
-	} else if (**list == '-') {
-		if ((ret = match_filter_list(def, *list + 1)) == NULL)
-			return SSH_ERR_ALLOC_FAIL;
-		free(*list);
-		*list = ret;
+
+	list = *listp;
+	*listp = NULL;
+	if (*list == '+') {
+		/* Append names to default list */
+		if ((tmp = kex_names_cat(def, list + 1)) == NULL) {
+			r = SSH_ERR_ALLOC_FAIL;
+			goto fail;
+		}
+		free(list);
+		list = tmp;
+	} else if (*list == '-') {
+		/* Remove names from default list */
+		if ((*listp = match_filter_blacklist(def, list + 1)) == NULL) {
+			r = SSH_ERR_ALLOC_FAIL;
+			goto fail;
+		}
+		free(list);
+		/* filtering has already been done */
+		return 0;
+	} else {
+		/* Explicit list, overrides default - just use "list" as is */
 	}
 
-	return 0;
+	/*
+	 * The supplied names may be a pattern-list. For the -list case,
+	 * the patterns are applied above. For the +list and explicit list
+	 * cases we need to do it now.
+	 */
+	ret = NULL;
+	if ((patterns = opatterns = strdup(list)) == NULL) {
+		r = SSH_ERR_ALLOC_FAIL;
+		goto fail;
+	}
+	/* Apply positive (i.e. non-negated) patterns from the list */
+	while ((cp = strsep(&patterns, ",")) != NULL) {
+		if (*cp == '!') {
+			/* negated matches are not supported here */
+			r = SSH_ERR_INVALID_ARGUMENT;
+			goto fail;
+		}
+		free(matching);
+		if ((matching = match_filter_whitelist(all, cp)) == NULL) {
+			r = SSH_ERR_ALLOC_FAIL;
+			goto fail;
+		}
+		if ((tmp = kex_names_cat(ret, matching)) == NULL) {
+			r = SSH_ERR_ALLOC_FAIL;
+			goto fail;
+		}
+		free(ret);
+		ret = tmp;
+	}
+	if (ret == NULL || *ret == '\0') {
+		/* An empty name-list is an error */
+		/* XXX better error code? */
+		r = SSH_ERR_INVALID_ARGUMENT;
+		goto fail;
+	}
+
+	/* success */
+	*listp = ret;
+	ret = NULL;
+	r = 0;
+
+ fail:
+	free(matching);
+	free(opatterns);
+	free(list);
+	free(ret);
+	return r;
 }
 
 /* put algorithm proposal into buffer */
diff --git a/kex.h b/kex.h
index 6210630d..3ffae2df 100644
--- a/kex.h
+++ b/kex.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.h,v 1.84 2018/07/03 11:39:54 djm Exp $ */
+/* $OpenBSD: kex.h,v 1.85 2018/07/04 13:49:31 djm Exp $ */
 
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
@@ -169,7 +169,7 @@ struct kex {
 int	 kex_names_valid(const char *);
 char	*kex_alg_list(char);
 char	*kex_names_cat(const char *, const char *);
-int	 kex_assemble_names(const char *, char **);
+int	 kex_assemble_names(char **, const char *, const char *);
 
 int	 kex_new(struct ssh *, char *[PROPOSAL_MAX], struct kex **);
 int	 kex_setup(struct ssh *, char *[PROPOSAL_MAX]);
diff --git a/match.c b/match.c
index 3cf40306..bb3e95f6 100644
--- a/match.c
+++ b/match.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: match.c,v 1.37 2017/03/10 04:24:55 djm Exp $ */
+/* $OpenBSD: match.c,v 1.38 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -294,16 +294,20 @@ match_list(const char *client, const char *server, u_int *next)
 }
 
 /*
- * Filters a comma-separated list of strings, excluding any entry matching
- * the 'filter' pattern list. Caller must free returned string.
+ * Filter proposal using pattern-list filter.
+ * "blacklist" determines sense of filter:
+ * non-zero indicates that items matching filter should be excluded.
+ * zero indicates that only items matching filter should be included.
+ * returns NULL on allocation error, otherwise caller must free result.
  */
-char *
-match_filter_list(const char *proposal, const char *filter)
+static char *
+filter_list(const char *proposal, const char *filter, int blacklist)
 {
 	size_t len = strlen(proposal) + 1;
 	char *fix_prop = malloc(len);
 	char *orig_prop = strdup(proposal);
 	char *cp, *tmp;
+	int r;
 
 	if (fix_prop == NULL || orig_prop == NULL) {
 		free(orig_prop);
@@ -314,7 +318,8 @@ match_filter_list(const char *proposal, const char *filter)
 	tmp = orig_prop;
 	*fix_prop = '\0';
 	while ((cp = strsep(&tmp, ",")) != NULL) {
-		if (match_pattern_list(cp, filter, 0) != 1) {
+		r = match_pattern_list(cp, filter, 0);
+		if ((blacklist && r != 1) || (!blacklist && r == 1)) {
 			if (*fix_prop != '\0')
 				strlcat(fix_prop, ",", len);
 			strlcat(fix_prop, cp, len);
@@ -324,3 +329,22 @@ match_filter_list(const char *proposal, const char *filter)
 	return fix_prop;
 }
 
+/*
+ * Filters a comma-separated list of strings, excluding any entry matching
+ * the 'filter' pattern list. Caller must free returned string.
+ */
+char *
+match_filter_blacklist(const char *proposal, const char *filter)
+{
+	return filter_list(proposal, filter, 1);
+}
+
+/*
+ * Filters a comma-separated list of strings, including only entries matching
+ * the 'filter' pattern list. Caller must free returned string.
+ */
+char *
+match_filter_whitelist(const char *proposal, const char *filter)
+{
+	return filter_list(proposal, filter, 0);
+}
diff --git a/match.h b/match.h
index 937ba041..852b1a5c 100644
--- a/match.h
+++ b/match.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: match.h,v 1.17 2017/02/03 23:01:19 djm Exp $ */
+/* $OpenBSD: match.h,v 1.18 2018/07/04 13:49:31 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -20,7 +20,8 @@ int	 match_hostname(const char *, const char *);
 int	 match_host_and_ip(const char *, const char *, const char *);
 int	 match_user(const char *, const char *, const char *, const char *);
 char	*match_list(const char *, const char *, u_int *);
-char	*match_filter_list(const char *, const char *);
+char	*match_filter_blacklist(const char *, const char *);
+char	*match_filter_whitelist(const char *, const char *);
 
 /* addrmatch.c */
 int	 addr_match_list(const char *, const char *);
diff --git a/readconf.c b/readconf.c
index 8d202954..2bc27075 100644
--- a/readconf.c
+++ b/readconf.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: readconf.c,v 1.291 2018/06/10 23:45:41 djm Exp $ */
+/* $OpenBSD: readconf.c,v 1.292 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1936,6 +1936,8 @@ fill_default_options_for_canonicalization(Options *options)
 void
 fill_default_options(Options * options)
 {
+	char *all_cipher, *all_mac, *all_kex, *all_key;
+
 	if (options->forward_agent == -1)
 		options->forward_agent = 0;
 	if (options->forward_x11 == -1)
@@ -2082,14 +2084,27 @@ fill_default_options(Options * options)
 		options->fingerprint_hash = SSH_FP_HASH_DEFAULT;
 	if (options->update_hostkeys == -1)
 		options->update_hostkeys = 0;
-	if (kex_assemble_names(KEX_CLIENT_ENCRYPT, &options->ciphers) != 0 ||
-	    kex_assemble_names(KEX_CLIENT_MAC, &options->macs) != 0 ||
-	    kex_assemble_names(KEX_CLIENT_KEX, &options->kex_algorithms) != 0 ||
-	    kex_assemble_names(KEX_DEFAULT_PK_ALG,
-	    &options->hostbased_key_types) != 0 ||
-	    kex_assemble_names(KEX_DEFAULT_PK_ALG,
-	    &options->pubkey_key_types) != 0)
+
+	/* Expand KEX name lists */
+	all_cipher = cipher_alg_list(',', 0);
+	all_mac = mac_alg_list(',');
+	all_kex = kex_alg_list(',');
+	all_key = sshkey_alg_list(0, 0, 1, ',');
+	if (kex_assemble_names(&options->ciphers,
+	    KEX_CLIENT_ENCRYPT, all_cipher) != 0 ||
+	    kex_assemble_names(&options->macs,
+	    KEX_CLIENT_MAC, all_mac) != 0 ||
+	    kex_assemble_names(&options->kex_algorithms,
+	    KEX_CLIENT_KEX, all_kex) != 0 ||
+	    kex_assemble_names(&options->hostbased_key_types,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0 ||
+	    kex_assemble_names(&options->pubkey_key_types,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0)
 		fatal("%s: kex_assemble_names failed", __func__);
+	free(all_cipher);
+	free(all_mac);
+	free(all_kex);
+	free(all_key);
 
 #define CLEAR_ON_NONE(v) \
 	do { \
@@ -2537,11 +2552,14 @@ void
 dump_client_config(Options *o, const char *host)
 {
 	int i;
-	char buf[8];
+	char buf[8], *all_key;
 
 	/* This is normally prepared in ssh_kex2 */
-	if (kex_assemble_names(KEX_DEFAULT_PK_ALG, &o->hostkeyalgorithms) != 0)
+	all_key = sshkey_alg_list(0, 0, 1, ',');
+	if (kex_assemble_names( &o->hostkeyalgorithms,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0)
 		fatal("%s: kex_assemble_names failed", __func__);
+	free(all_key);
 
 	/* Most interesting options first: user, host, port */
 	dump_cfg_string(oUser, o->user);
diff --git a/servconf.c b/servconf.c
index a41fdc26..a54219f0 100644
--- a/servconf.c
+++ b/servconf.c
@@ -1,5 +1,5 @@
 
-/* $OpenBSD: servconf.c,v 1.334 2018/07/03 10:59:35 djm Exp $ */
+/* $OpenBSD: servconf.c,v 1.335 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
  *                    All rights reserved
@@ -190,15 +190,29 @@ option_clear_or_none(const char *o)
 static void
 assemble_algorithms(ServerOptions *o)
 {
-	if (kex_assemble_names(KEX_SERVER_ENCRYPT, &o->ciphers) != 0 ||
-	    kex_assemble_names(KEX_SERVER_MAC, &o->macs) != 0 ||
-	    kex_assemble_names(KEX_SERVER_KEX, &o->kex_algorithms) != 0 ||
-	    kex_assemble_names(KEX_DEFAULT_PK_ALG,
-	    &o->hostkeyalgorithms) != 0 ||
-	    kex_assemble_names(KEX_DEFAULT_PK_ALG,
-	    &o->hostbased_key_types) != 0 ||
-	    kex_assemble_names(KEX_DEFAULT_PK_ALG, &o->pubkey_key_types) != 0)
+	char *all_cipher, *all_mac, *all_kex, *all_key;
+
+	all_cipher = cipher_alg_list(',', 0);
+	all_mac = mac_alg_list(',');
+	all_kex = kex_alg_list(',');
+	all_key = sshkey_alg_list(0, 0, 1, ',');
+	if (kex_assemble_names(&o->ciphers,
+	    KEX_SERVER_ENCRYPT, all_cipher) != 0 ||
+	    kex_assemble_names(&o->macs,
+	    KEX_SERVER_MAC, all_mac) != 0 ||
+	    kex_assemble_names(&o->kex_algorithms,
+	    KEX_SERVER_KEX, all_kex) != 0 ||
+	    kex_assemble_names(&o->hostkeyalgorithms,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0 ||
+	    kex_assemble_names(&o->hostbased_key_types,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0 ||
+	    kex_assemble_names(&o->pubkey_key_types,
+	    KEX_DEFAULT_PK_ALG, all_key) != 0)
 		fatal("kex_assemble_names failed");
+	free(all_cipher);
+	free(all_mac);
+	free(all_kex);
+	free(all_key);
 }
 
 static void
diff --git a/ssh_config.5 b/ssh_config.5
index eff9c5e6..df94d60d 100644
--- a/ssh_config.5
+++ b/ssh_config.5
@@ -33,8 +33,8 @@
 .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.\" $OpenBSD: ssh_config.5,v 1.278 2018/07/03 11:39:54 djm Exp $
-.Dd $Mdocdate: July 3 2018 $
+.\" $OpenBSD: ssh_config.5,v 1.279 2018/07/04 13:49:31 djm Exp $
+.Dd $Mdocdate: July 4 2018 $
 .Dt SSH_CONFIG 5
 .Os
 .Sh NAME
@@ -757,7 +757,7 @@ or
 (the default).
 .It Cm HostbasedKeyTypes
 Specifies the key types that will be used for hostbased authentication
-as a comma-separated pattern list.
+as a comma-separated list of patterns.
 Alternately if the specified value begins with a
 .Sq +
 character, then the specified key types will be appended to the default set
@@ -1242,7 +1242,7 @@ The default is
 .Cm no .
 .It Cm PubkeyAcceptedKeyTypes
 Specifies the key types that will be used for public key authentication
-as a comma-separated pattern list.
+as a comma-separated list of patterns.
 Alternately if the specified value begins with a
 .Sq +
 character, then the key types after it will be appended to the default
diff --git a/sshconnect2.c b/sshconnect2.c
index db95cb21..f3ccd53a 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sshconnect2.c,v 1.274 2018/07/03 13:20:25 djm Exp $ */
+/* $OpenBSD: sshconnect2.c,v 1.275 2018/07/04 13:49:31 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  * Copyright (c) 2008 Damien Miller.  All rights reserved.
@@ -158,7 +158,7 @@ void
 ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port)
 {
 	char *myproposal[PROPOSAL_MAX] = { KEX_CLIENT };
-	char *s;
+	char *s, *all_key;
 	struct kex *kex;
 	int r;
 
@@ -178,9 +178,11 @@ ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port)
 	myproposal[PROPOSAL_MAC_ALGS_CTOS] =
 	    myproposal[PROPOSAL_MAC_ALGS_STOC] = options.macs;
 	if (options.hostkeyalgorithms != NULL) {
-		if (kex_assemble_names(KEX_DEFAULT_PK_ALG,
-		    &options.hostkeyalgorithms) != 0)
+		all_key = sshkey_alg_list(0, 0, 1, ',');
+		if (kex_assemble_names(&options.hostkeyalgorithms,
+		    KEX_DEFAULT_PK_ALG, all_key) != 0)
 			fatal("%s: kex_assemble_namelist", __func__);
+		free(all_key);
 		myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] =
 		    compat_pkalg_proposal(options.hostkeyalgorithms);
 	} else {
diff --git a/sshd_config.5 b/sshd_config.5
index cc019ec7..aa888796 100644
--- a/sshd_config.5
+++ b/sshd_config.5
@@ -33,8 +33,8 @@
 .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.\" $OpenBSD: sshd_config.5,v 1.279 2018/07/03 11:39:54 djm Exp $
-.Dd $Mdocdate: July 3 2018 $
+.\" $OpenBSD: sshd_config.5,v 1.280 2018/07/04 13:49:31 djm Exp $
+.Dd $Mdocdate: July 4 2018 $
 .Dt SSHD_CONFIG 5
 .Os
 .Sh NAME
@@ -659,7 +659,7 @@ The default is
 .Cm yes .
 .It Cm HostbasedAcceptedKeyTypes
 Specifies the key types that will be accepted for hostbased authentication
-as a comma-separated pattern list.
+as a list of comma-separated patterns.
 Alternately if the specified value begins with a
 .Sq +
 character, then the specified key types will be appended to the default set
@@ -1386,7 +1386,7 @@ The default is
 .Cm yes .
 .It Cm PubkeyAcceptedKeyTypes
 Specifies the key types that will be accepted for public key authentication
-as a comma-separated pattern list.
+as a list of comma-separated patterns.
 Alternately if the specified value begins with a
 .Sq +
 character, then the specified key types will be appended to the default set