reject strings with embedded ASCII nul chars in certificate key IDs,

principal names and constraints

2 files changed, 43 insertions, 21 deletions

diff --git a/usr.bin/ssh/auth-options.c b/usr.bin/ssh/auth-options.c
index e765b5ad684..97776bac42d 100644
--- a/usr.bin/ssh/auth-options.c
+++ b/usr.bin/ssh/auth-options.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth-options.c,v 1.45 2010/02/26 20:29:54 djm Exp $ */
+/* $OpenBSD: auth-options.c,v 1.46 2010/03/03 01:44:36 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -389,7 +389,7 @@ int
 auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 {
 	u_char *name = NULL, *data_blob = NULL;
-	u_int len;
+	u_int nlen, dlen, clen;
 	Buffer c, data;
 	int ret = -1;
 
@@ -408,14 +408,18 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 	buffer_append(&c, buffer_ptr(c_orig), buffer_len(c_orig));
 
 	while (buffer_len(&c) > 0) {
-		if ((name = buffer_get_string_ret(&c, NULL)) == NULL ||
-		    (data_blob = buffer_get_string_ret(&c, &len)) == NULL) {
+		if ((name = buffer_get_string_ret(&c, &nlen)) == NULL ||
+		    (data_blob = buffer_get_string_ret(&c, &dlen)) == NULL) {
 			error("Certificate constraints corrupt");
 			goto out;
 		}
-		buffer_append(&data, data_blob, len);
+		buffer_append(&data, data_blob, dlen);
 		debug3("found certificate constraint \"%.100s\" len %u",
-		    name, len);
+		    name, dlen);
+		if (strlen(name) != nlen) {
+			error("Certificate constraint name contains \\0");
+			goto out;
+		}
 		if (strcmp(name, "permit-X11-forwarding") == 0)
 			cert_no_x11_forwarding_flag = 0;
 		else if (strcmp(name, "permit-agent-forwarding") == 0)
@@ -427,13 +431,17 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 		else if (strcmp(name, "permit-user-rc") == 0)
 			cert_no_user_rc = 0;
 		else if (strcmp(name, "force-command") == 0) {
-			char *command = buffer_get_string_ret(&data, NULL);
+			char *command = buffer_get_string_ret(&data, &clen);
 
 			if (command == NULL) {
 				error("Certificate constraint \"%s\" corrupt",
 				    name);
 				goto out;
 			}
+			if (strlen(command) != clen) {
+				error("force-command constrain contains \\0");
+				goto out;
+			}
 			if (cert_forced_command != NULL) {
 				error("Certificate has multiple "
 				    "forced-command constraints");
@@ -442,7 +450,7 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 			}
 			cert_forced_command = command;
 		} else if (strcmp(name, "source-address") == 0) {
-			char *allowed = buffer_get_string_ret(&data, NULL);
+			char *allowed = buffer_get_string_ret(&data, &clen);
 			const char *remote_ip = get_remote_ipaddr();
 			
 			if (allowed == NULL) {
@@ -450,6 +458,10 @@ auth_cert_constraints(Buffer *c_orig, struct passwd *pw)
 				    name);
 				goto out;
 			}
+			if (strlen(allowed) != clen) {
+				error("source-address constrain contains \\0");
+				goto out;
+			}
 			if (cert_source_address_done++) {
 				error("Certificate has multiple "
 				    "source-address constraints");
diff --git a/usr.bin/ssh/key.c b/usr.bin/ssh/key.c
index 509e1071196..aaf683b04bd 100644
--- a/usr.bin/ssh/key.c
+++ b/usr.bin/ssh/key.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.c,v 1.83 2010/02/26 20:29:54 djm Exp $ */
+/* $OpenBSD: key.c,v 1.84 2010/03/03 01:44:36 djm Exp $ */
 /*
  * read_bignum():
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -996,7 +996,7 @@ static int
 cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 {
 	u_char *principals, *constraints, *sig_key, *sig;
-	u_int signed_len, plen, clen, sklen, slen;
+	u_int signed_len, plen, clen, sklen, slen, kidlen;
 	Buffer tmp;
 	char *principal;
 	int ret = -1;
@@ -1008,7 +1008,7 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 
 	principals = constraints = sig_key = sig = NULL;
 	if (buffer_get_int_ret(&key->cert->type, b) != 0 ||
-	    (key->cert->key_id = buffer_get_string_ret(b, NULL)) == NULL ||
+	    (key->cert->key_id = buffer_get_string_ret(b, &kidlen)) == NULL ||
 	    (principals = buffer_get_string_ret(b, &plen)) == NULL ||
 	    buffer_get_int64_ret(&key->cert->valid_after, b) != 0 ||
 	    buffer_get_int64_ret(&key->cert->valid_before, b) != 0 ||
@@ -1020,6 +1020,11 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 		goto out;
 	}
 
+	if (kidlen != strlen(key->cert->key_id)) {
+		error("%s: key ID contains \\0 character", __func__);
+		goto out;
+	}
+
 	/* Signature is left in the buffer so we can calculate this length */
 	signed_len = buffer_len(&key->cert->certblob) - buffer_len(b);
 
@@ -1037,11 +1042,16 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 	buffer_append(&tmp, principals, plen);
 	while (buffer_len(&tmp) > 0) {
 		if (key->cert->nprincipals >= CERT_MAX_PRINCIPALS) {
-			error("Too many principals");
+			error("%s: Too many principals", __func__);
 			goto out;
 		}
-		if ((principal = buffer_get_string_ret(&tmp, NULL)) == NULL) {
-			error("Principals data invalid");
+		if ((principal = buffer_get_string_ret(&tmp, &plen)) == NULL) {
+			error("%s: Principals data invalid", __func__);
+			goto out;
+		}
+		if (strlen(principal) != plen) {
+			error("%s: Principal contains \\0 character",
+			    __func__);
 			goto out;
 		}
 		key->cert->principals = xrealloc(key->cert->principals,
@@ -1057,7 +1067,7 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 	while (buffer_len(&tmp) != 0) {
 		if (buffer_get_string_ptr(&tmp, NULL) == NULL ||
 		    buffer_get_string_ptr(&tmp, NULL) == NULL) {
-			error("Constraints data invalid");
+			error("%s: Constraints data invalid", __func__);
 			goto out;
 		}
 	}
@@ -1065,12 +1075,12 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 
 	if ((key->cert->signature_key = key_from_blob(sig_key,
 	    sklen)) == NULL) {
-		error("Signature key invalid");
+		error("%s: Signature key invalid", __func__);
 		goto out;
 	}
 	if (key->cert->signature_key->type != KEY_RSA &&
 	    key->cert->signature_key->type != KEY_DSA) {
-		error("Invalid signature key type %s (%d)",
+		error("%s: Invalid signature key type %s (%d)", __func__,
 		    key_type(key->cert->signature_key),
 		    key->cert->signature_key->type);
 		goto out;
@@ -1079,17 +1089,17 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 	switch (key_verify(key->cert->signature_key, sig, slen, 
 	    buffer_ptr(&key->cert->certblob), signed_len)) {
 	case 1:
+		ret = 0;
 		break; /* Good signature */
 	case 0:
-		error("Invalid signature on certificate");
+		error("%s: Invalid signature on certificate", __func__);
 		goto out;
 	case -1:
-		error("Certificate signature verification failed");
+		error("%s: Certificate signature verification failed",
+		    __func__);
 		goto out;
 	}
 
-	ret = 0;
-
  out:
 	buffer_free(&tmp);
 	if (principals != NULL)