Attachment 3176 Details for Bug 2687 – New patch set (openssh-7.8)

[patch] New patch set (openssh-7.8)

openssh-coverity.patch (text/plain), 11.73 KB, created by Jakub Jelen on 2018-08-29 00:26:47 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Jakub Jelen

Created: 2018-08-29 00:26:47 AEST

Size: 11.73 KB

patch

obsolete

>From 829e798c254f5d6801134c09264e5a9dc1f46a00 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Wed, 18 Jul 2018 14:52:56 +0200
>Subject: [PATCH 01/11] coverity: RESOURCE_LEAK (CWE-772)
>
>(this ran for older version, so the code is a bit different now)
>
>openssh-7.7p1/compat.c:212: overwrite_var: Overwriting "p" in "p = match_filter_list(p, "diffie-hellman-group-exchange-sha256,diffie-hellman-group-exchange-sha1")" leaks the storage that "p" points to.
>  210|   			fatal("match_filter_list failed");
>  211|   	if ((datafellows & SSH_OLD_DHGEX) != 0) {
>  212|-> 		if ((p = match_filter_list(p,
>  213|   		    "diffie-hellman-group-exchange-sha256,"
>  214|   		    "diffie-hellman-group-exchange-sha1")) == NULL)
>---
> compat.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
>diff --git a/compat.c b/compat.c
>index 0624dc6d..64f5860c 100644
>--- a/compat.c
>+++ b/compat.c
>@@ -186,11 +186,16 @@ proto_spec(const char *spec)
> char *
> compat_cipher_proposal(char *cipher_prop)
> {
>+	char *np = NULL;
>+
> 	if (!(datafellows & 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)
>+	np = match_filter_blacklist(cipher_prop, "aes*");
>+	if (np == NULL)
> 		fatal("match_filter_blacklist failed");
>+	free(cipher_prop);
>+	cipher_prop = np;
> 	debug2("%s: compat cipher proposal: %s", __func__, cipher_prop);
> 	if (*cipher_prop == '\0')
> 		fatal("No supported ciphers found");
>@@ -200,11 +205,16 @@ compat_cipher_proposal(char *cipher_prop)
> char *
> compat_pkalg_proposal(char *pkalg_prop)
> {
>+	char *np = NULL;
>+
> 	if (!(datafellows & 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)
>+	np = match_filter_blacklist(pkalg_prop, "ssh-rsa");
>+	if (np == NULL)
> 		fatal("match_filter_blacklist failed");
>+	free(pkalg_prop);
>+	pkalg_prop = np;
> 	debug2("%s: compat public key proposal: %s", __func__, pkalg_prop);
> 	if (*pkalg_prop == '\0')
> 		fatal("No supported PK algorithms found");
>@@ -217,15 +227,22 @@ compat_kex_proposal(char *p)
> 	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)
>+	if ((datafellows & SSH_BUG_CURVE25519PAD) != 0) {
>+		char *np = match_filter_blacklist(p,
>+		    "curve25519-sha256@libssh.org");
>+		if (np == NULL)
> 			fatal("match_filter_blacklist failed");
>+		free(p);
>+		p = np;
>+	}
> 	if ((datafellows & SSH_OLD_DHGEX) != 0) {
>-		if ((p = match_filter_blacklist(p,
>+		char *np = match_filter_blacklist(p,
> 		    "diffie-hellman-group-exchange-sha256,"
>-		    "diffie-hellman-group-exchange-sha1")) == NULL)
>+		    "diffie-hellman-group-exchange-sha1");
>+		if (np == NULL)
> 			fatal("match_filter_blacklist failed");
>+		free(p);
>+		p = np;
> 	}
> 	debug2("%s: compat KEX proposal: %s", __func__, p);
> 	if (*p == '\0')
>-- 
>2.17.1
>
>
>From cc43fe84e1b40a12bbfe0ed8a469423b305b6f2b Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:52:04 +0200
>Subject: [PATCH 02/11] auth-options: Avoid OOB access in case of alloc failure
>
>---
> auth-options.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/auth-options.c b/auth-options.c
>index 27c0eb05..32e9bda1 100644
>--- a/auth-options.c
>+++ b/auth-options.c
>@@ -779,6 +779,7 @@ deserialise_array(struct sshbuf *m, char ***ap, size_t *np)
> 	n = tmp;
> 	if (n > 0 && (a = calloc(n, sizeof(*a))) == NULL) {
> 		r = SSH_ERR_ALLOC_FAIL;
>+		n = 0;
> 		goto out;
> 	}
> 	for (i = 0; i < n; i++) {
>-- 
>2.17.1
>
>
>From e93f22cd5a5a1839e056720bb9f51b06f0b3f36c Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:52:24 +0200
>Subject: [PATCH 03/11] port-linux.c: Missing include
>
>---
> openbsd-compat/port-linux.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/openbsd-compat/port-linux.c b/openbsd-compat/port-linux.c
>index 34e08a80..4550139c 100644
>--- a/openbsd-compat/port-linux.c
>+++ b/openbsd-compat/port-linux.c
>@@ -26,6 +26,7 @@
> #include <stdarg.h>
> #include <string.h>
> #include <stdio.h>
>+#include <stdlib.h>
> 
> #include "log.h"
> #include "xmalloc.h"
>-- 
>2.17.1
>
>
>From 341915b9fb44599ab89e04a92b2771ed5c04726a Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:52:54 +0200
>Subject: [PATCH 04/11] setproctitle: Make sure the len is initilized
>
>---
> openbsd-compat/setproctitle.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/openbsd-compat/setproctitle.c b/openbsd-compat/setproctitle.c
>index 2b15c6e0..dbd1a95a 100644
>--- a/openbsd-compat/setproctitle.c
>+++ b/openbsd-compat/setproctitle.c
>@@ -125,7 +125,7 @@ setproctitle(const char *fmt, ...)
> #if SPT_TYPE != SPT_NONE
> 	va_list ap;
> 	char buf[1024], ptitle[1024];
>-	size_t len;
>+	size_t len = 0;
> 	int r;
> 	extern char *__progname;
> #if SPT_TYPE == SPT_PSTAT
>-- 
>2.17.1
>
>
>From c304bf93f030972270b0de44a4e897e9ec7f6811 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:53:38 +0200
>Subject: [PATCH 05/11] packet: Make sure the ms_remain is initialized in later
> calls
>
>---
> packet.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/packet.c b/packet.c
>index dcf35e6e..7f698457 100644
>--- a/packet.c
>+++ b/packet.c
>@@ -1282,7 +1282,7 @@ int
> ssh_packet_read_seqnr(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p)
> {
> 	struct session_state *state = ssh->state;
>-	int len, r, ms_remain;
>+	int len, r, ms_remain = 0;
> 	fd_set *setp;
> 	char buf[8192];
> 	struct timeval timeout, start, *timeoutp = NULL;
>-- 
>2.17.1
>
>
>From 50d5cc1ad70b8eedeeb5b18f7d292c98640e9f19 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:54:10 +0200
>Subject: [PATCH 06/11] session: Avoid memory leak
>
>---
> session.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/session.c b/session.c
>index e47f8e69..f61a997e 100644
>--- a/session.c
>+++ b/session.c
>@@ -1183,6 +1183,7 @@ do_setup_env(struct ssh *ssh, Session *s, const char *shell)
> 		}
> 		*value++ = '\0';
> 		child_set_env(&env, &envsize, cp, value);
>+		free(cp);
> 	}
> 
> 	/* SSH_CLIENT deprecated */
>-- 
>2.17.1
>
>
>From 6e6a0914e4ce93f81866dfc82626a5b9ca739fa7 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:54:44 +0200
>Subject: [PATCH 07/11] sftp: Explicitly note fallthrough in the switch
>
>---
> sftp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/sftp.c b/sftp.c
>index d068f7e0..2567ecd7 100644
>--- a/sftp.c
>+++ b/sftp.c
>@@ -1443,6 +1443,7 @@ parse_args(const char **cpp, int *ignore_errors, int *aflag,
> 	case I_LUMASK:
> 	case I_CHMOD:
> 		base = 8;
>+		/* FALLTHROUGH */
> 	case I_CHOWN:
> 	case I_CHGRP:
> 		if ((optidx = parse_no_flags(cmd, argv, argc)) == -1)
>@@ -1541,6 +1542,7 @@ parse_dispatch_command(struct sftp_conn *conn, const char *cmd, char **pwd,
> 		break;
> 	case I_SYMLINK:
> 		sflag = 1;
>+		/* FALLTHROUGH */
> 	case I_LINK:
> 		if (!sflag)
> 			path1 = make_absolute(path1, *pwd);
>-- 
>2.17.1
>
>
>From e640847a80a05cc1db12ac9131ceb8d5e9be4d3d Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 14:54:59 +0200
>Subject: [PATCH 08/11] sshd: Avoid memory leak
>
>---
> sshd.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/sshd.c b/sshd.c
>index fbf31b4f..13ecf980 100644
>--- a/sshd.c
>+++ b/sshd.c
>@@ -2275,6 +2275,7 @@ static void
> do_ssh2_kex(void)
> {
> 	char *myproposal[PROPOSAL_MAX] = { KEX_SERVER };
>+	char *algs = NULL;
> 	struct kex *kex;
> 	int r;
> 
>@@ -2296,8 +2297,9 @@ do_ssh2_kex(void)
> 		packet_set_rekey_limits(options.rekey_limit,
> 		    options.rekey_interval);
> 
>-	myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = compat_pkalg_proposal(
>-	    list_hostkey_types());
>+	algs = list_hostkey_types();
>+	myproposal[PROPOSAL_SERVER_HOST_KEY_ALGS] = compat_pkalg_proposal(algs);
>+	free(algs);
> 
> #ifdef GSSAPI
> 	{
>-- 
>2.17.1
>
>
>From 324ae07b393e41f9aafe3eb3d507d3770b62dbe8 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 15:02:02 +0200
>Subject: [PATCH 09/11] channels: Avoid memory leaks -- permission_set_add()
> already strdups the arguments
>
>---
> channels.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/channels.c b/channels.c
>index e90f7fea..2af62343 100644
>--- a/channels.c
>+++ b/channels.c
>@@ -3830,18 +3830,18 @@ channel_request_remote_forwarding(struct ssh *ssh, struct Forward *fwd)
> 		host_to_connect = listen_host = listen_path = NULL;
> 		port_to_connect = listen_port = 0;
> 		if (fwd->connect_path != NULL) {
>-			host_to_connect = xstrdup(fwd->connect_path);
>+			host_to_connect = fwd->connect_path;
> 			port_to_connect = PORT_STREAMLOCAL;
> 		} else {
>-			host_to_connect = xstrdup(fwd->connect_host);
>+			host_to_connect = fwd->connect_host;
> 			port_to_connect = fwd->connect_port;
> 		}
> 		if (fwd->listen_path != NULL) {
>-			listen_path = xstrdup(fwd->listen_path);
>+			listen_path = fwd->listen_path;
> 			listen_port = PORT_STREAMLOCAL;
> 		} else {
> 			if (fwd->listen_host != NULL)
>-				listen_host = xstrdup(fwd->listen_host);
>+				listen_host = fwd->listen_host;
> 			listen_port = fwd->listen_port;
> 		}
> 		idx = permission_set_add(ssh, FORWARD_USER, FORWARD_LOCAL,
>-- 
>2.17.1
>
>
>From a3c6626b39bf049ab9ea7e9dc1fa8caea18df4ad Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 15:12:01 +0200
>Subject: [PATCH 10/11] auth-pam: Avoid memory leak of buffer
>
>---
> auth-pam.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
>diff --git a/auth-pam.c b/auth-pam.c
>index d6423e9f..dffa3e0d 100644
>--- a/auth-pam.c
>+++ b/auth-pam.c
>@@ -788,13 +788,13 @@ sshpam_query(void *ctx, char **name, char **info,
>     u_int *num, char ***prompts, u_int **echo_on)
> {
> 	struct ssh *ssh = active_state; /* XXX */
>-	struct sshbuf *buffer;
>+	struct sshbuf *buffer = NULL;
> 	struct pam_ctxt *ctxt = ctx;
> 	size_t plen;
> 	u_char type;
>-	char *msg;
>+	char *msg = NULL;
> 	size_t len, mlen;
>-	int r;
>+	int r, rv = -1;
> 
> 	debug3("PAM: %s entering", __func__);
> 	if ((buffer = sshbuf_new()) == NULL)
>@@ -819,7 +819,8 @@ sshpam_query(void *ctx, char **name, char **info,
> 			plen += mlen;
> 			**echo_on = (type == PAM_PROMPT_ECHO_ON);
> 			free(msg);
>-			return (0);
>+			rv = 0;
>+			goto out;
> 		case PAM_ERROR_MSG:
> 		case PAM_TEXT_INFO:
> 			/* accumulate messages */
>@@ -846,8 +847,8 @@ sshpam_query(void *ctx, char **name, char **info,
> 				*num = 0;
> 				**echo_on = 0;
> 				ctxt->pam_done = -1;
>-				free(msg);
>-				return 0;
>+				rv = 0;
>+				goto out;
> 			}
> 			/* FALLTHROUGH */
> 		case PAM_SUCCESS:
>@@ -873,7 +874,8 @@ sshpam_query(void *ctx, char **name, char **info,
> 				**echo_on = 0;
> 				ctxt->pam_done = 1;
> 				free(msg);
>-				return (0);
>+				rv = 0;
>+				goto out;
> 			}
> 			error("PAM: %s for %s%.100s from %.100s", msg,
> 			    sshpam_authctxt->valid ? "" : "illegal user ",
>@@ -885,10 +887,12 @@ sshpam_query(void *ctx, char **name, char **info,
> 			**echo_on = 0;
> 			free(msg);
> 			ctxt->pam_done = -1;
>-			return (-1);
>+			goto out;
> 		}
> 	}
>-	return (-1);
>+out:
>+	sshbuf_free(buffer);
>+	return (rv);
> }
> 
> /*
>-- 
>2.17.1
>
>
>From 58f112876e339e1fe97baff1d87c68125c15edc7 Mon Sep 17 00:00:00 2001
>From: Jakub Jelen <jjelen@redhat.com>
>Date: Tue, 28 Aug 2018 15:13:07 +0200
>Subject: [PATCH 11/11] auth-options: Avoid memory leaks
>
>---
> auth-options.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/auth-options.c b/auth-options.c
>index 32e9bda1..bb4410e7 100644
>--- a/auth-options.c
>+++ b/auth-options.c
>@@ -333,6 +333,7 @@ handle_permit(const char **optsp, int allow_bare_port,
> 		 * listen_host wildcard.
> 		 */
> 		if (asprintf(&tmp, "*:%s", opt) < 0) {
>+			free(opt);
> 			*errstrp = "memory allocation failed";
> 			return -1;
> 		}
>-- 
>2.17.1
>

Actions: View | Diff

Attachments on bug 2687: 2953 | 2954 | 3176 | 3287