12 files changed, 83 insertions, 31 deletions

diff --git a/ChangeLog b/ChangeLog
index a56f04349..2f4acd9de 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 a7040247f..a9c26add6 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 bf442dbf6..cc85aec74 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 5d5468559..95820f96f 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 854fd510a..00208ca27 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 4ef4f80b3..93baae2c8 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 148cfee80..ca5aae3e4 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 e4aa25c03..aed4678cb 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 48f7fe613..49aa97335 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 33523d750..fd0b056fd 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 175e4d030..ede5e21e5 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 c471ff323..c6355fa09 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);