From 2e0145fb95bbe2b379412f661494c3954b1c21a1 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Dec 2011 05:23:43 +0800 Subject: [PATCH 1/5] - We don't need to test for NULL before free() --- dbutil.c | 6 ------ dbutil.h | 3 +-- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/dbutil.c b/dbutil.c index 8823ab6..5460799 100644 --- a/dbutil.c +++ b/dbutil.c @@ -800,12 +800,6 @@ void * m_strdup(const char * str) { return ret; } -void __m_free(void* ptr) { - if (ptr != NULL) { - free(ptr); - } -} - void * m_realloc(void* ptr, size_t size) { void *ret; diff --git a/dbutil.h b/dbutil.h index 14c4c28..0f16bf3 100644 --- a/dbutil.h +++ b/dbutil.h @@ -83,8 +83,7 @@ void m_close(int fd); void * m_malloc(size_t size); void * m_strdup(const char * str); void * m_realloc(void* ptr, size_t size); -#define m_free(X) __m_free(X); (X) = NULL; -void __m_free(void* ptr); +#define m_free(X) free(X); (X) = NULL; void m_burn(void* data, unsigned int len); void setnonblocking(int fd); void disallow_core(); From fd0b05943df886d71a20219e9ff6baa900b0eb8f Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Dec 2011 05:24:50 +0800 Subject: [PATCH 2/5] - Fix some format strings in TRACE()s --- signkey.c | 4 ++-- svr-x11fwd.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/signkey.c b/signkey.c index c53805a..1d908f4 100644 --- a/signkey.c +++ b/signkey.c @@ -105,11 +105,11 @@ int buf_get_pub_key(buffer *buf, sign_key *key, int *type) { m_free(ident); if (*type != DROPBEAR_SIGNKEY_ANY && *type != keytype) { - TRACE(("buf_get_pub_key bad type - got %d, expected %d", keytype, type)) + TRACE(("buf_get_pub_key bad type - got %d, expected %d", keytype, *type)) return DROPBEAR_FAILURE; } - TRACE(("buf_get_pub_key keytype is %d")) + TRACE(("buf_get_pub_key keytype is %d", keytype)) *type = keytype; diff --git a/svr-x11fwd.c b/svr-x11fwd.c index 1af027b..92dadd5 100644 --- a/svr-x11fwd.c +++ b/svr-x11fwd.c @@ -175,7 +175,7 @@ void x11cleanup(struct ChanSess *chansess) { m_free(chansess->x11authprot); m_free(chansess->x11authcookie); - TRACE(("chansess %s", chansess)) + TRACE(("chansess %x", chansess)) if (chansess->x11listener != NULL) { remove_listener(chansess->x11listener); chansess->x11listener = NULL; From baa32218b0df8bd342da9bfe04f7ae678f2664ff Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Dec 2011 05:27:29 +0800 Subject: [PATCH 3/5] - Make sure we don't use channel-specific data after it has been freed with a ChanType->closehandler() --- channel.h | 4 ++++ common-channel.c | 21 ++++++++++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/channel.h b/channel.h index 5c63226..d9e2894 100644 --- a/channel.h +++ b/channel.h @@ -69,6 +69,10 @@ struct Channel { int sent_close, recv_close; int recv_eof, sent_eof; + /* Set after running the ChanType-specific close hander + * to ensure we don't run it twice (nor type->checkclose()). */ + int close_handler_done; + int initconn; /* used for TCP forwarding, whether the channel has been fully initialised */ diff --git a/common-channel.c b/common-channel.c index 5821b08..9eaba50 100644 --- a/common-channel.c +++ b/common-channel.c @@ -138,6 +138,7 @@ struct Channel* newchannel(unsigned int remotechan, newchan->index = i; newchan->sent_close = newchan->recv_close = 0; newchan->sent_eof = newchan->recv_eof = 0; + newchan->close_handler_done = 0; newchan->remotechan = remotechan; newchan->transwindow = transwindow; @@ -270,7 +271,9 @@ static void check_close(struct Channel *channel) { cbuf_getused(channel->writebuf), channel->extrabuf ? cbuf_getused(channel->extrabuf) : 0)) - if (!channel->flushing && channel->type->check_close + if (!channel->flushing + && !channel->close_handler_done + && channel->type->check_close && channel->type->check_close(channel)) { channel->flushing = 1; @@ -281,7 +284,8 @@ static void check_close(struct Channel *channel) { channel, to ensure that the shell has exited (and the exit status retrieved) before we close things up. */ if (!channel->type->check_close - || channel->type->check_close(channel)) { + || channel->close_handler_done + || channel->type->check_close(channel)) { close_allowed = 1; } @@ -363,9 +367,11 @@ static void check_in_progress(struct Channel *channel) { /* Send the close message and set the channel as closed */ static void send_msg_channel_close(struct Channel *channel) { - TRACE(("enter send_msg_channel_close")) - if (channel->type->closehandler) { + TRACE(("enter send_msg_channel_close %p", channel)) + if (channel->type->closehandler + && !channel->close_handler_done) { channel->type->closehandler(channel); + channel->close_handler_done = 1; } CHECKCLEARTOWRITE(); @@ -568,16 +574,17 @@ void recv_msg_channel_request() { struct Channel *channel; - TRACE(("enter recv_msg_channel_request")) - channel = getchannel(); + TRACE(("enter recv_msg_channel_request %p", channel)) + if (channel->sent_close) { TRACE(("leave recv_msg_channel_request: already closed channel")) return; } - if (channel->type->reqhandler) { + if (channel->type->reqhandler + && !channel->close_handler_done) { channel->type->reqhandler(channel); } else { send_msg_channel_failure(channel); From 52a466b8afa32a1355dcbc90936eedcf5d740b1d Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Dec 2011 05:27:57 +0800 Subject: [PATCH 4/5] - Remove unused variable/code --- cli-agentfwd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli-agentfwd.c b/cli-agentfwd.c index c9ce833..a821305 100644 --- a/cli-agentfwd.c +++ b/cli-agentfwd.c @@ -260,7 +260,7 @@ void agent_buf_sign(buffer *sigblob, sign_key *key, const unsigned char *data, unsigned int len) { buffer *request_data = NULL; buffer *response = NULL; - unsigned int keylen, siglen; + unsigned int siglen; int packet_type; /* Request format @@ -271,7 +271,6 @@ void agent_buf_sign(buffer *sigblob, sign_key *key, */ request_data = buf_new(MAX_PUBKEY_SIZE + len + 12); buf_put_pub_key(request_data, key, key->type); - keylen = request_data->len - 4; buf_putstring(request_data, data, len); buf_putint(request_data, 0); From aec23e5f791b78ed195358057f07f3050ab5bf94 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sun, 4 Dec 2011 05:31:25 +0800 Subject: [PATCH 5/5] - Fix use-after-free if multiple command requests were sent. Move the original_command into chansess struct since that makes more sense --- auth.h | 1 - chansession.h | 4 ++++ svr-authpubkeyoptions.c | 13 +++++++------ svr-chansession.c | 12 ++++++++---- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/auth.h b/auth.h index 7ebf9ae..0fd9c73 100644 --- a/auth.h +++ b/auth.h @@ -133,7 +133,6 @@ struct PubKeyOptions { int no_pty_flag; /* "command=" option. */ unsigned char * forced_command; - unsigned char * original_command; }; #endif diff --git a/chansession.h b/chansession.h index 924518b..ef252ea 100644 --- a/chansession.h +++ b/chansession.h @@ -69,6 +69,10 @@ struct ChanSess { char * agentfile; char * agentdir; #endif + +#ifdef ENABLE_SVR_PUBKEY_OPTIONS + char *original_command; +#endif }; struct ChildPid { diff --git a/svr-authpubkeyoptions.c b/svr-authpubkeyoptions.c index fd87703..4490b58 100644 --- a/svr-authpubkeyoptions.c +++ b/svr-authpubkeyoptions.c @@ -92,14 +92,15 @@ int svr_pubkey_allows_pty() { * by any 'command' public key option. */ void svr_pubkey_set_forced_command(struct ChanSess *chansess) { if (ses.authstate.pubkey_options) { - ses.authstate.pubkey_options->original_command = chansess->cmd; - if (!chansess->cmd) - { - ses.authstate.pubkey_options->original_command = m_strdup(""); + if (chansess->cmd) { + /* original_command takes ownership */ + chansess->original_command = chansess->cmd; + } else { + chansess->original_command = m_strdup(""); } - chansess->cmd = ses.authstate.pubkey_options->forced_command; + chansess->cmd = m_strdup(ses.authstate.pubkey_options->forced_command); #ifdef LOG_COMMANDS - dropbear_log(LOG_INFO, "Command forced to '%s'", ses.authstate.pubkey_options->original_command); + dropbear_log(LOG_INFO, "Command forced to '%s'", chansess->original_command); #endif } } diff --git a/svr-chansession.c b/svr-chansession.c index 0b3e833..53790d1 100644 --- a/svr-chansession.c +++ b/svr-chansession.c @@ -217,6 +217,8 @@ static int newchansess(struct Channel *channel) { struct ChanSess *chansess; + TRACE(("new chansess %p", channel)) + dropbear_assert(channel->typedata == NULL); chansess = (struct ChanSess*)m_malloc(sizeof(struct ChanSess)); @@ -279,6 +281,10 @@ static void closechansess(struct Channel *channel) { m_free(chansess->cmd); m_free(chansess->term); +#ifdef ENABLE_SVR_PUBKEY_OPTIONS + m_free(chansess->original_command); +#endif + if (chansess->tty) { /* write the utmp/wtmp login record */ li = chansess_login_alloc(chansess); @@ -924,10 +930,8 @@ static void execchild(void *user_data) { } #ifdef ENABLE_SVR_PUBKEY_OPTIONS - if (ses.authstate.pubkey_options && - ses.authstate.pubkey_options->original_command) { - addnewvar("SSH_ORIGINAL_COMMAND", - ses.authstate.pubkey_options->original_command); + if (chansess->original_command) { + addnewvar("SSH_ORIGINAL_COMMAND", chansess->original_command); } #endif