Attachment 3434 Details for Bug 3198 – explicitly sort certificate extensions sections

[patch] explicitly sort certificate extensions sections

bz3198.diff (text/plain), 7.12 KB, created by Damien Miller on 2020-07-29 13:42:36 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2020-07-29 13:42:36 AEST

Size: 7.12 KB

patch

obsolete

>diff --git a/ssh-keygen.c b/ssh-keygen.c
>index 2bf461c..7d19418 100644
>--- a/ssh-keygen.c
>+++ b/ssh-keygen.c
>@@ -127,13 +127,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 {
>@@ -1579,31 +1579,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
>@@ -1611,44 +1612,59 @@ 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)
>+{
>+	if (certflags_command != NULL)
>+		cert_ext_add("force-command", certflags_command, 1);
>+	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);
>+	if (certflags_src_addr != NULL)
>+		cert_ext_add("source-address", certflags_src_addr, 0);
>+	if (ncert_ext > 0)
>+		qsort(cert_ext, ncert_ext, sizeof(*cert_ext), cert_ext_cmp);
> }
> 
> static struct sshkey *
>@@ -1765,6 +1781,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;
>@@ -1981,13 +1998,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);
> }
>@@ -1995,7 +2007,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;
> 
>@@ -2024,11 +2036,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");

Flags:

dtucker: ok+

Actions: View | Diff

Attachments on bug 3198: 3434