- 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.
diff --git a/ChangeLog b/ChangeLog
index a56f043..2f4acd9 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 a704024..a9c26ad 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 @@
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 @@
}
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 @@
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 bf442db..cc85aec 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 @@
* 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 @@
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 5d54685..95820f9 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 @@
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 @@
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 854fd51..00208ca 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 @@
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 4ef4f80..93baae2 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_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_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 148cfee..ca5aae3 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 @@
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 e4aa25c..aed4678 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 @@
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 @@
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 @@
#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 48f7fe6..49aa973 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 @@
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 33523d7..fd0b056 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_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 175e4d0..ede5e21 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 @@
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 c471ff3..c6355fa 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 @@
}
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);