Fix SOCKS5 state machine for authentication, two-pass dissection

1. SOCKSv5 dissection was broken if authentication was used since the
used state machine states were initialized, but not saved correctly, and
the first server state transition was wrong.
I also fixed the GSSAPI variant analogously, but could not verify this
since I have no traces of this.

2. SOCKSv5 actually has a different "subnegotiation field" for the
authentication messages. This is added, and the original SOCKS version
is added as a generated field (i.e. for filtering).

3. Info column setting is moved to the *_display_socks_* routines to fix
two-pass dissection (as used by GUI Wireshark), since only here the
correct per-packet state is used. The hash_info (as used by the
*_state_machine_* functions) is already fully populated on second pass.

Change-Id: Ib39434dafce08188cd2da347118d0509a7613915
Reviewed-on: https://code.wireshark.org/review/24712
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>

1 files changed, 76 insertions, 31 deletions

diff --git a/epan/dissectors/packet-socks.c b/epan/dissectors/packet-socks.c
index e2251c6741..26d683ec94 100644
--- a/epan/dissectors/packet-socks.c
+++ b/epan/dissectors/packet-socks.c
@@ -117,6 +117,7 @@ static int hf_client_port = -1;
 static int hf_server_accepted_auth_method = -1;
 static int hf_server_auth_status = -1;
 static int hf_server_remote_host_port = -1;
+static int hf_socks_subnegotiation_version = -1;
 static int hf_socks_username = -1;
 static int hf_socks_password = -1;
 static int hf_socks_remote_name = -1;
@@ -504,21 +505,23 @@ client_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo,
     unsigned int  i;
     const char   *AuthMethodStr;
     sock_state_t  new_state_info;
+    proto_item *ti;
 
     /* Either there is an error, or we're done with the state machine
       (so there's nothing to display) */
     if (state_info == NULL)
         return;
 
-    proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
-    offset += 1;
-
     if (state_info->client == clientStart)
     {
         proto_tree      *AuthTree;
-        proto_item      *ti;
         guint8 num_auth_methods, auth;
 
+        col_append_str(pinfo->cinfo, COL_INFO, " Connect to server request");
+
+        proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         AuthTree = proto_tree_add_subtree( tree, tvb, offset, -1, ett_socks_auth, &ti, "Client Authentication Methods");
 
         num_auth_methods = tvb_get_guint8(tvb, offset);
@@ -545,6 +548,11 @@ client_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo,
         }
     }
     else if (state_info->client == clientV5Command) {
+        col_append_fstr(pinfo->cinfo, COL_INFO, " Command Request - %s",
+                val_to_str_const(hash_info->command, cmd_strings, "Unknown"));
+
+        proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
 
         proto_tree_add_item( tree, hf_socks_cmd, tvb, offset, 1, ENC_BIG_ENDIAN);
         offset += 1;
@@ -560,11 +568,19 @@ client_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo,
         guint16 len;
         gchar* str;
 
+        ti = proto_tree_add_uint( tree, hf_socks_ver, tvb, offset, 0, 5);
+        PROTO_ITEM_SET_GENERATED(ti);
+
+        proto_tree_add_item( tree, hf_socks_subnegotiation_version, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         switch(hash_info->authentication_method)
         {
         case NO_AUTHENTICATION:
             break;
         case USER_NAME_AUTHENTICATION:
+            col_append_str(pinfo->cinfo, COL_INFO, " User authentication request");
+
             /* process user name */
             len = tvb_get_guint8(tvb, offset);
             str = tvb_get_string_enc(wmem_packet_scope(), tvb, offset+1, len, ENC_ASCII);
@@ -577,6 +593,8 @@ client_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo,
             /* offset += (len+1); */
             break;
         case GSS_API_AUTHENTICATION:
+            col_append_str(pinfo->cinfo, COL_INFO, " GSSAPI authentication request");
+
             proto_tree_add_item( tree, hf_gssapi_command, tvb, offset, 1, ENC_BIG_ENDIAN);
             proto_tree_add_item( tree, hf_gssapi_length, tvb, offset+1, 2, ENC_BIG_ENDIAN);
             len = tvb_get_ntohs(tvb, offset+1);
@@ -587,6 +605,11 @@ client_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo,
             break;
         }
     }
+    else {
+        if (hash_info->port != 0)
+            col_append_fstr(pinfo->cinfo, COL_INFO, ", Remote Port: %u",
+                hash_info->port);
+    }
 }
 
 static void
@@ -608,12 +631,14 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
     if (state_info == NULL)
         return;
 
-    proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
-    offset += 1;
-
     switch(state_info->server)
     {
     case serverStart:
+        col_append_str(pinfo->cinfo, COL_INFO, " Connect to server response");
+
+        proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         auth = tvb_get_guint8( tvb, offset);
         AuthMethodStr = get_auth_method_name(auth);
 
@@ -622,6 +647,14 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
         break;
 
     case serverUserReply:
+        col_append_str(pinfo->cinfo, COL_INFO, " User authentication reply");
+
+        ti = proto_tree_add_uint( tree, hf_socks_ver, tvb, offset, 0, 5);
+        PROTO_ITEM_SET_GENERATED(ti);
+
+        proto_tree_add_item( tree, hf_socks_subnegotiation_version, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         auth_status = tvb_get_guint8(tvb, offset);
         ti = proto_tree_add_item(tree, hf_server_auth_status, tvb, offset, 1, ENC_BIG_ENDIAN);
         if(auth_status != 0)
@@ -631,6 +664,14 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
         break;
 
     case serverGssApiReply:
+        col_append_str(pinfo->cinfo, COL_INFO, " GSSAPI authentication reply");
+
+        ti = proto_tree_add_uint( tree, hf_socks_ver, tvb, offset, 0, 5);
+        PROTO_ITEM_SET_GENERATED(ti);
+
+        proto_tree_add_item( tree, hf_socks_subnegotiation_version, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         auth_status = tvb_get_guint8(tvb, offset);
         proto_tree_add_item( tree, hf_gssapi_command, tvb, offset, 1, ENC_BIG_ENDIAN);
         if (auth_status != 0xFF) {
@@ -644,6 +685,12 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
         break;
 
     case serverCommandReply:
+        col_append_fstr(pinfo->cinfo, COL_INFO, " Command Response - %s",
+                val_to_str_const(hash_info->command, cmd_strings, "Unknown"));
+
+        proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         proto_tree_add_item( tree, hf_socks_results_5, tvb, offset, 1, ENC_BIG_ENDIAN);
         offset += 1;
 
@@ -655,6 +702,11 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
         break;
 
     case serverBindReply:
+        col_append_str(pinfo->cinfo, COL_INFO, " Command Response: Bind remote host info");
+
+        proto_tree_add_item( tree, hf_socks_ver, tvb, offset, 1, ENC_BIG_ENDIAN);
+        offset += 1;
+
         proto_tree_add_item( tree, hf_socks_results_5, tvb, offset, 1, ENC_BIG_ENDIAN);
         offset += 1;
 
@@ -664,7 +716,12 @@ server_display_socks_v5(tvbuff_t *tvb, int offset, packet_info *pinfo _U_,
         offset = display_address(tvb, offset, tree);
         proto_tree_add_item( tree, hf_server_remote_host_port, tvb, offset, 2, ENC_BIG_ENDIAN);
         break;
+
     default:
+        if ( hash_info->port != 0)
+            col_append_fstr(pinfo->cinfo, COL_INFO, ", Remote Port: %u",
+                hash_info->port);
+
         break;
     }
 }
@@ -722,13 +779,14 @@ client_state_machine_v5( socks_hash_entry_t *hash_info, tvbuff_t *tvb,
 /* list.  Based upon the current state, decode the packet and determine */
 /* what the next state should be. */
 
-    if (start_of_frame)
+    if (start_of_frame) {
         save_client_state(pinfo, hash_info->clientState);
+        save_server_state(pinfo, hash_info->serverState);
+    }
 
     if (hash_info->clientState == clientStart)
     {
         guint8 num_auth_methods;
-        col_append_str(pinfo->cinfo, COL_INFO, " Connect to server request");
 
         num_auth_methods = tvb_get_guint8(tvb, offset + 1);
                         /* skip past auth methods */
@@ -766,12 +824,8 @@ client_state_machine_v5( socks_hash_entry_t *hash_info, tvbuff_t *tvb,
             break;
         }
     } else if (hash_info->clientState == clientV5Command) {
-
         hash_info->command = tvb_get_guint8(tvb, offset + 1); /* get command */
 
-        col_append_fstr(pinfo->cinfo, COL_INFO, " Command Request - %s",
-                val_to_str_const(hash_info->command, cmd_strings, "Unknown"));
-
         offset += 3;            /* skip to address type */
 
         offset = get_address_v5(tvb, offset, hash_info);
@@ -800,23 +854,19 @@ server_state_machine_v5( socks_hash_entry_t *hash_info, tvbuff_t *tvb,
 
     switch (hash_info->serverState) {
     case serverStart:
-        col_append_str(pinfo->cinfo, COL_INFO, " Connect to server response");
-
         hash_info->authentication_method = tvb_get_guint8(tvb, offset + 1);
-        hash_info->serverState = serverInitReply;
-
         switch (hash_info->authentication_method)
         {
         case NO_AUTHENTICATION:
-            hash_info->serverState = serverCommandReply;
             /* If there is no authentication, client should expect command immediately */
+            hash_info->serverState = serverCommandReply;
             hash_info->clientState = clientV5Command;
             break;
         case USER_NAME_AUTHENTICATION:
-            hash_info->serverState = serverUserReply;
+            hash_info->serverState = serverInitReply;
             break;
         case GSS_API_AUTHENTICATION:
-            hash_info->serverState = serverGssApiReply;
+            hash_info->serverState = serverInitReply;
             break;
         default:
             hash_info->serverState = serverError;
@@ -824,22 +874,17 @@ server_state_machine_v5( socks_hash_entry_t *hash_info, tvbuff_t *tvb,
         }
         break;
     case serverUserReply:
-        col_append_str(pinfo->cinfo, COL_INFO, " User authentication reply");
+        hash_info->serverState = serverCommandReply;
         break;
     case serverGssApiReply:
         if (tvb_get_guint8(tvb, offset+1) == 0xFF) {
-            col_append_str(pinfo->cinfo, COL_INFO, " GSSAPI Authentication failure");
             hash_info->serverState = serverError;
         } else {
-            col_append_str(pinfo->cinfo, COL_INFO, " GSSAPI Authentication reply");
             if (tvb_get_ntohs(tvb, offset+2) == 0)
                 hash_info->serverState = serverCommandReply;
         }
         break;
     case serverCommandReply:
-        col_append_fstr(pinfo->cinfo, COL_INFO, " Command Response - %s",
-                val_to_str_const(hash_info->command, cmd_strings, "Unknown"));
-
         switch(hash_info->command)
         {
         case CONNECT_COMMAND:
@@ -871,7 +916,6 @@ server_state_machine_v5( socks_hash_entry_t *hash_info, tvbuff_t *tvb,
         }
         break;
     case serverBindReply:
-        col_append_str(pinfo->cinfo, COL_INFO, " Command Response: Bind remote host info");
         break;
     default:
         break;
@@ -1047,10 +1091,6 @@ dissect_socks(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void *data) {
     if ( hash_info->command == TRACERT_COMMAND)
         col_append_str(pinfo->cinfo, COL_INFO, ", Traceroute Req");
 
-    if ( hash_info->port != 0)
-        col_append_fstr(pinfo->cinfo, COL_INFO, ", Remote Port: %u",
-            hash_info->port);
-
     /* run state machine if needed */
     if ((!pinfo->fd->flags.visited) &&
         (!((hash_info->clientState == clientDone) &&
@@ -1251,6 +1291,11 @@ proto_register_socks( void){
                 BASE_DEC, NULL, 0x0, NULL, HFILL
             }
         },
+        { &hf_socks_subnegotiation_version,
+            { "Subnegotiation Version", "socks.subnegotation_version", FT_UINT8, BASE_DEC, NULL,
+                0x0, NULL, HFILL
+            }
+        },
         { &hf_socks_username,
             { "User name", "socks.username", FT_STRING, BASE_NONE,
                 NULL, 0x0, NULL, HFILL