[openssh-commits] [openssh] 04/04: upstream: ensure that certificate extensions are lexically sorted.

git+noreply at mindrot.org git+noreply at mindrot.org
Mon Aug 3 14:28:08 AEST 2020


This is an automated email from the git hooks/post-receive script.

djm pushed a commit to branch master
in repository openssh.

commit 2d8a3b7e8b0408dfeb933ac5cfd3a58f5bac49af
Author: djm at openbsd.org <djm at openbsd.org>
Date:   Mon Aug 3 02:53:51 2020 +0000

    upstream: ensure that certificate extensions are lexically sorted.
    
    Previously if the user specified a custom extension then the everything would
    be in order except the custom ones. bz3198 ok dtucker markus
    
    OpenBSD-Commit-ID: d97deb90587b06cb227c66ffebb2d9667bf886f0
---
 ssh-keygen.c | 152 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 85 insertions(+), 67 deletions(-)

diff --git a/ssh-keygen.c b/ssh-keygen.c
index 4a27d3bf..cc092368 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.414 2020/07/15 07:50:46 solene Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.415 2020/08/03 02:53:51 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo at cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo at cs.hut.fi>, Espoo, Finland
@@ -133,13 +133,13 @@ static char *certflags_command = NULL;
 static char *certflags_src_addr = NULL;
 
 /* Arbitrary extensions specified by user */
-struct cert_userext {
+struct cert_ext {
 	char *key;
 	char *val;
 	int crit;
 };
-static struct cert_userext *cert_userext;
-static size_t ncert_userext;
+static struct cert_ext *cert_ext;
+static size_t ncert_ext;
 
 /* Conversion to/from various formats */
 enum {
@@ -1601,31 +1601,32 @@ do_change_comment(struct passwd *pw, const char *identity_comment)
 }
 
 static void
-add_flag_option(struct sshbuf *c, const char *name)
+cert_ext_add(const char *key, const char *value, int iscrit)
 {
-	int r;
-
-	debug3("%s: %s", __func__, name);
-	if ((r = sshbuf_put_cstring(c, name)) != 0 ||
-	    (r = sshbuf_put_string(c, NULL, 0)) != 0)
-		fatal("%s: buffer error: %s", __func__, ssh_err(r));
+	cert_ext = xreallocarray(cert_ext, ncert_ext + 1, sizeof(*cert_ext));
+	cert_ext[ncert_ext].key = xstrdup(key);
+	cert_ext[ncert_ext].val = value == NULL ? NULL : xstrdup(value);
+	cert_ext[ncert_ext].crit = iscrit;
+	ncert_ext++;
 }
 
-static void
-add_string_option(struct sshbuf *c, const char *name, const char *value)
+/* qsort(3) comparison function for certificate extensions */
+static int
+cert_ext_cmp(const void *_a, const void *_b)
 {
-	struct sshbuf *b;
+	const struct cert_ext *a = (const struct cert_ext *)_a;
+	const struct cert_ext *b = (const struct cert_ext *)_b;
 	int r;
 
-	debug3("%s: %s=%s", __func__, name, value);
-	if ((b = sshbuf_new()) == NULL)
-		fatal("%s: sshbuf_new failed", __func__);
-	if ((r = sshbuf_put_cstring(b, value)) != 0 ||
-	    (r = sshbuf_put_cstring(c, name)) != 0 ||
-	    (r = sshbuf_put_stringb(c, b)) != 0)
-		fatal("%s: buffer error: %s", __func__, ssh_err(r));
-
-	sshbuf_free(b);
+	if (a->crit != b->crit)
+		return (a->crit < b->crit) ? -1 : 1;
+	if ((r = strcmp(a->key, b->key)) != 0)
+		return r;
+	if ((a->val == NULL) != (b->val == NULL))
+		return (a->val == NULL) ? -1 : 1;
+	if (a->val != NULL && (r = strcmp(a->val, b->val)) != 0)
+		return r;
+	return 0;
 }
 
 #define OPTIONS_CRITICAL	1
@@ -1633,44 +1634,62 @@ add_string_option(struct sshbuf *c, const char *name, const char *value)
 static void
 prepare_options_buf(struct sshbuf *c, int which)
 {
+	struct sshbuf *b;
 	size_t i;
+	int r;
+	const struct cert_ext *ext;
 
+	if ((b = sshbuf_new()) == NULL)
+		fatal("%s: sshbuf_new failed", __func__);
 	sshbuf_reset(c);
-	if ((which & OPTIONS_CRITICAL) != 0 &&
-	    certflags_command != NULL)
-		add_string_option(c, "force-command", certflags_command);
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_X_FWD) != 0)
-		add_flag_option(c, "permit-X11-forwarding");
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_AGENT_FWD) != 0)
-		add_flag_option(c, "permit-agent-forwarding");
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_PORT_FWD) != 0)
-		add_flag_option(c, "permit-port-forwarding");
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_PTY) != 0)
-		add_flag_option(c, "permit-pty");
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_USER_RC) != 0)
-		add_flag_option(c, "permit-user-rc");
-	if ((which & OPTIONS_EXTENSIONS) != 0 &&
-	    (certflags_flags & CERTOPT_NO_REQUIRE_USER_PRESENCE) != 0)
-		add_flag_option(c, "no-touch-required");
-	if ((which & OPTIONS_CRITICAL) != 0 &&
-	    certflags_src_addr != NULL)
-		add_string_option(c, "source-address", certflags_src_addr);
-	for (i = 0; i < ncert_userext; i++) {
-		if ((cert_userext[i].crit && (which & OPTIONS_EXTENSIONS)) ||
-		    (!cert_userext[i].crit && (which & OPTIONS_CRITICAL)))
+	for (i = 0; i < ncert_ext; i++) {
+		ext = &cert_ext[i];
+		if ((ext->crit && (which & OPTIONS_EXTENSIONS)) ||
+		    (!ext->crit && (which & OPTIONS_CRITICAL)))
 			continue;
-		if (cert_userext[i].val == NULL)
-			add_flag_option(c, cert_userext[i].key);
-		else {
-			add_string_option(c, cert_userext[i].key,
-			    cert_userext[i].val);
+		if (ext->val == NULL) {
+			/* flag option */
+			debug3("%s: %s", __func__, ext->key);
+			if ((r = sshbuf_put_cstring(c, ext->key)) != 0 ||
+			    (r = sshbuf_put_string(c, NULL, 0)) != 0)
+				fatal("%s: buffer: %s", __func__, ssh_err(r));
+		} else {
+			/* key/value option */
+			debug3("%s: %s=%s", __func__, ext->key, ext->val);
+			sshbuf_reset(b);
+			if ((r = sshbuf_put_cstring(c, ext->key)) != 0 ||
+			    (r = sshbuf_put_cstring(b, ext->val)) != 0 ||
+			    (r = sshbuf_put_stringb(c, b)) != 0)
+				fatal("%s: buffer: %s", __func__, ssh_err(r));
 		}
 	}
+	sshbuf_free(b);
+}
+
+static void
+finalise_cert_exts(void)
+{
+	/* critical options */
+	if (certflags_command != NULL)
+		cert_ext_add("force-command", certflags_command, 1);
+	if (certflags_src_addr != NULL)
+		cert_ext_add("source-address", certflags_src_addr, 1);
+	/* extensions */
+	if ((certflags_flags & CERTOPT_X_FWD) != 0)
+		cert_ext_add("permit-X11-forwarding", NULL, 0);
+	if ((certflags_flags & CERTOPT_AGENT_FWD) != 0)
+		cert_ext_add("permit-agent-forwarding", NULL, 0);
+	if ((certflags_flags & CERTOPT_PORT_FWD) != 0)
+		cert_ext_add("permit-port-forwarding", NULL, 0);
+	if ((certflags_flags & CERTOPT_PTY) != 0)
+		cert_ext_add("permit-pty", NULL, 0);
+	if ((certflags_flags & CERTOPT_USER_RC) != 0)
+		cert_ext_add("permit-user-rc", NULL, 0);
+	if ((certflags_flags & CERTOPT_NO_REQUIRE_USER_PRESENCE) != 0)
+		cert_ext_add("no-touch-required", NULL, 0);
+	/* order lexically by key */
+	if (ncert_ext > 0)
+		qsort(cert_ext, ncert_ext, sizeof(*cert_ext), cert_ext_cmp);
 }
 
 static struct sshkey *
@@ -1780,6 +1799,7 @@ do_ca_sign(struct passwd *pw, const char *ca_key_path, int prefer_agent,
 	}
 	ca_fp = sshkey_fingerprint(ca, fingerprint_hash, SSH_FP_DEFAULT);
 
+	finalise_cert_exts();
 	for (i = 0; i < argc; i++) {
 		/* Split list of principals */
 		n = 0;
@@ -1994,13 +2014,8 @@ add_cert_option(char *opt)
 		val = xstrdup(strchr(opt, ':') + 1);
 		if ((cp = strchr(val, '=')) != NULL)
 			*cp++ = '\0';
-		cert_userext = xreallocarray(cert_userext, ncert_userext + 1,
-		    sizeof(*cert_userext));
-		cert_userext[ncert_userext].key = val;
-		cert_userext[ncert_userext].val = cp == NULL ?
-		    NULL : xstrdup(cp);
-		cert_userext[ncert_userext].crit = iscrit;
-		ncert_userext++;
+		cert_ext_add(val, cp, iscrit);
+		free(val);
 	} else
 		fatal("Unsupported certificate option \"%s\"", opt);
 }
@@ -2008,7 +2023,7 @@ add_cert_option(char *opt)
 static void
 show_options(struct sshbuf *optbuf, int in_critical)
 {
-	char *name, *arg;
+	char *name, *arg, *hex;
 	struct sshbuf *options, *option = NULL;
 	int r;
 
@@ -2037,11 +2052,14 @@ show_options(struct sshbuf *optbuf, int in_critical)
 				    __func__, ssh_err(r));
 			printf(" %s\n", arg);
 			free(arg);
-		} else {
-			printf(" UNKNOWN OPTION (len %zu)\n",
-			    sshbuf_len(option));
+		} else if (sshbuf_len(option) > 0) {
+			hex = sshbuf_dtob16(option);
+			printf(" UNKNOWN OPTION: %s (len %zu)\n",
+			    hex, sshbuf_len(option));
 			sshbuf_reset(option);
-		}
+			free(hex);
+		} else
+			printf(" UNKNOWN FLAG OPTION\n");
 		free(name);
 		if (sshbuf_len(option) != 0)
 			fatal("Option corrupt: extra data at end");

-- 
To stop receiving notification emails like this one, please contact
djm at mindrot.org.


More information about the openssh-commits mailing list