From: djm Date: Mon, 13 Aug 2018 02:41:05 +0000 (+0000) Subject: revert compat.[ch] section of the following change. It causes X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a8da790758f1987ad578961b45b4780f6b1c5610;p=openbsd revert compat.[ch] section of the following change. It causes double-free under some circumstances. -- date: 2018/07/31 03:07:24; author: djm; state: Exp; lines: +33 -18; commitid: f7g4UI8eeOXReTPh; fix some memory leaks spotted by Coverity via Jakub Jelen in bz#2366 feedback and ok dtucker@ --- diff --git a/usr.bin/ssh/compat.c b/usr.bin/ssh/compat.c index 71a7317eb5d..c2bde1353db 100644 --- a/usr.bin/ssh/compat.c +++ b/usr.bin/ssh/compat.c @@ -1,4 +1,4 @@ -/* $OpenBSD: compat.c,v 1.112 2018/07/31 03:07:24 djm Exp $ */ +/* $OpenBSD: compat.c,v 1.113 2018/08/13 02:41:05 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001, 2002 Markus Friedl. All rights reserved. * @@ -182,17 +182,13 @@ proto_spec(const char *spec) } char * -compat_cipher_proposal(char *cipher_prop, u_int compat) +compat_cipher_proposal(char *cipher_prop) { - char *cp; - - if (!(compat & SSH_BUG_BIGENDIANAES)) + if (!(datafellows & SSH_BUG_BIGENDIANAES)) return cipher_prop; debug2("%s: original cipher proposal: %s", __func__, cipher_prop); - if ((cp = match_filter_blacklist(cipher_prop, "aes*")) == NULL) + if ((cipher_prop = 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"); @@ -200,17 +196,13 @@ compat_cipher_proposal(char *cipher_prop, u_int compat) } char * -compat_pkalg_proposal(char *pkalg_prop, u_int compat) +compat_pkalg_proposal(char *pkalg_prop) { - char *cp; - - if (!(compat & SSH_BUG_RSASIGMD5)) + if (!(datafellows & SSH_BUG_RSASIGMD5)) return pkalg_prop; debug2("%s: original public key proposal: %s", __func__, pkalg_prop); - if ((cp = match_filter_blacklist(pkalg_prop, "ssh-rsa")) == NULL) + if ((pkalg_prop = 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"); @@ -218,31 +210,24 @@ compat_pkalg_proposal(char *pkalg_prop, u_int compat) } char * -compat_kex_proposal(char *kex_prop, u_int compat) +compat_kex_proposal(char *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, + 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, "curve25519-sha256@libssh.org")) == NULL) fatal("match_filter_blacklist failed"); - free(kex_prop); - kex_prop = cp; - } - if ((compat & SSH_OLD_DHGEX) != 0) { - if ((cp = match_filter_blacklist(kex_prop, + if ((datafellows & SSH_OLD_DHGEX) != 0) { + if ((p = match_filter_blacklist(p, "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__, kex_prop); - if (*kex_prop == '\0') + debug2("%s: compat KEX proposal: %s", __func__, p); + if (*p == '\0') fatal("No supported key exchange algorithms found"); - return kex_prop; + return p; } diff --git a/usr.bin/ssh/compat.h b/usr.bin/ssh/compat.h index e2877737b50..d611d33e736 100644 --- a/usr.bin/ssh/compat.h +++ b/usr.bin/ssh/compat.h @@ -1,4 +1,4 @@ -/* $OpenBSD: compat.h,v 1.53 2018/07/31 03:07:24 djm Exp $ */ +/* $OpenBSD: compat.h,v 1.54 2018/08/13 02:41:05 djm Exp $ */ /* * Copyright (c) 1999, 2000, 2001 Markus Friedl. All rights reserved. @@ -65,15 +65,9 @@ u_int compat_datafellows(const char *); int proto_spec(const 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); +char *compat_cipher_proposal(char *); +char *compat_pkalg_proposal(char *); +char *compat_kex_proposal(char *); extern int datafellows; #endif diff --git a/usr.bin/ssh/sshconnect2.c b/usr.bin/ssh/sshconnect2.c index c309ab7b315..957a04b692c 100644 --- a/usr.bin/ssh/sshconnect2.c +++ b/usr.bin/ssh/sshconnect2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshconnect2.c,v 1.283 2018/07/31 03:07:24 djm Exp $ */ +/* $OpenBSD: sshconnect2.c,v 1.284 2018/08/13 02:41:05 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, datafellows); + myproposal[PROPOSAL_KEX_ALGS] = compat_kex_proposal(s); myproposal[PROPOSAL_ENC_ALGS_CTOS] = - compat_cipher_proposal(options.ciphers, datafellows); + compat_cipher_proposal(options.ciphers); myproposal[PROPOSAL_ENC_ALGS_STOC] = - compat_cipher_proposal(options.ciphers, datafellows); + compat_cipher_proposal(options.ciphers); myproposal[PROPOSAL_COMP_ALGS_CTOS] = myproposal[PROPOSAL_COMP_ALGS_STOC] = options.compression ? "zlib@openssh.com,zlib,none" : "none,zlib@openssh.com,zlib"; @@ -178,15 +178,14 @@ 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, - datafellows); + compat_pkalg_proposal(options.hostkeyalgorithms); } 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), datafellows); + order_hostkeyalgs(host, hostaddr, port)); } if (options.rekey_limit || options.rekey_interval) @@ -216,7 +215,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, datafellows); + compat_kex_proposal(options.kex_algorithms); 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 a125f4aaa9d..46b5689bf82 100644 --- a/usr.bin/ssh/sshd.c +++ b/usr.bin/ssh/sshd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshd.c,v 1.513 2018/07/31 03:07:24 djm Exp $ */ +/* $OpenBSD: sshd.c,v 1.514 2018/08/13 02:41:05 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, datafellows); + options.kex_algorithms); myproposal[PROPOSAL_ENC_ALGS_CTOS] = compat_cipher_proposal( - options.ciphers, datafellows); + options.ciphers); myproposal[PROPOSAL_ENC_ALGS_STOC] = compat_cipher_proposal( - options.ciphers, datafellows); + options.ciphers); 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(), datafellows); + list_hostkey_types()); /* start key exchange */ if ((r = kex_setup(active_state, myproposal)) != 0)