- djm@cvs.openbsd.org 2010/05/20 23:46:02

[PROTOCOL.certkeys auth-options.c ssh-keygen.c]
     Move the permit-* options to the non-critical "extensions" field for v01
     certificates. The logic is that if another implementation fails to
     implement them then the connection just loses features rather than fails
     outright.

     ok markus@

ChangeLog

@@ -33,6 +33,14 @@
      [auth2-pubkey.c]
      fix logspam when key options (from="..." especially) deny non-matching
      keys; reported by henning@ also bz#1765; ok markus@ dtucker@
+   - djm@cvs.openbsd.org 2010/05/20 23:46:02
+     [PROTOCOL.certkeys auth-options.c ssh-keygen.c]
+     Move the permit-* options to the non-critical "extensions" field for v01
+     certificates. The logic is that if another implementation fails to
+     implement them then the connection just loses features rather than fails
+     outright.
+     
+     ok markus@
 
 20100511
  - (dtucker) [Makefile.in] Bug #1770: Link libopenbsd-compat twice to solve

PROTOCOL.certkeys

@@ -131,7 +131,7 @@ must refuse to authorise a key that has an unrecognised option.
 
 extensions is a set of zero or more optional extensions. These extensions
 are not critical, and an implementation that encounters one that it does
-not recognise may safely ignore it. No extensions are defined at present.
+not recognise may safely ignore it.
 
 The reserved field is currently unused and is ignored in this version of
 the protocol.
@@ -172,6 +172,28 @@ force-command           string        Specifies a command that is executed
                                       ssh command-line) whenever this key is
                                       used for authentication.
 
+source-address          string        Comma-separated list of source addresses
+                                      from which this certificate is accepted
+                                      for authentication. Addresses are
+                                      specified in CIDR format (nn.nn.nn.nn/nn
+                                      or hhhh::hhhh/nn).
+                                      If this option is not present then
+                                      certificates may be presented from any
+                                      source address.
+
+Extensions
+----------
+
+The extensions section of the certificate specifies zero or more
+non-critical certificate extensions. The encoding of extensions in this
+field is identical to that of the critical options. If an implementation
+does not recognise an extension, then it should ignore it.
+
+The supported extensions and the contents and structure of their data
+fields are:
+
+Name                    Format        Description
+-----------------------------------------------------------------------------
 permit-X11-forwarding   empty         Flag indicating that X11 forwarding
                                       should be permitted. X11 forwarding will
                                       be refused if this option is absent.
@@ -196,13 +218,4 @@ permit-user-rc          empty         Flag indicating that execution of
                                       of this script will not be permitted if
                                       this option is not present.
 
-source-address          string        Comma-separated list of source addresses
+$OpenBSD: PROTOCOL.certkeys,v 1.6 2010/05/20 23:46:02 djm Exp $
-                                      from which this certificate is accepted
-                                      for authentication. Addresses are
-                                      specified in CIDR format (nn.nn.nn.nn/nn
-                                      or hhhh::hhhh/nn).
-                                      If this option is not present then
-                                      certificates may be presented from any
-                                      source address.
-
-$OpenBSD: PROTOCOL.certkeys,v 1.5 2010/05/01 02:50:50 djm Exp $

auth-options.c

@@ -1,4 +1,4 @@
-/* $OpenBSD: auth-options.c,v 1.51 2010/05/07 11:30:29 djm Exp $ */
+/* $OpenBSD: auth-options.c,v 1.52 2010/05/20 23:46:02 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -417,32 +417,31 @@ bad_option:
 	return 0;
 }
 
-/*
+#define OPTIONS_CRITICAL	1
- * Set options from critical certificate options. These supersede user key
+#define OPTIONS_EXTENSIONS	2
- * options so this must be called after auth_parse_options().
+static int
- */
+parse_option_list(u_char *optblob, size_t optblob_len, struct passwd *pw,
-int
+    u_int which, int crit,
-auth_cert_options(Key *k, struct passwd *pw)
+    int *cert_no_port_forwarding_flag,
+    int *cert_no_agent_forwarding_flag,
+    int *cert_no_x11_forwarding_flag,
+    int *cert_no_pty_flag,
+    int *cert_no_user_rc,
+    char **cert_forced_command,
+    int *cert_source_address_done)
 {
+	char *command, *allowed;
+	const char *remote_ip;
 	u_char *name = NULL, *data_blob = NULL;
 	u_int nlen, dlen, clen;
 	Buffer c, data;
-	int ret = -1;
+	int ret = -1, found;
-
-	int cert_no_port_forwarding_flag = 1;
-	int cert_no_agent_forwarding_flag = 1;
-	int cert_no_x11_forwarding_flag = 1;
-	int cert_no_pty_flag = 1;
-	int cert_no_user_rc = 1;
-	char *cert_forced_command = NULL;
-	int cert_source_address_done = 0;
 
 	buffer_init(&data);
 
 	/* Make copy to avoid altering original */
 	buffer_init(&c);
-	buffer_append(&c,
+	buffer_append(&c, optblob, optblob_len);
-	    buffer_ptr(&k->cert->critical), buffer_len(&k->cert->critical));
 
 	while (buffer_len(&c) > 0) {
 		if ((name = buffer_get_string_ret(&c, &nlen)) == NULL ||
@@ -451,90 +450,114 @@ auth_cert_options(Key *k, struct passwd *pw)
 			goto out;
 		}
 		buffer_append(&data, data_blob, dlen);
-		debug3("found certificate constraint \"%.100s\" len %u",
+		debug3("found certificate option \"%.100s\" len %u",
 		    name, dlen);
 		if (strlen(name) != nlen) {
 			error("Certificate constraint name contains \\0");
 			goto out;
 		}
-		if (strcmp(name, "permit-X11-forwarding") == 0)
+		found = 0;
-			cert_no_x11_forwarding_flag = 0;
+		if ((which & OPTIONS_EXTENSIONS) != 0) {
-		else if (strcmp(name, "permit-agent-forwarding") == 0)
+			if (strcmp(name, "permit-X11-forwarding") == 0) {
-			cert_no_agent_forwarding_flag = 0;
+				*cert_no_x11_forwarding_flag = 0;
-		else if (strcmp(name, "permit-port-forwarding") == 0)
+				found = 1;
-			cert_no_port_forwarding_flag = 0;
+			} else if (strcmp(name,
-		else if (strcmp(name, "permit-pty") == 0)
+			    "permit-agent-forwarding") == 0) {
-			cert_no_pty_flag = 0;
+				*cert_no_agent_forwarding_flag = 0;
-		else if (strcmp(name, "permit-user-rc") == 0)
+				found = 1;
-			cert_no_user_rc = 0;
+			} else if (strcmp(name,
-		else if (strcmp(name, "force-command") == 0) {
+			    "permit-port-forwarding") == 0) {
-			char *command = buffer_get_string_ret(&data, &clen);
+				*cert_no_port_forwarding_flag = 0;
-
+				found = 1;
-			if (command == NULL) {
+			} else if (strcmp(name, "permit-pty") == 0) {
-				error("Certificate constraint \"%s\" corrupt",
+				*cert_no_pty_flag = 0;
-				    name);
+				found = 1;
-				goto out;
+			} else if (strcmp(name, "permit-user-rc") == 0) {
+				*cert_no_user_rc = 0;
+				found = 1;
 			}
-			if (strlen(command) != clen) {
+		}
-				error("force-command constraint contains \\0");
+		if (!found && (which & OPTIONS_CRITICAL) != 0) {
-				goto out;
+			if (strcmp(name, "force-command") == 0) {
+				if ((command = buffer_get_string_ret(&data,
+				    &clen)) == NULL) {
+					error("Certificate constraint \"%s\" "
+					    "corrupt", name);
+					goto out;
+				}
+				if (strlen(command) != clen) {
+					error("force-command constraint "
+					    "contains \\0");
+					goto out;
+				}
+				if (*cert_forced_command != NULL) {
+					error("Certificate has multiple "
+					    "force-command options");
+					xfree(command);
+					goto out;
+				}
+				*cert_forced_command = command;
+				found = 1;
 			}
-			if (cert_forced_command != NULL) {
+			if (strcmp(name, "source-address") == 0) {
-				error("Certificate has multiple "
+				if ((allowed = buffer_get_string_ret(&data,
-				    "force-command options");
+				    &clen)) == NULL) {
-				xfree(command);
+					error("Certificate constraint "
-				goto out;
+					    "\"%s\" corrupt", name);
+					goto out;
+				}
+				if (strlen(allowed) != clen) {
+					error("source-address constraint "
+					    "contains \\0");
+					goto out;
+				}
+				if ((*cert_source_address_done)++) {
+					error("Certificate has multiple "
+					    "source-address options");
+					xfree(allowed);
+					goto out;
+				}
+				remote_ip = get_remote_ipaddr();
+				switch (addr_match_cidr_list(remote_ip,
+				    allowed)) {
+				case 1:
+					/* accepted */
+					xfree(allowed);
+					break;
+				case 0:
+					/* no match */
+					logit("Authentication tried for %.100s "
+					    "with valid certificate but not "
+					    "from a permitted host "
+					    "(ip=%.200s).", pw->pw_name,
+					    remote_ip);
+					auth_debug_add("Your address '%.200s' "
+					    "is not permitted to use this "
+					    "certificate for login.",
+					    remote_ip);
+					xfree(allowed);
+					goto out;
+				case -1:
+					error("Certificate source-address "
+					    "contents invalid");
+					xfree(allowed);
+					goto out;
+				}
+				found = 1;
 			}
-			cert_forced_command = command;
-		} else if (strcmp(name, "source-address") == 0) {
-			char *allowed = buffer_get_string_ret(&data, &clen);
-			const char *remote_ip = get_remote_ipaddr();
-			
-			if (allowed == NULL) {
-				error("Certificate constraint \"%s\" corrupt",
-				    name);
-				goto out;
-			}
-			if (strlen(allowed) != clen) {
-				error("source-address constraint contains \\0");
-				goto out;
-			}
-			if (cert_source_address_done++) {
-				error("Certificate has multiple "
-				    "source-address options");
-				xfree(allowed);
-				goto out;
-			}
-			switch (addr_match_cidr_list(remote_ip, allowed)) {
-			case 1:
-				/* accepted */
-				xfree(allowed);
-				break;
-			case 0:
-				/* no match */
-				logit("Authentication tried for %.100s with "
-				    "valid certificate but not from a "
-				    "permitted host (ip=%.200s).",
-				    pw->pw_name, remote_ip);
-				auth_debug_add("Your address '%.200s' is not "
-				    "permitted to use this certificate for "
-				    "login.", remote_ip);
-				xfree(allowed);
-				goto out;
-			case -1:
-				error("Certificate source-address contents "
-				    "invalid");
-				xfree(allowed);
-				goto out;
-			}
-		} else {
-			error("Certificate constraint \"%s\" is not supported",
-			    name);
-			goto out;
 		}
 
-		if (buffer_len(&data) != 0) {
+		if (!found) {
-			error("Certificate constraint \"%s\" corrupt "
+			if (crit) {
+				error("Certificate critical option \"%s\" "
+				    "is not supported", name);
+				goto out;
+			} else {
+				logit("Certificate extension \"%s\" "
+				    "is not supported", name);
+			}
+		} else if (buffer_len(&data) != 0) {
+			error("Certificate option \"%s\" corrupt "
 			    "(extra data)", name);
 			goto out;
 		}
@@ -543,10 +566,73 @@ auth_cert_options(Key *k, struct passwd *pw)
 		xfree(data_blob);
 		name = data_blob = NULL;
 	}
-
 	/* successfully parsed all options */
 	ret = 0;
 
+ out:
+	if (ret != 0 &&
+	    cert_forced_command != NULL &&
+	    *cert_forced_command != NULL) {
+		xfree(*cert_forced_command);
+		*cert_forced_command = NULL;
+	}
+	if (name != NULL)
+		xfree(name);
+	if (data_blob != NULL)
+		xfree(data_blob);
+	buffer_free(&data);
+	buffer_free(&c);
+	return ret;
+}
+
+/*
+ * Set options from critical certificate options. These supersede user key
+ * options so this must be called after auth_parse_options().
+ */
+int
+auth_cert_options(Key *k, struct passwd *pw)
+{
+	int cert_no_port_forwarding_flag = 1;
+	int cert_no_agent_forwarding_flag = 1;
+	int cert_no_x11_forwarding_flag = 1;
+	int cert_no_pty_flag = 1;
+	int cert_no_user_rc = 1;
+	char *cert_forced_command = NULL;
+	int cert_source_address_done = 0;
+
+	if (key_cert_is_legacy(k)) {
+		/* All options are in the one field for v00 certs */
+		if (parse_option_list(buffer_ptr(&k->cert->critical),
+		    buffer_len(&k->cert->critical), pw,
+		    OPTIONS_CRITICAL|OPTIONS_EXTENSIONS, 1,
+		    &cert_no_port_forwarding_flag,
+		    &cert_no_agent_forwarding_flag,
+		    &cert_no_x11_forwarding_flag,
+		    &cert_no_pty_flag,
+		    &cert_no_user_rc,
+		    &cert_forced_command,
+		    &cert_source_address_done) == -1)
+			return -1;
+	} else {
+		/* Separate options and extensions for v01 certs */
+		if (parse_option_list(buffer_ptr(&k->cert->critical),
+		    buffer_len(&k->cert->critical), pw,
+		    OPTIONS_CRITICAL, 1, NULL, NULL, NULL, NULL, NULL,
+		    &cert_forced_command,
+		    &cert_source_address_done) == -1)
+			return -1;
+		if (parse_option_list(buffer_ptr(&k->cert->extensions),
+		    buffer_len(&k->cert->extensions), pw,
+		    OPTIONS_EXTENSIONS, 1,
+		    &cert_no_port_forwarding_flag,
+		    &cert_no_agent_forwarding_flag,
+		    &cert_no_x11_forwarding_flag,
+		    &cert_no_pty_flag,
+		    &cert_no_user_rc,
+		    NULL, NULL) == -1)
+			return -1;
+	}
+
 	no_port_forwarding_flag |= cert_no_port_forwarding_flag;
 	no_agent_forwarding_flag |= cert_no_agent_forwarding_flag;
 	no_x11_forwarding_flag |= cert_no_x11_forwarding_flag;
@@ -558,14 +644,6 @@ auth_cert_options(Key *k, struct passwd *pw)
 			xfree(forced_command);
 		forced_command = cert_forced_command;
 	}
-	
+	return 0;
- out:
-	if (name != NULL)
-		xfree(name);
-	if (data_blob != NULL)
-		xfree(data_blob);
-	buffer_free(&data);
-	buffer_free(&c);
-	return ret;
 }
 

ssh-keygen.c

@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.189 2010/04/23 22:48:31 djm Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.190 2010/05/20 23:46:02 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -122,17 +122,16 @@ u_int64_t cert_valid_from = 0;
 u_int64_t cert_valid_to = ~0ULL;
 
 /* Certificate options */
-#define CRITOPT_X_FWD	(1)
+#define CERTOPT_X_FWD	(1)
-#define CRITOPT_AGENT_FWD	(1<<1)
+#define CERTOPT_AGENT_FWD	(1<<1)
-#define CRITOPT_PORT_FWD	(1<<2)
+#define CERTOPT_PORT_FWD	(1<<2)
-#define CRITOPT_PTY		(1<<3)
+#define CERTOPT_PTY		(1<<3)
-#define CRITOPT_USER_RC	(1<<4)
+#define CERTOPT_USER_RC	(1<<4)
-#define CRITOPT_DEFAULT	(CRITOPT_X_FWD|CRITOPT_AGENT_FWD| \
+#define CERTOPT_DEFAULT	(CERTOPT_X_FWD|CERTOPT_AGENT_FWD| \
-				CRITOPT_PORT_FWD|CRITOPT_PTY| \
+			 CERTOPT_PORT_FWD|CERTOPT_PTY|CERTOPT_USER_RC)
-				CRITOPT_USER_RC)
+u_int32_t certflags_flags = CERTOPT_DEFAULT;
-u_int32_t critical_flags = CRITOPT_DEFAULT;
+char *certflags_command = NULL;
-char *critical_command = NULL;
+char *certflags_src_addr = NULL;
-char *critical_src_addr = NULL;
 
 /* Dump public key file in format used by real and the original SSH 2 */
 int convert_to_ssh2 = 0;
@@ -1133,24 +1132,33 @@ add_string_option(Buffer *c, const char *name, const char *value)
 	buffer_free(&b);
 }
 
+#define OPTIONS_CRITICAL	1
+#define OPTIONS_EXTENSIONS	2
 static void
-prepare_options_buf(Buffer *c)
+prepare_options_buf(Buffer *c, int which)
 {
 	buffer_clear(c);
-	if ((critical_flags & CRITOPT_X_FWD) != 0)
+	if ((which & OPTIONS_EXTENSIONS) != 0 &&
+	    (certflags_flags & CERTOPT_X_FWD) != 0)
 		add_flag_option(c, "permit-X11-forwarding");
-	if ((critical_flags & CRITOPT_AGENT_FWD) != 0)
+	if ((which & OPTIONS_EXTENSIONS) != 0 &&
+	    (certflags_flags & CERTOPT_AGENT_FWD) != 0)
 		add_flag_option(c, "permit-agent-forwarding");
-	if ((critical_flags & CRITOPT_PORT_FWD) != 0)
+	if ((which & OPTIONS_EXTENSIONS) != 0 &&
+	    (certflags_flags & CERTOPT_PORT_FWD) != 0)
 		add_flag_option(c, "permit-port-forwarding");
-	if ((critical_flags & CRITOPT_PTY) != 0)
+	if ((which & OPTIONS_EXTENSIONS) != 0 &&
+	    (certflags_flags & CERTOPT_PTY) != 0)
 		add_flag_option(c, "permit-pty");
-	if ((critical_flags & CRITOPT_USER_RC) != 0)
+	if ((which & OPTIONS_EXTENSIONS) != 0 &&
+	    (certflags_flags & CERTOPT_USER_RC) != 0)
 		add_flag_option(c, "permit-user-rc");
-	if (critical_command != NULL)
+	if ((which & OPTIONS_CRITICAL) != 0 &&
-		add_string_option(c, "force-command", critical_command);
+	    certflags_command != NULL)
-	if (critical_src_addr != NULL)
+		add_string_option(c, "force-command", certflags_command);
-		add_string_option(c, "source-address", critical_src_addr);
+	if ((which & OPTIONS_CRITICAL) != 0 &&
+	    certflags_src_addr != NULL)
+		add_string_option(c, "source-address", certflags_src_addr);
 }
 
 static void
@@ -1218,7 +1226,15 @@ do_ca_sign(struct passwd *pw, int argc, char **argv)
 		public->cert->principals = plist;
 		public->cert->valid_after = cert_valid_from;
 		public->cert->valid_before = cert_valid_to;
-		prepare_options_buf(&public->cert->critical);
+		if (v00) {
+			prepare_options_buf(&public->cert->critical,
+			    OPTIONS_CRITICAL|OPTIONS_EXTENSIONS);
+		} else {
+			prepare_options_buf(&public->cert->critical,
+			    OPTIONS_CRITICAL);
+			prepare_options_buf(&public->cert->extensions,
+			    OPTIONS_EXTENSIONS);
+		}
 		public->cert->signature_key = key_from_private(ca);
 
 		if (key_certify(public, ca) != 0)
@@ -1354,43 +1370,43 @@ add_cert_option(char *opt)
 	char *val;
 
 	if (strcmp(opt, "clear") == 0)
-		critical_flags = 0;
+		certflags_flags = 0;
 	else if (strcasecmp(opt, "no-x11-forwarding") == 0)
-		critical_flags &= ~CRITOPT_X_FWD;
+		certflags_flags &= ~CERTOPT_X_FWD;
 	else if (strcasecmp(opt, "permit-x11-forwarding") == 0)
-		critical_flags |= CRITOPT_X_FWD;
+		certflags_flags |= CERTOPT_X_FWD;
 	else if (strcasecmp(opt, "no-agent-forwarding") == 0)
-		critical_flags &= ~CRITOPT_AGENT_FWD;
+		certflags_flags &= ~CERTOPT_AGENT_FWD;
 	else if (strcasecmp(opt, "permit-agent-forwarding") == 0)
-		critical_flags |= CRITOPT_AGENT_FWD;
+		certflags_flags |= CERTOPT_AGENT_FWD;
 	else if (strcasecmp(opt, "no-port-forwarding") == 0)
-		critical_flags &= ~CRITOPT_PORT_FWD;
+		certflags_flags &= ~CERTOPT_PORT_FWD;
 	else if (strcasecmp(opt, "permit-port-forwarding") == 0)
-		critical_flags |= CRITOPT_PORT_FWD;
+		certflags_flags |= CERTOPT_PORT_FWD;
 	else if (strcasecmp(opt, "no-pty") == 0)
-		critical_flags &= ~CRITOPT_PTY;
+		certflags_flags &= ~CERTOPT_PTY;
 	else if (strcasecmp(opt, "permit-pty") == 0)
-		critical_flags |= CRITOPT_PTY;
+		certflags_flags |= CERTOPT_PTY;
 	else if (strcasecmp(opt, "no-user-rc") == 0)
-		critical_flags &= ~CRITOPT_USER_RC;
+		certflags_flags &= ~CERTOPT_USER_RC;
 	else if (strcasecmp(opt, "permit-user-rc") == 0)
-		critical_flags |= CRITOPT_USER_RC;
+		certflags_flags |= CERTOPT_USER_RC;
 	else if (strncasecmp(opt, "force-command=", 14) == 0) {
 		val = opt + 14;
 		if (*val == '\0')
 			fatal("Empty force-command option");
-		if (critical_command != NULL)
+		if (certflags_command != NULL)
 			fatal("force-command already specified");
-		critical_command = xstrdup(val);
+		certflags_command = xstrdup(val);
 	} else if (strncasecmp(opt, "source-address=", 15) == 0) {
 		val = opt + 15;
 		if (*val == '\0')
 			fatal("Empty source-address option");
-		if (critical_src_addr != NULL)
+		if (certflags_src_addr != NULL)
 			fatal("source-address already specified");
 		if (addr_match_cidr_list(NULL, val) != 0)
 			fatal("Invalid source-address list");
-		critical_src_addr = xstrdup(val);
+		certflags_src_addr = xstrdup(val);
 	} else
 		fatal("Unsupported certificate option \"%s\"", opt);
 }
@@ -1667,7 +1683,7 @@ main(int argc, char **argv)
 			break;
 		case 'h':
 			cert_key_type = SSH2_CERT_TYPE_HOST;
-			critical_flags = 0;
+			certflags_flags = 0;
 			break;
 		case 'i':
 		case 'X':