From 1addc7ae5beca221ada86e5a67a6872f22c23fd1 Mon Sep 17 00:00:00 2001 From: djm Date: Tue, 31 Jul 2018 03:07:24 +0000 Subject: [PATCH] fix some memory leaks spotted by Coverity via Jakub Jelen in bz#2366 feedback and ok dtucker@ --- usr.bin/ssh/addrmatch.c | 25 +++++++++---------- usr.bin/ssh/compat.c | 51 +++++++++++++++++++++++++-------------- usr.bin/ssh/compat.h | 14 ++++++++--- usr.bin/ssh/mux.c | 3 ++- usr.bin/ssh/sftp-client.c | 20 +++++++++------ usr.bin/ssh/sshconnect2.c | 15 ++++++------ usr.bin/ssh/sshd.c | 10 ++++---- 7 files changed, 82 insertions(+), 56 deletions(-) diff --git a/usr.bin/ssh/addrmatch.c b/usr.bin/ssh/addrmatch.c index 452aa4e6a35..ee1f1708c99 100644 --- a/usr.bin/ssh/addrmatch.c +++ b/usr.bin/ssh/addrmatch.c @@ -1,4 +1,4 @@ -/* $OpenBSD: addrmatch.c,v 1.13 2016/09/21 16:55:42 djm Exp $ */ +/* $OpenBSD: addrmatch.c,v 1.14 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 2004-2008 Damien Miller @@ -201,25 +201,24 @@ addr_cmp(const struct xaddr *a, const struct xaddr *b) static int addr_pton(const char *p, struct xaddr *n) { - struct addrinfo hints, *ai; + struct addrinfo hints, *ai = NULL; + int ret = -1; memset(&hints, '\0', sizeof(hints)); hints.ai_flags = AI_NUMERICHOST; if (p == NULL || getaddrinfo(p, NULL, &hints, &ai) != 0) - return -1; - + goto out; if (ai == NULL || ai->ai_addr == NULL) - return -1; - - if (n != NULL && - addr_sa_to_xaddr(ai->ai_addr, ai->ai_addrlen, n) == -1) { + goto out; + if (n != NULL && addr_sa_to_xaddr(ai->ai_addr, ai->ai_addrlen, n) == -1) + goto out; + /* success */ + ret = 0; + out: + if (ai != NULL) freeaddrinfo(ai); - return -1; - } - - freeaddrinfo(ai); - return 0; + return ret; } /* diff --git a/usr.bin/ssh/compat.c b/usr.bin/ssh/compat.c index 9347aed8645..71a7317eb5d 100644 --- a/usr.bin/ssh/compat.c +++ b/usr.bin/ssh/compat.c @@ -1,4 +1,4 @@ -/* $OpenBSD: compat.c,v 1.111 2018/07/09 21:03:30 markus Exp $ */ +/* $OpenBSD: compat.c,v 1.112 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl. All rights reserved. * @@ -182,13 +182,17 @@ proto_spec(const char *spec) } char * -compat_cipher_proposal(char *cipher_prop) +compat_cipher_proposal(char *cipher_prop, u_int compat) { - if (!(datafellows & SSH_BUG_BIGENDIANAES)) + char *cp; + + if (!(compat & SSH_BUG_BIGENDIANAES)) return cipher_prop; debug2("%s: original cipher proposal: %s", __func__, cipher_prop); - if ((cipher_prop = match_filter_blacklist(cipher_prop, "aes*")) == NULL) + if ((cp = match_filter_blacklist(cipher_prop, "aes*")) == NULL) fatal("match_filter_blacklist failed"); + free(cipher_prop); + cipher_prop = cp; debug2("%s: compat cipher proposal: %s", __func__, cipher_prop); if (*cipher_prop == '\0') fatal("No supported ciphers found"); @@ -196,13 +200,17 @@ compat_cipher_proposal(char *cipher_prop) } char * -compat_pkalg_proposal(char *pkalg_prop) +compat_pkalg_proposal(char *pkalg_prop, u_int compat) { - if (!(datafellows & SSH_BUG_RSASIGMD5)) + char *cp; + + if (!(compat & SSH_BUG_RSASIGMD5)) return pkalg_prop; debug2("%s: original public key proposal: %s", __func__, pkalg_prop); - if ((pkalg_prop = match_filter_blacklist(pkalg_prop, "ssh-rsa")) == NULL) + if ((cp = match_filter_blacklist(pkalg_prop, "ssh-rsa")) == NULL) fatal("match_filter_blacklist failed"); + free(pkalg_prop); + pkalg_prop = cp; debug2("%s: compat public key proposal: %s", __func__, pkalg_prop); if (*pkalg_prop == '\0') fatal("No supported PK algorithms found"); @@ -210,24 +218,31 @@ compat_pkalg_proposal(char *pkalg_prop) } char * -compat_kex_proposal(char *p) +compat_kex_proposal(char *kex_prop, u_int compat) { - if ((datafellows & (SSH_BUG_CURVE25519PAD|SSH_OLD_DHGEX)) == 0) - return p; - debug2("%s: original KEX proposal: %s", __func__, p); - if ((datafellows & SSH_BUG_CURVE25519PAD) != 0) - if ((p = match_filter_blacklist(p, + char *cp; + + if ((compat & (SSH_BUG_CURVE25519PAD|SSH_OLD_DHGEX)) == 0) + return kex_prop; + debug2("%s: original KEX proposal: %s", __func__, kex_prop); + if ((compat & SSH_BUG_CURVE25519PAD) != 0) { + if ((cp = match_filter_blacklist(kex_prop, "curve25519-sha256@libssh.org")) == NULL) fatal("match_filter_blacklist failed"); - if ((datafellows & SSH_OLD_DHGEX) != 0) { - if ((p = match_filter_blacklist(p, + free(kex_prop); + kex_prop = cp; + } + if ((compat & SSH_OLD_DHGEX) != 0) { + if ((cp = match_filter_blacklist(kex_prop, "diffie-hellman-group-exchange-sha256," "diffie-hellman-group-exchange-sha1")) == NULL) fatal("match_filter_blacklist failed"); + free(kex_prop); + kex_prop = cp; } - debug2("%s: compat KEX proposal: %s", __func__, p); - if (*p == '\0') + debug2("%s: compat KEX proposal: %s", __func__, kex_prop); + if (*kex_prop == '\0') fatal("No supported key exchange algorithms found"); - return p; + return kex_prop; } diff --git a/usr.bin/ssh/compat.h b/usr.bin/ssh/compat.h index 28d2c81354e..e2877737b50 100644 --- a/usr.bin/ssh/compat.h +++ b/usr.bin/ssh/compat.h @@ -1,4 +1,4 @@ -/* $OpenBSD: compat.h,v 1.52 2018/07/03 11:39:54 djm Exp $ */ +/* $OpenBSD: compat.h,v 1.53 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001 Markus Friedl. All rights reserved. @@ -65,9 +65,15 @@ u_int compat_datafellows(const char *); int proto_spec(const char *); -char *compat_cipher_proposal(char *); -char *compat_pkalg_proposal(char *); -char *compat_kex_proposal(char *); + +/* + * compat_*_proposal will update their respective proposals based on the + * active compat flags. The replacement is performed in-place - i.e. they + * will free their argument and return a new heap-allocated string. + */ +char *compat_cipher_proposal(char *, u_int compat); +char *compat_pkalg_proposal(char *, u_int compat); +char *compat_kex_proposal(char *, u_int compat); extern int datafellows; #endif diff --git a/usr.bin/ssh/mux.c b/usr.bin/ssh/mux.c index 91b292cf3f9..512cc6e496a 100644 --- a/usr.bin/ssh/mux.c +++ b/usr.bin/ssh/mux.c @@ -1,4 +1,4 @@ -/* $OpenBSD: mux.c,v 1.74 2018/07/11 18:53:29 markus Exp $ */ +/* $OpenBSD: mux.c,v 1.75 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 2002-2008 Damien Miller * @@ -1029,6 +1029,7 @@ process_mux_stdio_fwd(struct ssh *ssh, u_int rid, set_nonblock(new_fd[1]); nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1]); + free(chost); nc->ctl_chan = c->self; /* link session -> control channel */ c->remote_id = nc->self; /* link control -> session channel */ diff --git a/usr.bin/ssh/sftp-client.c b/usr.bin/ssh/sftp-client.c index d66eb486f3e..a7f6353baf2 100644 --- a/usr.bin/ssh/sftp-client.c +++ b/usr.bin/ssh/sftp-client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sftp-client.c,v 1.129 2018/05/25 04:25:46 djm Exp $ */ +/* $OpenBSD: sftp-client.c,v 1.130 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 2001-2004 Damien Miller * @@ -1443,7 +1443,7 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, { int i, ret = 0; SFTP_DIRENT **dir_entries; - char *filename, *new_src, *new_dst; + char *filename, *new_src = NULL, *new_dst = NULL; mode_t mode = 0777; if (depth >= MAX_DIR_DEPTH) { @@ -1481,8 +1481,10 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, } for (i = 0; dir_entries[i] != NULL && !interrupted; i++) { - filename = dir_entries[i]->filename; + free(new_dst); + free(new_src); + filename = dir_entries[i]->filename; new_dst = path_append(dst, filename); new_src = path_append(src, filename); @@ -1505,9 +1507,9 @@ download_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, } else logit("%s: not a regular file\n", new_src); - free(new_dst); - free(new_src); } + free(new_dst); + free(new_src); if (preserve_flag) { if (dirattrib->flags & SSH2_FILEXFER_ATTR_ACMODTIME) { @@ -1774,7 +1776,7 @@ upload_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, int ret = 0; DIR *dirp; struct dirent *dp; - char *filename, *new_src, *new_dst; + char *filename, *new_src = NULL, *new_dst = NULL; struct stat sb; Attrib a, *dirattrib; @@ -1825,6 +1827,8 @@ upload_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, while (((dp = readdir(dirp)) != NULL) && !interrupted) { if (dp->d_ino == 0) continue; + free(new_dst); + free(new_src); filename = dp->d_name; new_dst = path_append(dst, filename); new_src = path_append(src, filename); @@ -1851,9 +1855,9 @@ upload_dir_internal(struct sftp_conn *conn, const char *src, const char *dst, } } else logit("%s: not a regular file\n", filename); - free(new_dst); - free(new_src); } + free(new_dst); + free(new_src); do_setstat(conn, dst, &a); diff --git a/usr.bin/ssh/sshconnect2.c b/usr.bin/ssh/sshconnect2.c index 4e5e95dd9d1..c309ab7b315 100644 --- a/usr.bin/ssh/sshconnect2.c +++ b/usr.bin/ssh/sshconnect2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshconnect2.c,v 1.282 2018/07/18 11:34:04 dtucker Exp $ */ +/* $OpenBSD: sshconnect2.c,v 1.283 2018/07/31 03:07:24 djm Exp $ */ /* * Copyright (c) 2000 Markus Friedl. All rights reserved. * Copyright (c) 2008 Damien Miller. All rights reserved. @@ -161,11 +161,11 @@ ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port) if ((s = kex_names_cat(options.kex_algorithms, "ext-info-c")) == NULL) fatal("%s: kex_names_cat", __func__); - myproposal[PROPOSAL_KEX_ALGS] = compat_kex_proposal(s); + myproposal[PROPOSAL_KEX_ALGS] = compat_kex_proposal(s, datafellows); myproposal[PROPOSAL_ENC_ALGS_CTOS] = - compat_cipher_proposal(options.ciphers); + compat_cipher_proposal(options.ciphers, datafellows); myproposal[PROPOSAL_ENC_ALGS_STOC] = - compat_cipher_proposal(options.ciphers); + compat_cipher_proposal(options.ciphers, datafellows); myproposal[PROPOSAL_COMP_ALGS_CTOS] = myproposal[PROPOSAL_COMP_ALGS_STOC] = options.compression ? "zlib@openssh.com,zlib,none" : "none,zlib@openssh.com,zlib"; @@ -178,14 +178,15 @@ ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port) fatal("%s: kex_assemble_namelist", __func__); free(all_key); myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = - compat_pkalg_proposal(options.hostkeyalgorithms); + compat_pkalg_proposal(options.hostkeyalgorithms, + datafellows); } else { /* Enforce default */ options.hostkeyalgorithms = xstrdup(KEX_DEFAULT_PK_ALG); /* Prefer algorithms that we already have keys for */ myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = compat_pkalg_proposal( - order_hostkeyalgs(host, hostaddr, port)); + order_hostkeyalgs(host, hostaddr, port), datafellows); } if (options.rekey_limit || options.rekey_interval) @@ -215,7 +216,7 @@ ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port) /* remove ext-info from the KEX proposals for rekeying */ myproposal[PROPOSAL_KEX_ALGS] = - compat_kex_proposal(options.kex_algorithms); + compat_kex_proposal(options.kex_algorithms, datafellows); if ((r = kex_prop2buf(kex->my, myproposal)) != 0) fatal("kex_prop2buf: %s", ssh_err(r)); diff --git a/usr.bin/ssh/sshd.c b/usr.bin/ssh/sshd.c index 2719e10c90e..a125f4aaa9d 100644 --- a/usr.bin/ssh/sshd.c +++ b/usr.bin/ssh/sshd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshd.c,v 1.512 2018/07/11 18:53:29 markus Exp $ */ +/* $OpenBSD: sshd.c,v 1.513 2018/07/31 03:07:24 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -2082,11 +2082,11 @@ do_ssh2_kex(void) int r; myproposal[PROPOSAL_KEX_ALGS] = compat_kex_proposal( - options.kex_algorithms); + options.kex_algorithms, datafellows); myproposal[PROPOSAL_ENC_ALGS_CTOS] = compat_cipher_proposal( - options.ciphers); + options.ciphers, datafellows); myproposal[PROPOSAL_ENC_ALGS_STOC] = compat_cipher_proposal( - options.ciphers); + options.ciphers, datafellows); myproposal[PROPOSAL_MAC_ALGS_CTOS] = myproposal[PROPOSAL_MAC_ALGS_STOC] = options.macs; @@ -2100,7 +2100,7 @@ do_ssh2_kex(void) options.rekey_interval); myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = compat_pkalg_proposal( - list_hostkey_types()); + list_hostkey_types(), datafellows); /* start key exchange */ if ((r = kex_setup(active_state, myproposal)) != 0) -- 2.20.1