From Robert Bullen via https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8643

When a TCP segment contains the end of two or more SSL PDUs, the TCP reassembly
code passes that segment up to the SSL dissector multiple times--one for each
SSL PDU. The SSL dissector queues the packet for SSL tap listeners each time it
is invoked. Therefore a single packet can be processed by SSL tap listeners
multiple times. But the tap data that the SSL dissector sends to its tap
listeners is a linked list of all PDUs in the packet.

The SSL tap listener responsible for populating the Follow SSL Stream dialog
did not account for the possibility of seeing a packet multiple times. As a
result, it would process the entire linked list of PDUs each time it received a
packet, and that would result in some SSL PDUs showing up two or more times in
the dialog.

This patch fixes the described bug. It also implements a few slight
improvements in closely related code. See bugzilla for details.


svn path=/trunk/; revision=49387

3 files changed, 139 insertions, 129 deletions

diff --git a/ui/gtk/follow_ssl.c b/ui/gtk/follow_ssl.c
index 18378c847b..5ec7e36802 100644
--- a/ui/gtk/follow_ssl.c
+++ b/ui/gtk/follow_ssl.c
@@ -68,61 +68,65 @@
 
 
 typedef struct {
-    gboolean is_server;
+    gboolean   is_from_server;
     StringInfo data;
 } SslDecryptedRecord;
 
 static int
 ssl_queue_packet_data(void *tapdata, packet_info *pinfo, epan_dissect_t *edt _U_, const void *ssl)
 {
-    follow_info_t* follow_info = (follow_info_t*)tapdata;
-    SslDecryptedRecord* rec;
-    SslDataInfo* appl_data;
-    gint total_len;
-    guchar *p;
-    int proto_ssl = GPOINTER_TO_INT(ssl);
-    SslPacketInfo* pi = (SslPacketInfo*)p_get_proto_data(pinfo->fd, proto_ssl, 0);
-
-    /* skip packet without decrypted data payload*/
-    if (!pi || !pi->appl_data)
-        return 0;
-
-    /* compute total length */
-    total_len = 0;
-    appl_data = pi->appl_data;
-    do {
-      total_len += appl_data->plain_data.data_len;
-      appl_data = appl_data->next;
-    } while (appl_data);
-
-    /* compute packet direction */
-    rec = (SslDecryptedRecord*)g_malloc(sizeof(SslDecryptedRecord) + total_len);
-
+    follow_info_t *      follow_info = (follow_info_t*) tapdata;
+    SslDecryptedRecord * rec = NULL;
+    SslDataInfo *        appl_data = NULL;
+    int                  proto_ssl = GPOINTER_TO_INT(ssl);
+    SslPacketInfo *      pi = NULL;
+    show_stream_t        from = FROM_CLIENT;
+
+    /* Skip packets without decrypted payload data. */
+    pi = (SslPacketInfo*) p_get_proto_data(pinfo->fd, proto_ssl, 0);
+    if (!pi || !pi->appl_data) return 0;
+
+    /* Compute the packet's sender. */
     if (follow_info->client_port == 0) {
         follow_info->client_port = pinfo->srcport;
         COPY_ADDRESS(&follow_info->client_ip, &pinfo->src);
     }
     if (ADDRESSES_EQUAL(&follow_info->client_ip, &pinfo->src) &&
-        follow_info->client_port == pinfo->srcport)
-        rec->is_server = 0;
-    else
-        rec->is_server = 1;
-
-    /* update stream counter */
-    follow_info->bytes_written[rec->is_server] += total_len;
-
-    /* extract decrypted data and queue it locally */
-    rec->data.data = (guchar*)(rec + 1);
-    rec->data.data_len = total_len;
-    appl_data = pi->appl_data;
-    p = rec->data.data;
-    do {
-      memcpy(p, appl_data->plain_data.data, appl_data->plain_data.data_len);
-      p += appl_data->plain_data.data_len;
-      appl_data = appl_data->next;
-    } while (appl_data);
-    follow_info->payload = g_list_append(
-        follow_info->payload,rec);
+            follow_info->client_port == pinfo->srcport) {
+        from = FROM_CLIENT;
+    } else {
+        from = FROM_SERVER;
+    }
+
+    for (appl_data = pi->appl_data; appl_data != NULL; appl_data = appl_data->next) {
+
+        /* TCP segments that contain the end of two or more SSL PDUs will be
+           queued to SSL taps for each of those PDUs. Therefore a single
+           packet could be processed by this SSL tap listener multiple times.
+           The following test handles that scenario by treating the
+           follow_info->bytes_written[] values as the next expected
+           appl_data->seq. Any appl_data instances that fall below that have
+           already been processed and must be skipped. */
+        if (appl_data->seq < follow_info->bytes_written[from]) continue;
+
+        /* Allocate a SslDecryptedRecord to hold the current appl_data
+           instance's decrypted data. Even though it would be possible to
+           consolidate multiple appl_data instances into a single rec, it is
+           beneficial to use a one-to-one mapping. This affords the Follow
+           Stream dialog view modes (ASCII, EBCDIC, Hex Dump, C Arrays, Raw)
+           the opportunity to accurately reflect SSL PDU boundaries. Currently
+           the Hex Dump view does by starting a new line, and the C Arrays
+           view does by starting a new array declaration. */
+        rec = (SslDecryptedRecord*) g_malloc(sizeof(SslDecryptedRecord) + appl_data->plain_data.data_len);
+        rec->is_from_server = from == FROM_SERVER;
+        rec->data.data = (guchar*) (rec + 1);
+        rec->data.data_len = appl_data->plain_data.data_len;
+        memcpy(rec->data.data, appl_data->plain_data.data, appl_data->plain_data.data_len);
+
+        /* Append the record to the follow_info structure. */
+        follow_info->payload = g_list_append(follow_info->payload, rec);
+        follow_info->bytes_written[from] += rec->data.data_len;
+    }
 
     return 0;
 }
@@ -137,18 +141,27 @@ packet_is_ssl(epan_dissect_t* edt);
 void
 follow_ssl_stream_cb(GtkWidget * w _U_, gpointer data _U_)
 {
-    GtkWidget	*filter_te, *filter_cm;
-    gchar	*follow_filter;
-    const gchar	*previous_filter;
-    int		filter_out_filter_len, previous_filter_len;
-    const char	*hostname0, *hostname1;
-    char	*port0, *port1;
-    gchar	*server_to_client_string = NULL;
-    gchar       *client_to_server_string = NULL;
-    gchar	*both_directions_string = NULL;
-    follow_stats_t stats;
-    follow_info_t *follow_info;
-    GString*    msg;
+    GtkWidget *     filter_te;
+    GtkWidget *     filter_cm;
+    gchar *         follow_filter;
+    const gchar *   previous_filter;
+    int             filter_out_filter_len;
+    int             previous_filter_len;
+    const char *    hostname0;
+    const char *    hostname1;
+    const char *    port0;
+    const char *    port1;
+    const char *    client_hostname;
+    const char *    server_hostname;
+    const char *    client_port;
+    const char *    server_port;
+    gchar *         server_to_client_string = NULL;
+    gchar *         client_to_server_string = NULL;
+    gchar *         both_directions_string = NULL;
+    const gchar *   single_direction_format = NULL;
+    follow_stats_t  stats;
+    follow_info_t * follow_info;
+    GString *       msg;
 
     /* we got ssl so we can follow */
     if (!packet_is_ssl(cfile.edt)) {
@@ -245,35 +258,34 @@ follow_ssl_stream_cb(GtkWidget * w _U_, gpointer data _U_)
 
     follow_info->is_ipv6 = stats.is_ipv6;
 
-   /* Both Stream Directions */
+    /* Generate the strings for the follow stream dialog's combo box,
+       starting with both directions... */
     both_directions_string = g_strdup_printf("Entire conversation (%u bytes)", follow_info->bytes_written[0] + follow_info->bytes_written[1]);
 
-    if(follow_info->client_port == stats.port[0]) {
-	    server_to_client_string =
-		    g_strdup_printf("%s:%s " UTF8_RIGHTWARDS_ARROW " %s:%s (%u bytes)",
-				    hostname0, port0,
-				    hostname1, port1,
-				    follow_info->bytes_written[0]);
-
-	    client_to_server_string =
-		    g_strdup_printf("%s:%s " UTF8_RIGHTWARDS_ARROW " %s:%s (%u bytes)",
-				    hostname1, port1,
-				    hostname0, port0,
-				    follow_info->bytes_written[1]);
+    /* ...and then the server-to-client and client-to-server directions. */
+    if (follow_info->client_port == stats.port[0]) {
+        server_hostname = hostname1;
+        server_port = port1;
+        client_hostname = hostname0;
+        client_port = port0;
     } else {
-	    server_to_client_string =
-		    g_strdup_printf("%s:%s " UTF8_RIGHTWARDS_ARROW " %s:%s (%u bytes)",
-				    hostname1, port1,
-				    hostname0, port0,
-				    follow_info->bytes_written[0]);
-
-	    client_to_server_string =
-		    g_strdup_printf("%s:%s " UTF8_RIGHTWARDS_ARROW " %s:%s (%u bytes)",
-				    hostname0, port0,
-				    hostname1, port1,
-				    follow_info->bytes_written[1]);
+        server_hostname = hostname0;
+        server_port = port0;
+        client_hostname = hostname1;
+        client_port = port1;
     }
 
+    single_direction_format = "%s:%s " UTF8_RIGHTWARDS_ARROW " %s:%s (%u bytes)";
+    server_to_client_string = g_strdup_printf(single_direction_format,
+        server_hostname, server_port,
+        client_hostname, client_port,
+        follow_info->bytes_written[0]);
+    client_to_server_string = g_strdup_printf(single_direction_format,
+        client_hostname, client_port,
+        server_hostname, server_port,
+        follow_info->bytes_written[1]);
+
+    /* Invoke the dialog. */
     follow_stream("Follow SSL Stream", follow_info, both_directions_string,
 		  server_to_client_string, client_to_server_string);
 
@@ -303,43 +315,41 @@ follow_ssl_stream_cb(GtkWidget * w _U_, gpointer data _U_)
  */
 frs_return_t
 follow_read_ssl_stream(follow_info_t *follow_info,
-		       gboolean (*print_line_fcn_p)(char *, size_t, gboolean, void *),
-		       void *arg)
+               gboolean (*print_line_fcn_p)(char *, size_t, gboolean, void *),
+               void *arg)
 {
-    guint32		global_client_pos = 0, global_server_pos = 0;
-    guint32		server_packet_count = 0;
-    guint32		client_packet_count = 0;
-    guint32		*global_pos;
-    gboolean		skip;
-    GList* cur;
-    frs_return_t        frs_return;
+    guint32      global_client_pos = 0, global_server_pos = 0;
+    guint32      server_packet_count = 0;
+    guint32      client_packet_count = 0;
+    guint32 *    global_pos;
+    GList *      cur;
+    frs_return_t frs_return;
 
     for (cur = follow_info->payload; cur; cur = g_list_next(cur)) {
-        SslDecryptedRecord* rec = (SslDecryptedRecord*)cur->data;
-	skip = FALSE;
-	if (!rec->is_server) {
-	    global_pos = &global_client_pos;
-	    if (follow_info->show_stream == FROM_SERVER) {
-		skip = TRUE;
-	    }
-	} else {
-	    global_pos = &global_server_pos;
-	    if (follow_info->show_stream == FROM_CLIENT) {
-		skip = TRUE;
-	    }
-	}
-
-        if (!skip) {
+        SslDecryptedRecord * rec = (SslDecryptedRecord*) cur->data;
+        gboolean             include_rec = FALSE;
+
+        if (rec->is_from_server) {
+            global_pos = &global_server_pos;
+            include_rec = (follow_info->show_stream == BOTH_HOSTS) ||
+                (follow_info->show_stream == FROM_SERVER);
+        } else {
+            global_pos = &global_client_pos;
+            include_rec = (follow_info->show_stream == BOTH_HOSTS) ||
+                (follow_info->show_stream == FROM_CLIENT);
+        }
+
+        if (include_rec) {
             size_t nchars = rec->data.data_len;
             gchar *buffer = (gchar *)g_memdup(rec->data.data, (guint) nchars);
 
-	    frs_return = follow_show(follow_info, print_line_fcn_p, buffer, nchars,
-				     rec->is_server, arg, global_pos,
-				     &server_packet_count, &client_packet_count);
-	    g_free(buffer);
-	    if(frs_return == FRS_PRINT_ERROR)
-		    return frs_return;
-	}
+            frs_return = follow_show(follow_info, print_line_fcn_p, buffer, nchars,
+                rec->is_from_server, arg, global_pos,
+                &server_packet_count, &client_packet_count);
+            g_free(buffer);
+            if (frs_return == FRS_PRINT_ERROR)
+                return frs_return;
+        }
     }
 
     return FRS_OK;
diff --git a/ui/gtk/follow_stream.c b/ui/gtk/follow_stream.c
index 2537cb4b59..c00839696a 100644
--- a/ui/gtk/follow_stream.c
+++ b/ui/gtk/follow_stream.c
@@ -100,7 +100,7 @@ follow_read_stream(follow_info_t *follow_info,
 }
 
 gboolean
-follow_add_to_gtk_text(char *buffer, size_t nchars, gboolean is_server,
+follow_add_to_gtk_text(char *buffer, size_t nchars, gboolean is_from_server,
 		       void *arg)
 {
 	GtkWidget *text = (GtkWidget *)arg;
@@ -123,7 +123,7 @@ follow_add_to_gtk_text(char *buffer, size_t nchars, gboolean is_server,
 	}
 
 	gtk_text_buffer_get_end_iter(buf, &iter);
-	if (is_server) {
+	if (is_from_server) {
 		gtk_text_buffer_insert_with_tags(buf, &iter, buffer, (gint) nchars,
 						 server_tag, NULL);
 	} else {
@@ -141,7 +141,7 @@ follow_add_to_gtk_text(char *buffer, size_t nchars, gboolean is_server,
  * suggestion.
  */
 static gboolean
-follow_print_text(char *buffer, size_t nchars, gboolean is_server _U_,
+follow_print_text(char *buffer, size_t nchars, gboolean is_from_server _U_,
 		  void *arg)
 {
 	print_stream_t *stream = (print_stream_t *)arg;
@@ -168,7 +168,7 @@ follow_print_text(char *buffer, size_t nchars, gboolean is_server _U_,
 }
 
 static gboolean
-follow_write_raw(char *buffer, size_t nchars, gboolean is_server _U_, void *arg)
+follow_write_raw(char *buffer, size_t nchars, gboolean is_from_server _U_, void *arg)
 {
 	FILE *fh = (FILE *)arg;
 	size_t nwritten;
@@ -636,11 +636,11 @@ follow_stream_direction_changed(GtkWidget *w, gpointer data)
 		follow_load_text(follow_info);
 		break;
 	case 1 :
-		follow_info->show_stream = FROM_CLIENT;
+		follow_info->show_stream = FROM_SERVER;
 		follow_load_text(follow_info);
 		break;
 	case 2 :
-		follow_info->show_stream = FROM_SERVER;
+		follow_info->show_stream = FROM_CLIENT;
 		follow_load_text(follow_info);
 		break;
 	}
@@ -932,7 +932,7 @@ follow_destroy_cb(GtkWidget *w, gpointer data _U_)
 frs_return_t
 follow_show(follow_info_t *follow_info,
 	    gboolean (*print_line_fcn_p)(char *, size_t, gboolean, void *),
-	    char *buffer, size_t nchars, gboolean is_server, void *arg,
+	    char *buffer, size_t nchars, gboolean is_from_server, void *arg,
 	    guint32 *global_pos, guint32 *server_packet_count,
 	    guint32 *client_packet_count)
 {
@@ -945,7 +945,7 @@ follow_show(follow_info_t *follow_info,
 	case SHOW_EBCDIC:
 		/* If our native arch is ASCII, call: */
 		EBCDIC_to_ASCII(buffer, (guint) nchars);
-		if (!(*print_line_fcn_p) (buffer, nchars, is_server, arg))
+		if (!(*print_line_fcn_p) (buffer, nchars, is_from_server, arg))
 			return FRS_PRINT_ERROR;
 		break;
 
@@ -953,7 +953,7 @@ follow_show(follow_info_t *follow_info,
 		/* If our native arch is EBCDIC, call:
 		 * ASCII_TO_EBCDIC(buffer, nchars);
 		 */
-		if (!(*print_line_fcn_p) (buffer, nchars, is_server, arg))
+		if (!(*print_line_fcn_p) (buffer, nchars, is_from_server, arg))
 			return FRS_PRINT_ERROR;
 		break;
 
@@ -961,7 +961,7 @@ follow_show(follow_info_t *follow_info,
 		/* Don't translate, no matter what the native arch
 		 * is.
 		 */
-		if (!(*print_line_fcn_p) (buffer, nchars, is_server, arg))
+		if (!(*print_line_fcn_p) (buffer, nchars, is_from_server, arg))
 			return FRS_PRINT_ERROR;
 		break;
 
@@ -972,10 +972,10 @@ follow_show(follow_info_t *follow_info,
 			int i;
 			gchar *cur = hexbuf, *ascii_start;
 
-			/* is_server indentation : put 4 spaces at the
+			/* is_from_server indentation : put 4 spaces at the
 			 * beginning of the string */
 			/* XXX - We might want to prepend each line with "C" or "S" instead. */
-			if (is_server && follow_info->show_stream == BOTH_HOSTS) {
+			if (is_from_server && follow_info->show_stream == BOTH_HOSTS) {
 				memset(cur, ' ', 4);
 				cur += 4;
 			}
@@ -1008,7 +1008,7 @@ follow_show(follow_info_t *follow_info,
 			(*global_pos) += i;
 			*cur++ = '\n';
 			*cur = 0;
-			if (!(*print_line_fcn_p) (hexbuf, strlen(hexbuf), is_server, arg))
+			if (!(*print_line_fcn_p) (hexbuf, strlen(hexbuf), is_from_server, arg))
 				return FRS_PRINT_ERROR;
 		}
 		break;
@@ -1016,9 +1016,9 @@ follow_show(follow_info_t *follow_info,
 	case SHOW_CARRAY:
 		current_pos = 0;
 		g_snprintf(initbuf, sizeof(initbuf), "char peer%d_%d[] = {\n",
-			   is_server ? 1 : 0,
-			   is_server ? (*server_packet_count)++ : (*client_packet_count)++);
-		if (!(*print_line_fcn_p) (initbuf, strlen(initbuf), is_server, arg))
+			   is_from_server ? 1 : 0,
+			   is_from_server ? (*server_packet_count)++ : (*client_packet_count)++);
+		if (!(*print_line_fcn_p) (initbuf, strlen(initbuf), is_from_server, arg))
 			return FRS_PRINT_ERROR;
 
 		while (current_pos < nchars) {
@@ -1052,7 +1052,7 @@ follow_show(follow_info_t *follow_info,
 			(*global_pos) += i;
 			hexbuf[cur++] = '\n';
 			hexbuf[cur] = 0;
-			if (!(*print_line_fcn_p) (hexbuf, strlen(hexbuf), is_server, arg))
+			if (!(*print_line_fcn_p) (hexbuf, strlen(hexbuf), is_from_server, arg))
 				return FRS_PRINT_ERROR;
 		}
 		break;
diff --git a/ui/gtk/follow_stream.h b/ui/gtk/follow_stream.h
index 5636438359..beba0d0219 100644
--- a/ui/gtk/follow_stream.h
+++ b/ui/gtk/follow_stream.h
@@ -80,7 +80,7 @@ typedef struct {
 	GtkWidget	*filter_te;
 	GtkWidget	*streamwindow;
         GList           *payload;
-        guint           bytes_written[2];
+        guint           bytes_written[2]; /* Index with FROM_CLIENT or FROM_SERVER for readability. */
         guint           client_port;
         address         client_ip;
 } follow_info_t;