diff options
| -rw-r--r-- | ChangeLog | 14 | ||||
| -rw-r--r-- | auth-options.c | 8 | ||||
| -rw-r--r-- | auth1.c | 6 | ||||
| -rw-r--r-- | auth2.c | 10 | ||||
| -rw-r--r-- | bufaux.c | 35 | ||||
| -rw-r--r-- | buffer.h | 4 | ||||
| -rw-r--r-- | kex.c | 4 | ||||
| -rw-r--r-- | key.c | 13 | ||||
| -rw-r--r-- | packet.c | 9 | ||||
| -rw-r--r-- | packet.h | 3 | ||||
| -rw-r--r-- | ssh-dss.c | 4 | ||||
| -rw-r--r-- | ssh-rsa.c | 4 |
12 files changed, 83 insertions, 31 deletions
@@ -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); @@ -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) @@ -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); @@ -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) { @@ -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 *); @@ -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 */ @@ -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; } @@ -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 @@ -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))); @@ -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); @@ -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); |