Add buffer_get_cstring() and related functions that verify that the

string extracted from the buffer contains no embedded \0 characters*
This prevents random (possibly malicious) crap from being appended to
strings where it would not be noticed if the string is used with
a string(3) function.

Use the new API in a few sensitive places.

* actually, we allow a single one at the end of the string for now because
we don't know how many deployed implementations get this wrong, but don't
count on this to remain indefinitely.

11 files changed, 69 insertions, 31 deletions

diff --git a/usr.bin/ssh/auth-options.c b/usr.bin/ssh/auth-options.c
index 5cfe9ebd533..89c40f6000a 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.52 2010/05/20 23:46:02 djm Exp $ */
+/* $OpenBSD: auth-options.c,v 1.53 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -442,7 +442,7 @@ parse_option_list(u_char *optblob, size_t optblob_len, struct passwd *pw,
 	buffer_append(&c, optblob, optblob_len);
 
 	while (buffer_len(&c) > 0) {
-		if ((name = buffer_get_string_ret(&c, &nlen)) == NULL ||
+		if ((name = buffer_get_cstring_ret(&c, &nlen)) == NULL ||
 		    (data_blob = buffer_get_string_ret(&c, &dlen)) == NULL) {
 			error("Certificate options corrupt");
 			goto out;
@@ -477,7 +477,7 @@ parse_option_list(u_char *optblob, size_t optblob_len, struct passwd *pw,
 		}
 		if (!found && (which & OPTIONS_CRITICAL) != 0) {
 			if (strcmp(name, "force-command") == 0) {
-				if ((command = buffer_get_string_ret(&data,
+				if ((command = buffer_get_cstring_ret(&data,
 				    &clen)) == NULL) {
 					error("Certificate constraint \"%s\" "
 					    "corrupt", name);
@@ -498,7 +498,7 @@ parse_option_list(u_char *optblob, size_t optblob_len, struct passwd *pw,
 				found = 1;
 			}
 			if (strcmp(name, "source-address") == 0) {
-				if ((allowed = buffer_get_string_ret(&data,
+				if ((allowed = buffer_get_cstring_ret(&data,
 				    &clen)) == NULL) {
 					error("Certificate constraint "
 					    "\"%s\" corrupt", name);
diff --git a/usr.bin/ssh/auth1.c b/usr.bin/ssh/auth1.c
index 73361042084..63fecc19e92 100644
--- a/usr.bin/ssh/auth1.c
+++ b/usr.bin/ssh/auth1.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth1.c,v 1.74 2010/06/25 08:46:17 djm Exp $ */
+/* $OpenBSD: auth1.c,v 1.75 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
  *                    All rights reserved
@@ -161,7 +161,7 @@ auth1_process_rhosts_rsa(Authctxt *authctxt, char *info, size_t infolen)
 	 * trust the client; root on the client machine can
 	 * claim to be any user.
 	 */
-	client_user = packet_get_string(&ulen);
+	client_user = packet_get_cstring(&ulen);
 
 	/* Get the client host key. */
 	client_host_key = key_new(KEY_RSA1);
@@ -321,7 +321,7 @@ do_authentication(Authctxt *authctxt)
 	packet_read_expect(SSH_CMSG_USER);
 
 	/* Get the user name. */
-	user = packet_get_string(&ulen);
+	user = packet_get_cstring(&ulen);
 	packet_check_eom();
 
 	if ((style = strchr(user, ':')) != NULL)
diff --git a/usr.bin/ssh/auth2.c b/usr.bin/ssh/auth2.c
index c8d2f4efa2d..b197315f60f 100644
--- a/usr.bin/ssh/auth2.c
+++ b/usr.bin/ssh/auth2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth2.c,v 1.121 2009/06/22 05:39:28 dtucker Exp $ */
+/* $OpenBSD: auth2.c,v 1.122 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  *
@@ -169,7 +169,7 @@ input_service_request(int type, u_int32_t seq, void *ctxt)
 	Authctxt *authctxt = ctxt;
 	u_int len;
 	int acceptit = 0;
-	char *service = packet_get_string(&len);
+	char *service = packet_get_cstring(&len);
 	packet_check_eom();
 
 	if (authctxt == NULL)
@@ -208,9 +208,9 @@ input_userauth_request(int type, u_int32_t seq, void *ctxt)
 	if (authctxt == NULL)
 		fatal("input_userauth_request: no authctxt");
 
-	user = packet_get_string(NULL);
-	service = packet_get_string(NULL);
-	method = packet_get_string(NULL);
+	user = packet_get_cstring(NULL);
+	service = packet_get_cstring(NULL);
+	method = packet_get_cstring(NULL);
 	debug("userauth-request for user %s service %s method %s", user, service, method);
 	debug("attempt %d failures %d", authctxt->attempt, authctxt->failures);
 
diff --git a/usr.bin/ssh/bufaux.c b/usr.bin/ssh/bufaux.c
index 8a889611714..f5a5d881222 100644
--- a/usr.bin/ssh/bufaux.c
+++ b/usr.bin/ssh/bufaux.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: bufaux.c,v 1.49 2010/03/26 03:13:17 djm Exp $ */
+/* $OpenBSD: bufaux.c,v 1.50 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -200,6 +200,39 @@ buffer_get_string(Buffer *buffer, u_int *length_ptr)
 	return (ret);
 }
 
+char *
+buffer_get_cstring_ret(Buffer *buffer, u_int *length_ptr)
+{
+	u_int length;
+	char *cp, *ret = buffer_get_string_ret(buffer, &length);
+
+	if (ret == NULL)
+		return NULL;
+	if ((cp = memchr(ret, '\0', length)) != NULL) {
+		/* XXX allow \0 at end-of-string for a while, remove later */
+		if (cp == ret + length - 1)
+			error("buffer_get_cstring_ret: string contains \\0");
+		else {
+			bzero(ret, length);
+			xfree(ret);
+			return NULL;
+		}
+	}
+	if (length_ptr != NULL)
+		*length_ptr = length;
+	return ret;
+}
+
+char *
+buffer_get_cstring(Buffer *buffer, u_int *length_ptr)
+{
+	char *ret;
+
+	if ((ret = buffer_get_cstring_ret(buffer, length_ptr)) == NULL)
+		fatal("buffer_get_cstring: buffer error");
+	return ret;
+}
+
 void *
 buffer_get_string_ptr_ret(Buffer *buffer, u_int *length_ptr)
 {
diff --git a/usr.bin/ssh/buffer.h b/usr.bin/ssh/buffer.h
index 4ef4f80b35a..93baae2c820 100644
--- a/usr.bin/ssh/buffer.h
+++ b/usr.bin/ssh/buffer.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: buffer.h,v 1.19 2010/02/09 03:56:28 djm Exp $ */
+/* $OpenBSD: buffer.h,v 1.20 2010/08/31 09:58:37 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -68,6 +68,7 @@ void    buffer_put_char(Buffer *, int);
 void   *buffer_get_string(Buffer *, u_int *);
 void   *buffer_get_string_ptr(Buffer *, u_int *);
 void    buffer_put_string(Buffer *, const void *, u_int);
+char   *buffer_get_cstring(Buffer *, u_int *);
 void	buffer_put_cstring(Buffer *, const char *);
 
 #define buffer_skip_string(b) \
@@ -81,6 +82,7 @@ int	buffer_get_short_ret(u_short *, Buffer *);
 int	buffer_get_int_ret(u_int *, Buffer *);
 int	buffer_get_int64_ret(u_int64_t *, Buffer *);
 void	*buffer_get_string_ret(Buffer *, u_int *);
+char	*buffer_get_cstring_ret(Buffer *, u_int *);
 void	*buffer_get_string_ptr_ret(Buffer *, u_int *);
 int	buffer_get_char_ret(char *, Buffer *);
 
diff --git a/usr.bin/ssh/kex.c b/usr.bin/ssh/kex.c
index e7bd7a48a9f..c8d339824f3 100644
--- a/usr.bin/ssh/kex.c
+++ b/usr.bin/ssh/kex.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.c,v 1.82 2009/10/24 11:13:54 andreas Exp $ */
+/* $OpenBSD: kex.c,v 1.83 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
  *
@@ -87,7 +87,7 @@ kex_buf2prop(Buffer *raw, int *first_kex_follows)
 		buffer_get_char(&b);
 	/* extract kex init proposal strings */
 	for (i = 0; i < PROPOSAL_MAX; i++) {
-		proposal[i] = buffer_get_string(&b,NULL);
+		proposal[i] = buffer_get_cstring(&b,NULL);
 		debug2("kex_parse_kexinit: %s", proposal[i]);
 	}
 	/* first kex follows / reserved */
diff --git a/usr.bin/ssh/key.c b/usr.bin/ssh/key.c
index d3431de94fc..16ee2f960b9 100644
--- a/usr.bin/ssh/key.c
+++ b/usr.bin/ssh/key.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.c,v 1.90 2010/07/13 23:13:16 djm Exp $ */
+/* $OpenBSD: key.c,v 1.91 2010/08/31 09:58:37 djm Exp $ */
 /*
  * read_bignum():
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1063,7 +1063,7 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 	principals = exts = critical = sig_key = sig = NULL;
 	if ((!v00 && buffer_get_int64_ret(&key->cert->serial, b) != 0) ||
 	    buffer_get_int_ret(&key->cert->type, b) != 0 ||
-	    (key->cert->key_id = buffer_get_string_ret(b, &kidlen)) == NULL ||
+	    (key->cert->key_id = buffer_get_cstring_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 ||
@@ -1101,15 +1101,10 @@ cert_parse(Buffer *b, Key *key, const u_char *blob, u_int blen)
 			error("%s: Too many principals", __func__);
 			goto out;
 		}
-		if ((principal = buffer_get_string_ret(&tmp, &plen)) == NULL) {
+		if ((principal = buffer_get_cstring_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,
 		    key->cert->nprincipals + 1, sizeof(*key->cert->principals));
 		key->cert->principals[key->cert->nprincipals++] = principal;
@@ -1196,7 +1191,7 @@ key_from_blob(const u_char *blob, u_int blen)
 #endif
 	buffer_init(&b);
 	buffer_append(&b, blob, blen);
-	if ((ktype = buffer_get_string_ret(&b, NULL)) == NULL) {
+	if ((ktype = buffer_get_cstring_ret(&b, NULL)) == NULL) {
 		error("key_from_blob: can't read key type");
 		goto out;
 	}
diff --git a/usr.bin/ssh/packet.c b/usr.bin/ssh/packet.c
index d948890cf0e..711a232c77e 100644
--- a/usr.bin/ssh/packet.c
+++ b/usr.bin/ssh/packet.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.c,v 1.168 2010/07/13 23:13:16 djm Exp $ */
+/* $OpenBSD: packet.c,v 1.169 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1536,6 +1536,13 @@ packet_get_string_ptr(u_int *length_ptr)
 	return buffer_get_string_ptr(&active_state->incoming_packet, length_ptr);
 }
 
+/* Ensures the returned string has no embedded \0 characters in it. */
+char *
+packet_get_cstring(u_int *length_ptr)
+{
+	return buffer_get_cstring(&active_state->incoming_packet, length_ptr);
+}
+
 /*
  * Sends a diagnostic message from the server to the client.  This message
  * can be sent at any time (but not while constructing another message). The
diff --git a/usr.bin/ssh/packet.h b/usr.bin/ssh/packet.h
index 33523d7503c..fd0b056fd88 100644
--- a/usr.bin/ssh/packet.h
+++ b/usr.bin/ssh/packet.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.h,v 1.52 2009/06/27 09:29:06 andreas Exp $ */
+/* $OpenBSD: packet.h,v 1.53 2010/08/31 09:58:37 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -61,6 +61,7 @@ void     packet_get_bignum(BIGNUM * value);
 void     packet_get_bignum2(BIGNUM * value);
 void	*packet_get_raw(u_int *length_ptr);
 void	*packet_get_string(u_int *length_ptr);
+char	*packet_get_cstring(u_int *length_ptr);
 void	*packet_get_string_ptr(u_int *length_ptr);
 void     packet_disconnect(const char *fmt,...) __attribute__((format(printf, 1, 2)));
 void     packet_send_debug(const char *fmt,...) __attribute__((format(printf, 1, 2)));
diff --git a/usr.bin/ssh/ssh-dss.c b/usr.bin/ssh/ssh-dss.c
index 719f350f29b..9ffd87fabd9 100644
--- a/usr.bin/ssh/ssh-dss.c
+++ b/usr.bin/ssh/ssh-dss.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-dss.c,v 1.26 2010/04/16 01:47:26 djm Exp $ */
+/* $OpenBSD: ssh-dss.c,v 1.27 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  *
@@ -130,7 +130,7 @@ ssh_dss_verify(const Key *key, const u_char *signature, u_int signaturelen,
 		char *ktype;
 		buffer_init(&b);
 		buffer_append(&b, signature, signaturelen);
-		ktype = buffer_get_string(&b, NULL);
+		ktype = buffer_get_cstring(&b, NULL);
 		if (strcmp("ssh-dss", ktype) != 0) {
 			error("ssh_dss_verify: cannot handle type %s", ktype);
 			buffer_free(&b);
diff --git a/usr.bin/ssh/ssh-rsa.c b/usr.bin/ssh/ssh-rsa.c
index b29546783a2..5766582976f 100644
--- a/usr.bin/ssh/ssh-rsa.c
+++ b/usr.bin/ssh/ssh-rsa.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-rsa.c,v 1.44 2010/07/16 14:07:35 djm Exp $ */
+/* $OpenBSD: ssh-rsa.c,v 1.45 2010/08/31 09:58:37 djm Exp $ */
 /*
  * Copyright (c) 2000, 2003 Markus Friedl <markus@openbsd.org>
  *
@@ -124,7 +124,7 @@ ssh_rsa_verify(const Key *key, const u_char *signature, u_int signaturelen,
 	}
 	buffer_init(&b);
 	buffer_append(&b, signature, signaturelen);
-	ktype = buffer_get_string(&b, NULL);
+	ktype = buffer_get_cstring(&b, NULL);
 	if (strcmp("ssh-rsa", ktype) != 0) {
 		error("ssh_rsa_verify: cannot handle type %s", ktype);
 		buffer_free(&b);