session: Fix a possible use after free in ssh_free().

We need to cleanup the channels first cause we call ssh_channel_close()
on the channels which still require a working socket and poll context.

Thanks to sh4rm4!

1 files changed, 54 insertions, 33 deletions

diff --git a/src/session.c b/src/session.c
index 5a7a07c3..4e713948 100644
--- a/src/session.c
+++ b/src/session.c
@@ -166,37 +166,51 @@ void ssh_free(ssh_session session) {
   if (session == NULL) {
     return;
   }
-  enter_function();
 
-  SAFE_FREE(session->serverbanner);
-  SAFE_FREE(session->clientbanner);
-  SAFE_FREE(session->banner);
+  /*
+   * Delete all channels
+   *
+   * This needs the first thing we clean up cause if there is still an open
+   * channel we call ssh_channel_close() first. So we need a working socket
+   * and poll context for it.
+   */
+  for (it = ssh_list_get_iterator(session->channels);
+       it != NULL;
+       it = ssh_list_get_iterator(session->channels)) {
+      ssh_channel_do_free(ssh_iterator_value(ssh_channel,it));
+      ssh_list_remove(session->channels, it);
+  }
+  ssh_list_free(session->channels);
+  session->channels = NULL;
+
 #ifdef WITH_PCAP
-  if(session->pcap_ctx){
-  	ssh_pcap_context_free(session->pcap_ctx);
-  	session->pcap_ctx=NULL;
+  if (session->pcap_ctx) {
+      ssh_pcap_context_free(session->pcap_ctx);
+      session->pcap_ctx = NULL;
   }
 #endif
+
+  ssh_socket_free(session->socket);
+  session->socket = NULL;
+
+  if (session->default_poll_ctx) {
+      ssh_poll_ctx_free(session->default_poll_ctx);
+  }
+
   ssh_buffer_free(session->in_buffer);
   ssh_buffer_free(session->out_buffer);
-  if(session->in_hashbuf != NULL)
-    ssh_buffer_free(session->in_hashbuf);
-  if(session->out_hashbuf != NULL)
-    ssh_buffer_free(session->out_hashbuf);
-  session->in_buffer=session->out_buffer=NULL;
-  crypto_free(session->current_crypto);
-  crypto_free(session->next_crypto);
-  ssh_socket_free(session->socket);
-  if(session->default_poll_ctx){
-  	ssh_poll_ctx_free(session->default_poll_ctx);
+  session->in_buffer = session->out_buffer = NULL;
+
+  if (session->in_hashbuf != NULL) {
+      ssh_buffer_free(session->in_hashbuf);
   }
-  /* delete all channels */
-  while ((it=ssh_list_get_iterator(session->channels)) != NULL) {
-    ssh_channel_do_free(ssh_iterator_value(ssh_channel,it));
-    ssh_list_remove(session->channels, it);
+  if (session->out_hashbuf != NULL) {
+      ssh_buffer_free(session->out_hashbuf);
   }
-  ssh_list_free(session->channels);
-  session->channels=NULL;
+
+  crypto_free(session->current_crypto);
+  crypto_free(session->next_crypto);
+
 #ifndef _WIN32
   agent_free(session->agent);
 #endif /* _WIN32 */
@@ -204,30 +218,37 @@ void ssh_free(ssh_session session) {
   ssh_key_free(session->srv.dsa_key);
   ssh_key_free(session->srv.rsa_key);
 
-  if(session->ssh_message_list){
-    ssh_message msg;
-    while((msg=ssh_list_pop_head(ssh_message ,session->ssh_message_list))
-        != NULL){
-      ssh_message_free(msg);
-    }
-    ssh_list_free(session->ssh_message_list);
+  if (session->ssh_message_list) {
+      ssh_message msg;
+
+      for (msg = ssh_list_pop_head(ssh_message, session->ssh_message_list);
+           msg != NULL;
+           msg = ssh_list_pop_head(ssh_message, session->ssh_message_list)) {
+          ssh_message_free(msg);
+      }
+      ssh_list_free(session->ssh_message_list);
   }
 
-  if (session->packet_callbacks)
+  if (session->packet_callbacks) {
     ssh_list_free(session->packet_callbacks);
+  }
 
   /* options */
   if (session->opts.identity) {
       char *id;
 
       for (id = ssh_list_pop_head(char *, session->opts.identity);
-              id != NULL;
-              id = ssh_list_pop_head(char *, session->opts.identity)) {
+           id != NULL;
+           id = ssh_list_pop_head(char *, session->opts.identity)) {
           SAFE_FREE(id);
       }
       ssh_list_free(session->opts.identity);
   }
 
+  SAFE_FREE(session->serverbanner);
+  SAFE_FREE(session->clientbanner);
+  SAFE_FREE(session->banner);
+
   SAFE_FREE(session->opts.bindaddr);
   SAFE_FREE(session->opts.username);
   SAFE_FREE(session->opts.host);