From 1a31d02b2411c4718de58ce796dbb7b5e14db93e Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Mon, 2 May 2016 08:49:03 +0000 Subject: [PATCH] upstream commit fix signed/unsigned errors reported by clang-3.7; add sshbuf_dup_string() to replace a common idiom of strdup(sshbuf_ptr()) with better safety checking; feedback and ok markus@ Upstream-ID: 71f926d9bb3f1efed51319a6daf37e93d57c8820 --- auth2-chall.c | 6 +++--- auth2.c | 6 +++--- kex.h | 7 ++++--- kexc25519.c | 6 +++--- monitor.c | 27 ++++++++++++++++----------- servconf.c | 5 +++-- sftp-client.c | 5 ++--- ssh-agent.c | 15 ++++++++------- ssh-keygen.c | 8 ++++---- sshbuf-misc.c | 25 ++++++++++++++++++++++++- sshbuf.h | 9 ++++++++- sshconnect2.c | 6 +++--- sshd.c | 51 +++++++++++++++++++++++++++++++-------------------- 13 files changed, 112 insertions(+), 64 deletions(-) diff --git a/auth2-chall.c b/auth2-chall.c index 4aff09d80..ead480318 100644 --- a/auth2-chall.c +++ b/auth2-chall.c @@ -1,4 +1,4 @@ -/* $OpenBSD: auth2-chall.c,v 1.43 2015/07/18 07:57:14 djm Exp $ */ +/* $OpenBSD: auth2-chall.c,v 1.44 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2001 Markus Friedl. All rights reserved. * Copyright (c) 2001 Per Allansson. All rights reserved. @@ -122,8 +122,8 @@ kbdint_alloc(const char *devs) buffer_append(&b, devices[i]->name, strlen(devices[i]->name)); } - buffer_append(&b, "\0", 1); - kbdintctxt->devices = xstrdup(buffer_ptr(&b)); + if ((kbdintctxt->devices = sshbuf_dup_string(&b)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); buffer_free(&b); } else { kbdintctxt->devices = xstrdup(devs); diff --git a/auth2.c b/auth2.c index 717796228..9108b8612 100644 --- a/auth2.c +++ b/auth2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: auth2.c,v 1.135 2015/01/19 20:07:45 markus Exp $ */ +/* $OpenBSD: auth2.c,v 1.136 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2000 Markus Friedl. All rights reserved. * @@ -424,8 +424,8 @@ authmethods_get(Authctxt *authctxt) buffer_append(&b, authmethods[i]->name, strlen(authmethods[i]->name)); } - buffer_append(&b, "\0", 1); - list = xstrdup(buffer_ptr(&b)); + if ((list = sshbuf_dup_string(&b)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); buffer_free(&b); return list; } diff --git a/kex.h b/kex.h index 1c5896605..131b8d93d 100644 --- a/kex.h +++ b/kex.h @@ -1,4 +1,4 @@ -/* $OpenBSD: kex.h,v 1.76 2016/02/08 10:57:07 djm Exp $ */ +/* $OpenBSD: kex.h,v 1.77 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2000, 2001 Markus Friedl. All rights reserved. @@ -205,8 +205,9 @@ int kex_ecdh_hash(int, const EC_GROUP *, const char *, const char *, const u_char *, size_t, const u_char *, size_t, const u_char *, size_t, const EC_POINT *, const EC_POINT *, const BIGNUM *, u_char *, size_t *); -int kex_c25519_hash(int, const char *, const char *, const char *, size_t, - const char *, size_t, const u_char *, size_t, const u_char *, const u_char *, +int kex_c25519_hash(int, const char *, const char *, + const u_char *, size_t, const u_char *, size_t, + const u_char *, size_t, const u_char *, const u_char *, const u_char *, size_t, u_char *, size_t *); void kexc25519_keygen(u_char key[CURVE25519_SIZE], u_char pub[CURVE25519_SIZE]) diff --git a/kexc25519.c b/kexc25519.c index 8d8cd4a2b..0897b8c51 100644 --- a/kexc25519.c +++ b/kexc25519.c @@ -1,4 +1,4 @@ -/* $OpenBSD: kexc25519.c,v 1.9 2015/03/26 07:00:04 djm Exp $ */ +/* $OpenBSD: kexc25519.c,v 1.10 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2001, 2013 Markus Friedl. All rights reserved. * Copyright (c) 2010 Damien Miller. All rights reserved. @@ -86,8 +86,8 @@ kex_c25519_hash( int hash_alg, const char *client_version_string, const char *server_version_string, - const char *ckexinit, size_t ckexinitlen, - const char *skexinit, size_t skexinitlen, + const u_char *ckexinit, size_t ckexinitlen, + const u_char *skexinit, size_t skexinitlen, const u_char *serverhostkeyblob, size_t sbloblen, const u_char client_dh_pub[CURVE25519_SIZE], const u_char server_dh_pub[CURVE25519_SIZE], diff --git a/monitor.c b/monitor.c index 6b780e480..dce920c23 100644 --- a/monitor.c +++ b/monitor.c @@ -1,4 +1,4 @@ -/* $OpenBSD: monitor.c,v 1.158 2016/03/07 19:02:43 djm Exp $ */ +/* $OpenBSD: monitor.c,v 1.159 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright 2002 Niels Provos * Copyright 2002 Markus Friedl @@ -34,6 +34,7 @@ #include #include +#include #ifdef HAVE_PATHS_H #include #endif @@ -688,7 +689,8 @@ mm_answer_sign(int sock, Buffer *m) u_char *p = NULL, *signature = NULL; char *alg = NULL; size_t datlen, siglen, alglen; - int r, keyid, is_proof = 0; + int r, is_proof = 0; + u_int keyid; const char proof_req[] = "hostkeys-prove-00@openssh.com"; debug3("%s", __func__); @@ -697,6 +699,8 @@ mm_answer_sign(int sock, Buffer *m) (r = sshbuf_get_string(m, &p, &datlen)) != 0 || (r = sshbuf_get_cstring(m, &alg, &alglen)) != 0) fatal("%s: buffer error: %s", __func__, ssh_err(r)); + if (keyid > INT_MAX) + fatal("%s: invalid key ID", __func__); /* * Supported KEX types use SHA1 (20 bytes), SHA256 (32 bytes), @@ -1289,7 +1293,8 @@ static int monitor_valid_userblob(u_char *data, u_int datalen) { Buffer b; - char *p, *userstyle; + u_char *p; + char *userstyle, *cp; u_int len; int fail = 0; @@ -1314,26 +1319,26 @@ monitor_valid_userblob(u_char *data, u_int datalen) } if (buffer_get_char(&b) != SSH2_MSG_USERAUTH_REQUEST) fail++; - p = buffer_get_cstring(&b, NULL); + cp = buffer_get_cstring(&b, NULL); xasprintf(&userstyle, "%s%s%s", authctxt->user, authctxt->style ? ":" : "", authctxt->style ? authctxt->style : ""); - if (strcmp(userstyle, p) != 0) { - logit("wrong user name passed to monitor: expected %s != %.100s", - userstyle, p); + if (strcmp(userstyle, cp) != 0) { + logit("wrong user name passed to monitor: " + "expected %s != %.100s", userstyle, cp); fail++; } free(userstyle); - free(p); + free(cp); buffer_skip_string(&b); if (datafellows & SSH_BUG_PKAUTH) { if (!buffer_get_char(&b)) fail++; } else { - p = buffer_get_cstring(&b, NULL); - if (strcmp("publickey", p) != 0) + cp = buffer_get_cstring(&b, NULL); + if (strcmp("publickey", cp) != 0) fail++; - free(p); + free(cp); if (!buffer_get_char(&b)) fail++; buffer_skip_string(&b); diff --git a/servconf.c b/servconf.c index ba39dce1d..6111c5a94 100644 --- a/servconf.c +++ b/servconf.c @@ -1,5 +1,5 @@ -/* $OpenBSD: servconf.c,v 1.286 2016/03/07 19:02:43 djm Exp $ */ +/* $OpenBSD: servconf.c,v 1.287 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland * All rights reserved @@ -2059,7 +2059,8 @@ parse_server_config(ServerOptions *options, const char *filename, Buffer *conf, debug2("%s: config %s len %d", __func__, filename, buffer_len(conf)); - obuf = cbuf = xstrdup(buffer_ptr(conf)); + if ((obuf = cbuf = sshbuf_dup_string(conf)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); active = connectinfo ? 0 : 1; linenum = 1; while ((cp = strsep(&cbuf, "\n")) != NULL) { diff --git a/sftp-client.c b/sftp-client.c index cd990579e..faf14684c 100644 --- a/sftp-client.c +++ b/sftp-client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sftp-client.c,v 1.122 2016/04/08 08:19:17 djm Exp $ */ +/* $OpenBSD: sftp-client.c,v 1.123 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2001-2004 Damien Miller * @@ -515,8 +515,7 @@ do_lsreaddir(struct sftp_conn *conn, const char *path, int print_flag, struct sshbuf *msg; u_int count, id, i, expected_id, ents = 0; size_t handle_len; - u_char type; - char *handle; + u_char type, *handle; int status = SSH2_FX_FAILURE; int r; diff --git a/ssh-agent.c b/ssh-agent.c index c38906d94..8aa25b30d 100644 --- a/ssh-agent.c +++ b/ssh-agent.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-agent.c,v 1.212 2016/02/15 09:47:49 dtucker Exp $ */ +/* $OpenBSD: ssh-agent.c,v 1.213 2016/05/02 08:49:03 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -144,8 +144,8 @@ char socket_dir[PATH_MAX]; #define LOCK_SALT_SIZE 16 #define LOCK_ROUNDS 1 int locked = 0; -char lock_passwd[LOCK_SIZE]; -char lock_salt[LOCK_SALT_SIZE]; +u_char lock_pwhash[LOCK_SIZE]; +u_char lock_salt[LOCK_SALT_SIZE]; extern char *__progname; @@ -677,7 +677,8 @@ static void process_lock_agent(SocketEntry *e, int lock) { int r, success = 0, delay; - char *passwd, passwdhash[LOCK_SIZE]; + char *passwd; + u_char passwdhash[LOCK_SIZE]; static u_int fail_count = 0; size_t pwlen; @@ -689,11 +690,11 @@ process_lock_agent(SocketEntry *e, int lock) if (bcrypt_pbkdf(passwd, pwlen, lock_salt, sizeof(lock_salt), passwdhash, sizeof(passwdhash), LOCK_ROUNDS) < 0) fatal("bcrypt_pbkdf"); - if (timingsafe_bcmp(passwdhash, lock_passwd, LOCK_SIZE) == 0) { + if (timingsafe_bcmp(passwdhash, lock_pwhash, LOCK_SIZE) == 0) { debug("agent unlocked"); locked = 0; fail_count = 0; - explicit_bzero(lock_passwd, sizeof(lock_passwd)); + explicit_bzero(lock_pwhash, sizeof(lock_pwhash)); success = 1; } else { /* delay in 0.1s increments up to 10s */ @@ -710,7 +711,7 @@ process_lock_agent(SocketEntry *e, int lock) locked = 1; arc4random_buf(lock_salt, sizeof(lock_salt)); if (bcrypt_pbkdf(passwd, pwlen, lock_salt, sizeof(lock_salt), - lock_passwd, sizeof(lock_passwd), LOCK_ROUNDS) < 0) + lock_pwhash, sizeof(lock_pwhash), LOCK_ROUNDS) < 0) fatal("bcrypt_pbkdf"); success = 1; } diff --git a/ssh-keygen.c b/ssh-keygen.c index 478520123..079f10321 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssh-keygen.c,v 1.288 2016/02/15 09:47:49 dtucker Exp $ */ +/* $OpenBSD: ssh-keygen.c,v 1.289 2016/05/02 08:49:03 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1994 Tatu Ylonen , Espoo, Finland @@ -883,7 +883,7 @@ do_fingerprint(struct passwd *pw) char *comment = NULL, *cp, *ep, line[SSH_MAX_PUBKEY_BYTES]; int i, invalid = 1; const char *path; - long int lnum = 0; + u_long lnum = 0; if (!have_identity) ask_filename(pw, "Enter file in which the key is"); @@ -946,7 +946,7 @@ do_fingerprint(struct passwd *pw) } /* Retry after parsing leading hostname/key options */ if (public == NULL && (public = try_read_key(&cp)) == NULL) { - debug("%s:%ld: not a public key", path, lnum); + debug("%s:%lu: not a public key", path, lnum); continue; } @@ -1920,7 +1920,7 @@ do_show_cert(struct passwd *pw) FILE *f; char *cp, line[SSH_MAX_PUBKEY_BYTES]; const char *path; - long int lnum = 0; + u_long lnum = 0; if (!have_identity) ask_filename(pw, "Enter file in which the key is"); diff --git a/sshbuf-misc.c b/sshbuf-misc.c index 3da4b80e7..15dcfbc79 100644 --- a/sshbuf-misc.c +++ b/sshbuf-misc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshbuf-misc.c,v 1.5 2015/10/05 17:11:21 djm Exp $ */ +/* $OpenBSD: sshbuf-misc.c,v 1.6 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2011 Damien Miller * @@ -136,3 +136,26 @@ sshbuf_b64tod(struct sshbuf *buf, const char *b64) return 0; } +char * +sshbuf_dup_string(struct sshbuf *buf) +{ + const u_char *p = NULL, *s = sshbuf_ptr(buf); + size_t l = sshbuf_len(buf); + char *r; + + if (s == NULL || l > SIZE_MAX) + return NULL; + /* accept a nul only as the last character in the buffer */ + if (l > 0 && (p = memchr(s, '\0', l)) != NULL) { + if (p != s + l - 1) + return NULL; + l--; /* the nul is put back below */ + } + if ((r = malloc(l + 1)) == NULL) + return NULL; + if (l > 0) + memcpy(r, s, l); + r[l] = '\0'; + return r; +} + diff --git a/sshbuf.h b/sshbuf.h index 63495fbb0..52ff017cc 100644 --- a/sshbuf.h +++ b/sshbuf.h @@ -1,4 +1,4 @@ -/* $OpenBSD: sshbuf.h,v 1.6 2015/12/10 07:01:35 mmcc Exp $ */ +/* $OpenBSD: sshbuf.h,v 1.7 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2011 Damien Miller * @@ -239,6 +239,13 @@ char *sshbuf_dtob64(struct sshbuf *buf); /* Decode base64 data and append it to the buffer */ int sshbuf_b64tod(struct sshbuf *buf, const char *b64); +/* + * Duplicate the contents of a buffer to a string (caller to free). + * Returns NULL on buffer error, or if the buffer contains a premature + * nul character. + */ +char *sshbuf_dup_string(struct sshbuf *buf); + /* Macros for decoding/encoding integers */ #define PEEK_U64(p) \ (((u_int64_t)(((const u_char *)(p))[0]) << 56) | \ diff --git a/sshconnect2.c b/sshconnect2.c index f7d0644e8..1dddf75aa 100644 --- a/sshconnect2.c +++ b/sshconnect2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshconnect2.c,v 1.241 2016/04/28 14:30:21 djm Exp $ */ +/* $OpenBSD: sshconnect2.c,v 1.242 2016/05/02 08:49:03 djm Exp $ */ /* * Copyright (c) 2000 Markus Friedl. All rights reserved. * Copyright (c) 2008 Damien Miller. All rights reserved. @@ -1922,8 +1922,8 @@ authmethods_get(void) buffer_append(&b, method->name, strlen(method->name)); } } - buffer_append(&b, "\0", 1); - list = xstrdup(buffer_ptr(&b)); + if ((list = sshbuf_dup_string(&b)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); buffer_free(&b); return list; } diff --git a/sshd.c b/sshd.c index d21aed515..8b8af2494 100644 --- a/sshd.c +++ b/sshd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshd.c,v 1.466 2016/03/07 19:02:43 djm Exp $ */ +/* $OpenBSD: sshd.c,v 1.467 2016/05/02 08:49:03 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -845,8 +845,8 @@ list_hostkey_types(void) break; } } - buffer_append(&b, "\0", 1); - ret = xstrdup(buffer_ptr(&b)); + if ((ret = sshbuf_dup_string(&b)) == NULL) + fatal("%s: sshbuf_dup_string failed", __func__); buffer_free(&b); debug("list_hostkey_types: %s", ret); return ret; @@ -1027,12 +1027,13 @@ usage(void) } static void -send_rexec_state(int fd, Buffer *conf) +send_rexec_state(int fd, struct sshbuf *conf) { - Buffer m; + struct sshbuf *m; + int r; - debug3("%s: entering fd = %d config len %d", __func__, fd, - buffer_len(conf)); + debug3("%s: entering fd = %d config len %zu", __func__, fd, + sshbuf_len(conf)); /* * Protocol from reexec master to child: @@ -1046,31 +1047,41 @@ send_rexec_state(int fd, Buffer *conf) * bignum q " * string rngseed (only if OpenSSL is not self-seeded) */ - buffer_init(&m); - buffer_put_cstring(&m, buffer_ptr(conf)); + if ((m = sshbuf_new()) == NULL) + fatal("%s: sshbuf_new failed", __func__); + if ((r = sshbuf_put_stringb(m, conf)) != 0) + fatal("%s: buffer error: %s", __func__, ssh_err(r)); #ifdef WITH_SSH1 if (sensitive_data.server_key != NULL && sensitive_data.server_key->type == KEY_RSA1) { - buffer_put_int(&m, 1); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->e); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->n); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->d); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->iqmp); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->p); - buffer_put_bignum(&m, sensitive_data.server_key->rsa->q); + if ((r = sshbuf_put_u32(m, 1)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->e)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->n)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->d)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->iqmp)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->p)) != 0 || + (r = sshbuf_put_bignum1(m, + sensitive_data.server_key->rsa->q)) != 0) + fatal("%s: buffer error: %s", __func__, ssh_err(r)); } else #endif - buffer_put_int(&m, 0); + if ((r = sshbuf_put_u32(m, 1)) != 0) + fatal("%s: buffer error: %s", __func__, ssh_err(r)); #if defined(WITH_OPENSSL) && !defined(OPENSSL_PRNG_ONLY) - rexec_send_rng_seed(&m); + rexec_send_rng_seed(m); #endif - if (ssh_msg_send(fd, 0, &m) == -1) + if (ssh_msg_send(fd, 0, m) == -1) fatal("%s: ssh_msg_send failed", __func__); - buffer_free(&m); + sshbuf_free(m); debug3("%s: done", __func__); }