- djm@cvs.openbsd.org 2010/08/31 09:58:37

     [auth-options.c auth1.c auth2.c bufaux.c buffer.h kex.c key.c packet.c]
     [packet.h ssh-dss.c ssh-rsa.c]
     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.

12 files changed, 83 insertions, 31 deletions

diff --git a/ChangeLog b/ChangeLog
index a56f0434..2f4acd9d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -11,6 +11,20 @@
    - djm@cvs.openbsd.org 2010/08/16 04:06:06
      [ssh-add.c ssh-agent.c ssh-keygen.c ssh-keysign.c ssh.c sshd.c]
      backout previous temporarily; discussed with deraadt@
+   - djm@cvs.openbsd.org 2010/08/31 09:58:37
+     [auth-options.c auth1.c auth2.c bufaux.c buffer.h kex.c key.c packet.c]
+     [packet.h ssh-dss.c ssh-rsa.c]
+     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.
 
 20100827
  - (dtucker) [contrib/redhat/sshd.init] Bug #1810: initlog is deprecated,
diff --git a/auth-options.c b/auth-options.c
index a7040247..a9c26add 100644
--- a/auth-options.c
+++ b/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
@@ -444,7 +444,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;
@@ -479,7 +479,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);
@@ -500,7 +500,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/auth1.c b/auth1.c
index bf442dbf..cc85aec7 100644
--- a/auth1.c
+++ b/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
@@ -167,7 +167,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);
@@ -389,7 +389,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/auth2.c b/auth2.c
index 5d546855..95820f96 100644
--- a/auth2.c
+++ b/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.
  *
@@ -182,7 +182,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)
@@ -221,9 +221,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/bufaux.c b/bufaux.c
index 854fd510..00208ca2 100644
--- a/bufaux.c
+++ b/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
@@ -202,6 +202,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/buffer.h b/buffer.h
index 4ef4f80b..93baae2c 100644
--- a/buffer.h
+++ b/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/kex.c b/kex.c
index 148cfee8..ca5aae3e 100644
--- a/kex.c
+++ b/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.
  *
@@ -98,7 +98,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/key.c b/key.c
index e4aa25c0..aed4678c 100644
--- a/key.c
+++ b/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
@@ -1067,7 +1067,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 ||
@@ -1105,15 +1105,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;
@@ -1200,7 +1195,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/packet.c b/packet.c
index 48f7fe61..49aa9733 100644
--- a/packet.c
+++ b/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
@@ -1546,6 +1546,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/packet.h b/packet.h
index 33523d75..fd0b056f 100644
--- a/packet.h
+++ b/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/ssh-dss.c b/ssh-dss.c
index 175e4d03..ede5e21e 100644
--- a/ssh-dss.c
+++ b/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.
  *
@@ -133,7 +133,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/ssh-rsa.c b/ssh-rsa.c
index c471ff32..c6355fa0 100644
--- a/ssh-rsa.c
+++ b/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>
  *
@@ -127,7 +127,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);