From 85f22c9f098e853a6c6836d1af8afcee3ea6c4b7 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 09:42:22 +0000 Subject: [PATCH 1/7] Fix free() of null pointer found by Klocwork --HG-- extra : convert_revision : 8084d31816a059cc07c6180b6fd5aa86770845cb --- svr-tcpfwd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/svr-tcpfwd.c b/svr-tcpfwd.c index 6391c4c..0151ffe 100644 --- a/svr-tcpfwd.c +++ b/svr-tcpfwd.c @@ -216,8 +216,10 @@ out: if (ret == DROPBEAR_FAILURE) { /* we only free it if a listener wasn't created, since the listener * has to remember it if it's to be cancelled */ - m_free(tcpinfo->listenaddr); - m_free(tcpinfo); + if (tcpinfo) { + m_free(tcpinfo->listenaddr); + m_free(tcpinfo); + } } TRACE(("leave remotetcpreq")) return ret; From 28f1026de851c17b2b0bfbefaf07e2f52e29ac4b Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 09:58:14 +0000 Subject: [PATCH 2/7] Fix potential null pointer dereference found by Klokwork --HG-- extra : convert_revision : ef7030b29eca0944e6fbbdcdd776aafe39197ffa --- svr-session.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/svr-session.c b/svr-session.c index 70029f8..fe78bcc 100644 --- a/svr-session.c +++ b/svr-session.c @@ -181,10 +181,15 @@ void svr_dropbear_log(int priority, const char* format, va_list param) { if (!svr_opts.usingsyslog || havetrace) { + struct tm * local_tm = NULL; timesec = time(NULL); - if (strftime(datestr, sizeof(datestr), "%b %d %H:%M:%S", - localtime(×ec)) == 0) { - datestr[0] = '?'; datestr[1] = '\0'; + local_tm = localtime(×ec); + if (local_tm == NULL + || strftime(datestr, sizeof(datestr), "%b %d %H:%M:%S", + localtime(×ec)) == 0) + { + // upon failure, just print the epoch-seconds time. + snprintf(datestr, sizeof(datestr), "%d", timesec); } fprintf(stderr, "[%d] %s %s\n", getpid(), datestr, printbuf); } From d0533106a8a5bfb81f1e95931db5a1a71ee11006 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 13:23:18 +0000 Subject: [PATCH 3/7] Remove extraneous tests in random mpint generation, courtesy of Klocwork --HG-- extra : convert_revision : 2b5e1d92fd1ce08361e69155a525fca481e79fe4 --- random.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/random.c b/random.c index c836de9..36fedff 100644 --- a/random.c +++ b/random.c @@ -234,8 +234,7 @@ void gen_random_mpint(mp_int *max, mp_int *rand) { /* keep regenerating until we get one satisfying * 0 < rand < max */ - } while ( ( (max != NULL) && (mp_cmp(rand, max) != MP_LT) ) - || (mp_cmp_d(rand, 0) != MP_GT) ); + } while (mp_cmp(rand, max) != MP_LT); m_burn(randbuf, len); m_free(randbuf); } From d4bc0aec5d55a3df5fbb6f5a872d44c882ee71de Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 13:31:01 +0000 Subject: [PATCH 4/7] Fix leak of keybuf in recv_msg_userauth_pk_ok, courtesy of Klocwork --HG-- extra : convert_revision : 9c39c3f447a47f61606df5d4bee364a449f12e18 --- cli-authpubkey.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli-authpubkey.c b/cli-authpubkey.c index 9d36bc3..7ef25b4 100644 --- a/cli-authpubkey.c +++ b/cli-authpubkey.c @@ -60,8 +60,8 @@ void cli_pubkeyfail() { void recv_msg_userauth_pk_ok() { - struct SignKeyList *keyitem; - buffer* keybuf; + struct SignKeyList *keyitem = NULL; + buffer* keybuf = NULL; char* algotype = NULL; unsigned int algolen; int keytype; @@ -121,6 +121,8 @@ void recv_msg_userauth_pk_ok() { } else { TRACE(("That was whacky. We got told that a key was valid, but it didn't match our list. Sounds like dodgy code on Dropbear's part")) } + + buf_free(keybuf); TRACE(("leave recv_msg_userauth_pk_ok")) } From d2f292b3adec9bdb2bbb221b1d05f13f3da21be7 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 13:50:47 +0000 Subject: [PATCH 5/7] Fix another leak found by Klocwork --HG-- extra : convert_revision : f6ffa6544086d7088a04d6e94b3bfb1bfc67908d --- cli-runopts.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli-runopts.c b/cli-runopts.c index 54d4875..d8eada7 100644 --- a/cli-runopts.c +++ b/cli-runopts.c @@ -358,8 +358,7 @@ static void addforward(char* origstr, struct TCPFwdList** fwdlist) { TRACE(("connectaddr == NULL")) goto fail; } - - connectaddr[0] = '\0'; + *connectaddr = '\0'; connectaddr++; connectport = strchr(connectaddr, ':'); @@ -367,8 +366,7 @@ static void addforward(char* origstr, struct TCPFwdList** fwdlist) { TRACE(("connectport == NULL")) goto fail; } - - connectport[0] = '\0'; + *connectport = '\0'; connectport++; newfwd = (struct TCPFwdList*)m_malloc(sizeof(struct TCPFwdList)); @@ -402,6 +400,8 @@ static void addforward(char* origstr, struct TCPFwdList** fwdlist) { newfwd->next = *fwdlist; *fwdlist = newfwd; + m_free(str); + TRACE(("leave addforward: done")) return; From 16517e67603787b9c11c6feeb779b3edb0597ed2 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 3 Feb 2007 13:57:35 +0000 Subject: [PATCH 6/7] Fix failure-handling in dropbear_listen() when errstring is unset (this codepath isn't used) --HG-- extra : convert_revision : d29f3665a91c4bf71a2d8bd99b33348a0317e605 --- dbutil.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbutil.c b/dbutil.c index 15f51ba..e096797 100644 --- a/dbutil.c +++ b/dbutil.c @@ -286,9 +286,9 @@ int dropbear_listen(const char* address, const char* port, len = 20 + strlen(strerror(err)); *errstring = (char*)m_malloc(len); snprintf(*errstring, len, "Error listening: %s", strerror(err)); - TRACE(("leave dropbear_listen: failure, %s", strerror(err))) - return -1; } + TRACE(("leave dropbear_listen: failure, %s", strerror(err))) + return -1; } TRACE(("leave dropbear_listen: success, %d socks bound", nsock)) From 60d4cd599621a843095a0948c58c40b0ba286de2 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Feb 2007 10:31:48 +0000 Subject: [PATCH 7/7] Add comments about requiring keysize <= 2*SHA1_HASH_SIZE --HG-- extra : convert_revision : bcb33fce2fad01a7626598209d43af3571bd86f0 --- common-algo.c | 2 ++ common-kex.c | 8 +++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common-algo.c b/common-algo.c index ae2102a..21ac96a 100644 --- a/common-algo.c +++ b/common-algo.c @@ -31,6 +31,8 @@ /* Mappings for ciphers, parameters are {&cipher_desc, keysize, blocksize} */ +/* NOTE: if keysize > 2*SHA1_HASH_SIZE, code such as hashkeys() + needs revisiting */ #ifdef DROPBEAR_AES256_CBC static const struct dropbear_cipher dropbear_aes256 = diff --git a/common-kex.c b/common-kex.c index 5db8e52..5a48758 100644 --- a/common-kex.c +++ b/common-kex.c @@ -217,12 +217,10 @@ static void kexinitialise() { * already initialised hash_state hs, which should already have processed * the dh_K and hash, since these are common. X is the letter 'A', 'B' etc. * out must have at least min(SHA1_HASH_SIZE, outlen) bytes allocated. - * The output will only be expanded once, since that is all that is required - * (for 3DES and SHA, with 24 and 20 bytes respectively). + * The output will only be expanded once, as we are assured that + * outlen <= 2*SHA1_HASH_SIZE for all known hashes. * - * See Section 5.2 of the IETF secsh Transport Draft for details */ - -/* Duplicated verbatim from kex.c --mihnea */ + * See Section 7.2 of rfc4253 (ssh transport) for details */ static void hashkeys(unsigned char *out, int outlen, const hash_state * hs, const unsigned char X) {