- 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.
2010-08-31 22:36:39 +10:00 · 2010-08-31 22:36:39 +10:00 · da108ece68
--- a/14
+++ b/14
@ -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,
--- 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);
--- 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)
--- 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);

--- 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)
 {
--- 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 *);

--- 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 */
--- 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;
 	}
--- 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
--- 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)));
--- 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);
--- 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);