ssl,dtls: move ClientHello to ssl-utils

Changes to ClientHello dissection:

 - Move ssl_find_private_key (and its pre-req, ssl_set_server) outside
   ssl_dissect_hnd_cli_hello. It has not really something to do with
   dissection, but state tracking and decoder param feeding.
 - dtls: add expert info for bad cipher suites len.
 - ssl: remove bad cipher suites len text label which is also
   available as expert info. Attach expert info to a the length proto
   item (which is converted to use add_item instead of add_uint).
 - Remove `if (tree || ssl)` since expert info seems not to apply
   otherwise (this also needs changes in common and handshake
   dissection).
 - ssl: remove tvb_ensure_bytes_exist so we can dissect more
   compression methods and cipher suites.
 - Since DTLS has an additional Cookie field which TLS does not have,
   pass these additional header fields through a struct whose type is
   defined in ssl-utils.

Change-Id: I41bef04c1c3353e582e30f561d1d246a744e1d60
Reviewed-on: https://code.wireshark.org/review/3021
Reviewed-by: Evan Huus <eapache@gmail.com>

4 files changed, 205 insertions, 365 deletions

diff --git a/epan/dissectors/packet-dtls.c b/epan/dissectors/packet-dtls.c
index 158cbb4392..f060a8baef 100644
--- a/epan/dissectors/packet-dtls.c
+++ b/epan/dissectors/packet-dtls.c
@@ -104,13 +104,6 @@ static gint hf_dtls_handshake_length            = -1;
 static gint hf_dtls_handshake_message_seq       = -1;
 static gint hf_dtls_handshake_fragment_offset   = -1;
 static gint hf_dtls_handshake_fragment_length   = -1;
-static gint hf_dtls_handshake_client_version    = -1;
-static gint hf_dtls_handshake_cookie_len        = -1;
-static gint hf_dtls_handshake_cookie            = -1;
-static gint hf_dtls_handshake_cipher_suites_len = -1;
-static gint hf_dtls_handshake_cipher_suites     = -1;
-static gint hf_dtls_handshake_comp_methods_len  = -1;
-static gint hf_dtls_handshake_comp_methods      = -1;
 static gint hf_dtls_handshake_session_ticket_lifetime_hint = -1;
 static gint hf_dtls_handshake_session_ticket_len = -1;
 static gint hf_dtls_handshake_session_ticket    = -1;
@@ -135,14 +128,15 @@ static gint hf_dtls_fragment_count              = -1;
 static gint hf_dtls_reassembled_in              = -1;
 static gint hf_dtls_reassembled_length          = -1;
 
+/* header fields used in ssl-utils, but defined here. */
+static dtls_hfs_t dtls_hfs = { -1, -1 };
+
 /* Initialize the subtree pointers */
 static gint ett_dtls                   = -1;
 static gint ett_dtls_record            = -1;
 static gint ett_dtls_alert             = -1;
 static gint ett_dtls_handshake         = -1;
 static gint ett_dtls_heartbeat         = -1;
-static gint ett_dtls_cipher_suites     = -1;
-static gint ett_dtls_comp_methods      = -1;
 static gint ett_dtls_new_ses_ticket    = -1;
 static gint ett_dtls_certs             = -1;
 
@@ -314,14 +308,6 @@ static void dissect_dtls_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
                                    const SslSession *session, guint32 record_length,
                                    gboolean decrypted);
 
-
-static void dissect_dtls_hnd_cli_hello(tvbuff_t *tvb,
-                                       packet_info *pinfo,
-                                       proto_tree *tree,
-                                       guint32 offset, guint32 length,
-                                       SslSession *session,
-                                       SslDecryptSession* ssl);
-
 static int dissect_dtls_hnd_hello_verify_request(tvbuff_t *tvb,
                                                   proto_tree *tree,
                                                   guint32 offset,
@@ -1344,7 +1330,15 @@ dissect_dtls_handshake(tvbuff_t *tvb, packet_info *pinfo,
             break;
 
           case SSL_HND_CLIENT_HELLO:
-            dissect_dtls_hnd_cli_hello(sub_tvb, pinfo, ssl_hand_tree, 0, length, session, ssl);
+            if (ssl) {
+                /* ClientHello is first packet so set direction and try to
+                 * find a private key matching the server port */
+                ssl_set_server(ssl, &pinfo->dst, pinfo->ptype, pinfo->destport);
+                ssl_find_private_key(ssl, dtls_key_hash, dtls_associations, pinfo);
+            }
+            ssl_dissect_hnd_cli_hello(&dissect_dtls_hf, sub_tvb, pinfo,
+                                      ssl_hand_tree, 0, length, session, ssl,
+                                      &dtls_hfs);
             break;
 
           case SSL_HND_SERVER_HELLO:
@@ -1493,142 +1487,6 @@ dissect_dtls_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
   }
 }
 
-static void
-dissect_dtls_hnd_cli_hello(tvbuff_t *tvb, packet_info *pinfo,
-                           proto_tree *tree, guint32 offset, guint32 length,
-                           SslSession *session, SslDecryptSession *ssl)
-{
-  /* struct {
-   *     ProtocolVersion client_version;
-   *     Random random;
-   *     SessionID session_id;
-   *     opaque cookie<0..32>;                   //new field
-   *     CipherSuite cipher_suites<2..2^16-1>;
-   *     CompressionMethod compression_methods<1..2^8-1>;
-   *     Extension client_hello_extension_list<0..2^16-1>;
-   * } ClientHello;
-   *
-   */
-
-  proto_tree *ti;
-  proto_tree *cs_tree;
-  guint16     cipher_suite_length;
-  guint8      compression_methods_length;
-  guint8      compression_method;
-  guint16     start_offset   = offset;
-  guint8      cookie_length;
-
-    if (ssl) {
-        ssl_set_server(ssl, &pinfo->dst, pinfo->ptype, pinfo->destport);
-        ssl_find_private_key(ssl, dtls_key_hash, dtls_associations, pinfo);
-    }
-
-  if (tree || ssl)
-    {
-      /* show the client version */
-      proto_tree_add_item(tree, hf_dtls_handshake_client_version, tvb,
-                            offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-
-      /* show the fields in common with server hello */
-      offset = ssl_dissect_hnd_hello_common(&dissect_dtls_hf, tvb, tree, offset, ssl, FALSE);
-
-      if (!tree)
-        return;
-
-      /* look for a cookie */
-      cookie_length = tvb_get_guint8(tvb, offset);
-
-      proto_tree_add_uint(tree, hf_dtls_handshake_cookie_len,
-                          tvb, offset, 1, cookie_length);
-      offset ++;            /* skip opaque length */
-
-      if (cookie_length > 0)
-        {
-          proto_tree_add_item(tree, hf_dtls_handshake_cookie, tvb, offset,
-                              cookie_length, ENC_NA);
-          offset += cookie_length;
-        }
-
-      /* tell the user how many cipher suites there are */
-      cipher_suite_length = tvb_get_ntohs(tvb, offset);
-
-      proto_tree_add_uint(tree, hf_dtls_handshake_cipher_suites_len,
-                          tvb, offset, 2, cipher_suite_length);
-      offset += 2;            /* skip opaque length */
-
-      if (cipher_suite_length > 0)
-        {
-          ti = proto_tree_add_none_format(tree,
-                                          hf_dtls_handshake_cipher_suites,
-                                          tvb, offset, cipher_suite_length,
-                                          "Cipher Suites (%u suite%s)",
-                                          cipher_suite_length / 2,
-                                          plurality(cipher_suite_length/2, "", "s"));
-
-          /* make this a subtree */
-          cs_tree = proto_item_add_subtree(ti, ett_dtls_cipher_suites);
-          if (!cs_tree)
-            {
-              cs_tree = tree; /* failsafe */
-            }
-
-          while (cipher_suite_length > 0)
-            {
-              proto_tree_add_item(cs_tree, dissect_dtls_hf.hf.hs_cipher_suite,
-                                  tvb, offset, 2, ENC_BIG_ENDIAN);
-              offset += 2;
-              cipher_suite_length -= 2;
-            }
-        }
-
-      /* tell the user how man compression methods there are */
-      compression_methods_length = tvb_get_guint8(tvb, offset);
-      proto_tree_add_uint(tree, hf_dtls_handshake_comp_methods_len,
-                          tvb, offset, 1, compression_methods_length);
-      offset++;
-
-      if (compression_methods_length > 0)
-        {
-          ti = proto_tree_add_none_format(tree,
-                                          hf_dtls_handshake_comp_methods,
-                                          tvb, offset, compression_methods_length,
-                                          "Compression Methods (%u method%s)",
-                                          compression_methods_length,
-                                          plurality(compression_methods_length,
-                                                    "", "s"));
-
-          /* make this a subtree */
-          cs_tree = proto_item_add_subtree(ti, ett_dtls_comp_methods);
-
-          while (compression_methods_length > 0)
-            {
-              compression_method = tvb_get_guint8(tvb, offset);
-              if (compression_method < 64)
-                proto_tree_add_uint(cs_tree, dissect_dtls_hf.hf.hs_comp_method,
-                                    tvb, offset, 1, compression_method);
-              else if (compression_method > 63 && compression_method < 193)
-                proto_tree_add_text(cs_tree, tvb, offset, 1,
-                                    "Compression Method: Reserved - to be assigned by IANA (%u)",
-                                    compression_method);
-              else
-                proto_tree_add_text(cs_tree, tvb, offset, 1,
-                                    "Compression Method: Private use range (%u)",
-                                    compression_method);
-              offset++;
-              compression_methods_length--;
-            }
-        }
-
-      if (length > offset - start_offset)
-        {
-          ssl_dissect_hnd_hello_ext(&dissect_dtls_hf, tvb, tree, offset,
-                                    length - (offset - start_offset), TRUE,
-                                    session, ssl);
-        }
-    }
-}
-
 static int
 dissect_dtls_hnd_hello_verify_request(tvbuff_t *tvb, proto_tree *tree,
                                       guint32 offset, SslDecryptSession* ssl _U_)
@@ -1651,13 +1509,13 @@ dissect_dtls_hnd_hello_verify_request(tvbuff_t *tvb, proto_tree *tree,
   /* look for a cookie */
   cookie_length = tvb_get_guint8(tvb, offset);
 
-  proto_tree_add_uint(tree, hf_dtls_handshake_cookie_len,
+  proto_tree_add_uint(tree, dtls_hfs.hf_dtls_handshake_cookie_len,
                         tvb, offset, 1, cookie_length);
   offset ++;            /* skip opaque length */
 
   if (cookie_length > 0)
   {
-     proto_tree_add_bytes_format(tree, hf_dtls_handshake_cookie,
+     proto_tree_add_bytes_format(tree, dtls_hfs.hf_dtls_handshake_cookie,
                                     tvb, offset, cookie_length,
                                     NULL, "Cookie (%u byte%s)",
                                     cookie_length,
@@ -1940,41 +1798,16 @@ proto_register_dtls(void)
         FT_UINT24, BASE_DEC, NULL, 0x0,
         "Fragment length of handshake message", HFILL }
     },
-    { &hf_dtls_handshake_client_version,
-      { "Version", "dtls.handshake.client_version",
-        FT_UINT16, BASE_HEX, VALS(ssl_versions), 0x0,
-        "Maximum version supported by client", HFILL }
-    },
-    { &hf_dtls_handshake_cipher_suites_len,
-      { "Cipher Suites Length", "dtls.handshake.cipher_suites_length",
-        FT_UINT16, BASE_DEC, NULL, 0x0,
-        "Length of cipher suites field", HFILL }
-    },
-    { &hf_dtls_handshake_cipher_suites,
-      { "Cipher Suites", "dtls.handshake.ciphersuites",
-        FT_NONE, BASE_NONE, NULL, 0x0,
-        "List of cipher suites supported by client", HFILL }
-    },
-    { &hf_dtls_handshake_cookie_len,
+    { &dtls_hfs.hf_dtls_handshake_cookie_len,
       { "Cookie Length", "dtls.handshake.cookie_length",
         FT_UINT8, BASE_DEC, NULL, 0x0,
         "Length of the cookie field", HFILL }
     },
-    { &hf_dtls_handshake_cookie,
+    { &dtls_hfs.hf_dtls_handshake_cookie,
       { "Cookie", "dtls.handshake.cookie",
         FT_BYTES, BASE_NONE, NULL, 0x0,
         NULL, HFILL }
     },
-    { &hf_dtls_handshake_comp_methods_len,
-      { "Compression Methods Length", "dtls.handshake.comp_methods_length",
-        FT_UINT8, BASE_DEC, NULL, 0x0,
-        "Length of compression methods field", HFILL }
-    },
-    { &hf_dtls_handshake_comp_methods,
-      { "Compression Methods", "dtls.handshake.comp_methods",
-        FT_NONE, BASE_NONE, NULL, 0x0,
-        "List of compression methods supported by client", HFILL }
-    },
     { &hf_dtls_handshake_session_ticket_lifetime_hint,
       { "Session Ticket Lifetime Hint", "dtls.handshake.session_ticket_lifetime_hint",
         FT_UINT32, BASE_DEC, NULL, 0x0,
@@ -2081,8 +1914,6 @@ proto_register_dtls(void)
     &ett_dtls_alert,
     &ett_dtls_handshake,
     &ett_dtls_heartbeat,
-    &ett_dtls_cipher_suites,
-    &ett_dtls_comp_methods,
     &ett_dtls_new_ses_ticket,
     &ett_dtls_certs,
     &ett_dtls_fragment,
diff --git a/epan/dissectors/packet-ssl-utils.c b/epan/dissectors/packet-ssl-utils.c
index d2c860df81..ee8c81b6e1 100644
--- a/epan/dissectors/packet-ssl-utils.c
+++ b/epan/dissectors/packet-ssl-utils.c
@@ -5237,6 +5237,124 @@ ssl_dissect_hnd_hello_common(ssl_common_dissect_t *hf, tvbuff_t *tvb,
 }
 
 void
+ssl_dissect_hnd_cli_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
+                          packet_info *pinfo, proto_tree *tree, guint32 offset,
+                          guint32 length, SslSession *session,
+                          SslDecryptSession *ssl, dtls_hfs_t *dtls_hfs)
+{
+    /* struct {
+     *     ProtocolVersion client_version;
+     *     Random random;
+     *     SessionID session_id;
+     *     opaque cookie<0..32>;                   //new field for DTLS
+     *     CipherSuite cipher_suites<2..2^16-1>;
+     *     CompressionMethod compression_methods<1..2^8-1>;
+     *     Extension client_hello_extension_list<0..2^16-1>;
+     * } ClientHello;
+     *
+     */
+    proto_item *ti;
+    proto_tree *cs_tree;
+    guint16     cipher_suite_length;
+    guint8      compression_methods_length;
+    guint8      compression_method;
+    guint16     start_offset = offset;
+
+    /* show the client version */
+    proto_tree_add_item(tree, hf->hf.hs_client_version, tvb,
+                        offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
+
+    /* dissect fields that are also present in ClientHello */
+    offset = ssl_dissect_hnd_hello_common(hf, tvb, tree, offset, ssl, FALSE);
+
+    /* fields specific for DTLS (cookie_len, cookie) */
+    if (dtls_hfs != NULL) {
+        /* look for a cookie */
+        guint8 cookie_length = tvb_get_guint8(tvb, offset);
+
+        proto_tree_add_uint(tree, dtls_hfs->hf_dtls_handshake_cookie_len,
+                            tvb, offset, 1, cookie_length);
+        offset++;
+        if (cookie_length > 0) {
+            proto_tree_add_item(tree, dtls_hfs->hf_dtls_handshake_cookie,
+                                tvb, offset, cookie_length, ENC_NA);
+            offset += cookie_length;
+        }
+    }
+
+    /* tell the user how many cipher suites there are */
+    cipher_suite_length = tvb_get_ntohs(tvb, offset);
+    ti = proto_tree_add_item(tree, hf->hf.hs_cipher_suites_len,
+                             tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
+    if (cipher_suite_length > 0) {
+        if (cipher_suite_length % 2) {
+            expert_add_info_format(pinfo, ti, &hf->ei.hs_cipher_suites_len_bad,
+                "Cipher suite length (%d) must be a multiple of 2",
+                cipher_suite_length);
+            return;
+        }
+        ti = proto_tree_add_none_format(tree,
+                                        hf->hf.hs_cipher_suites,
+                                        tvb, offset, cipher_suite_length,
+                                        "Cipher Suites (%d suite%s)",
+                                        cipher_suite_length / 2,
+                                        plurality(cipher_suite_length/2, "", "s"));
+
+        /* make this a subtree */
+        cs_tree = proto_item_add_subtree(ti, hf->ett.cipher_suites);
+
+        while (cipher_suite_length > 0) {
+            proto_tree_add_item(cs_tree, hf->hf.hs_cipher_suite,
+                                tvb, offset, 2, ENC_BIG_ENDIAN);
+            offset += 2;
+            cipher_suite_length -= 2;
+        }
+    }
+    /* tell the user how many compression methods there are */
+    compression_methods_length = tvb_get_guint8(tvb, offset);
+    proto_tree_add_uint(tree, hf->hf.hs_comp_methods_len,
+                        tvb, offset, 1, compression_methods_length);
+    offset += 1;
+    if (compression_methods_length > 0) {
+        ti = proto_tree_add_none_format(tree,
+                                        hf->hf.hs_comp_methods,
+                                        tvb, offset, compression_methods_length,
+                                        "Compression Methods (%u method%s)",
+                                        compression_methods_length,
+                                        plurality(compression_methods_length,
+                                          "", "s"));
+
+        /* make this a subtree */
+        cs_tree = proto_item_add_subtree(ti, hf->ett.comp_methods);
+
+        while (compression_methods_length > 0) {
+            compression_method = tvb_get_guint8(tvb, offset);
+            /* TODO: make reserved/private comp meth. fields selectable */
+            if (compression_method < 64)
+                proto_tree_add_uint(cs_tree, hf->hf.hs_comp_method,
+                                    tvb, offset, 1, compression_method);
+            else if (compression_method > 63 && compression_method < 193)
+                proto_tree_add_text(cs_tree, tvb, offset, 1,
+                                    "Compression Method: Reserved - to be assigned by IANA (%u)",
+                                    compression_method);
+            else
+                proto_tree_add_text(cs_tree, tvb, offset, 1,
+                                    "Compression Method: Private use range (%u)",
+                                    compression_method);
+            offset++;
+            compression_methods_length--;
+        }
+    }
+    if (length > offset - start_offset) {
+        ssl_dissect_hnd_hello_ext(hf, tvb, tree, offset,
+                                  length - (offset - start_offset), TRUE,
+                                  session, ssl);
+    }
+}
+
+void
 ssl_dissect_hnd_srv_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
                           proto_tree *tree, guint32 offset, guint32 length,
                           SslSession *session, SslDecryptSession *ssl)
diff --git a/epan/dissectors/packet-ssl-utils.h b/epan/dissectors/packet-ssl-utils.h
index 8bf952f98f..1cf510d3ab 100644
--- a/epan/dissectors/packet-ssl-utils.h
+++ b/epan/dissectors/packet-ssl-utils.h
@@ -692,8 +692,13 @@ typedef struct ssl_common_dissect {
         gint hs_random_bytes;
         gint hs_session_id;
         gint hs_session_id_len;
+        gint hs_client_version;
         gint hs_server_version;
+        gint hs_cipher_suites_len;
+        gint hs_cipher_suites;
         gint hs_cipher_suite;
+        gint hs_comp_methods_len;
+        gint hs_comp_methods;
         gint hs_comp_method;
 
         /* do not forget to update SSL_COMMON_LIST_T and SSL_COMMON_HF_LIST! */
@@ -716,17 +721,28 @@ typedef struct ssl_common_dissect {
         gint cert_types;
         gint dnames;
         gint hs_random;
+        gint cipher_suites;
+        gint comp_methods;
 
         /* do not forget to update SSL_COMMON_LIST_T and SSL_COMMON_ETT_LIST! */
     } ett;
     struct {
         expert_field hs_ext_cert_status_undecoded;
         expert_field hs_sig_hash_alg_len_bad;
+        expert_field hs_cipher_suites_len_bad;
 
         /* do not forget to update SSL_COMMON_LIST_T and SSL_COMMON_EI_LIST! */
     } ei;
 } ssl_common_dissect_t;
 
+/* Header fields specific to DTLS. See packet-dtls.c */
+typedef struct {
+    gint hf_dtls_handshake_cookie_len;
+    gint hf_dtls_handshake_cookie;
+
+    /* Do not forget to initialize dtls_hfs to -1 in packet-dtls.c! */
+} dtls_hfs_t;
+
 extern gint
 ssl_dissect_hnd_hello_ext(ssl_common_dissect_t *hf, tvbuff_t *tvb, proto_tree *tree,
                           guint32 offset, guint32 left, gboolean is_client,
@@ -738,6 +754,13 @@ ssl_dissect_hnd_hello_common(ssl_common_dissect_t *hf, tvbuff_t *tvb,
                              SslDecryptSession *ssl, gboolean from_server);
 
 extern void
+ssl_dissect_hnd_cli_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
+                          packet_info *pinfo, proto_tree *tree, guint32 offset,
+                          guint32 length, SslSession *session,
+                          SslDecryptSession *ssl,
+                          dtls_hfs_t *dtls_hfs);
+
+extern void
 ssl_dissect_hnd_srv_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
                           proto_tree *tree, guint32 offset, guint32 length,
                           SslSession *session, SslDecryptSession *ssl);
@@ -778,14 +801,14 @@ ssl_common_dissect_t name = {   \
         -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, \
         -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, \
         -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, \
-        -1, -1, -1, -1, -1, -1, -1,                                     \
+        -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,                 \
     },                                                                  \
     /* ett */ {                                                         \
         -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, \
-        -1,                                                             \
+        -1, -1, -1,                                                     \
     },                                                                  \
     /* ei */ {                                                          \
-        EI_INIT, EI_INIT,                                               \
+        EI_INIT, EI_INIT, EI_INIT,                                      \
     },                                                                  \
 }
 /* }}} */
@@ -1212,16 +1235,41 @@ ssl_common_dissect_t name = {   \
         FT_UINT8, BASE_DEC, NULL, 0x0,                                  \
         "Length of Session ID field", HFILL }                           \
     },                                                                  \
+    { & name .hf.hs_client_version,                                     \
+      { "Version", prefix ".handshake.version",                         \
+        FT_UINT16, BASE_HEX, VALS(ssl_versions), 0x0,                   \
+        "Maximum version supported by client", HFILL }                  \
+    },                                                                  \
     { & name .hf.hs_server_version,                                     \
       { "Version", prefix ".handshake.version",                         \
         FT_UINT16, BASE_HEX, VALS(ssl_versions), 0x0,                   \
         "Version selected by server", HFILL }                           \
     },                                                                  \
+    { & name .hf.hs_cipher_suites_len,                                  \
+      { "Cipher Suites Length", prefix ".handshake.cipher_suites_length", \
+        FT_UINT16, BASE_DEC, NULL, 0x0,                                 \
+        "Length of cipher suites field", HFILL }                        \
+    },                                                                  \
+    { & name .hf.hs_cipher_suites,                                      \
+      { "Cipher Suites", prefix ".handshake.ciphersuites",              \
+        FT_NONE, BASE_NONE, NULL, 0x0,                                  \
+        "List of cipher suites supported by client", HFILL }            \
+    },                                                                  \
     { & name .hf.hs_cipher_suite,                                       \
       { "Cipher Suite", prefix ".handshake.ciphersuite",                \
         FT_UINT16, BASE_HEX|BASE_EXT_STRING, &ssl_31_ciphersuite_ext, 0x0, \
         NULL, HFILL }                                                   \
     },                                                                  \
+    { & name .hf.hs_comp_methods_len,                                   \
+      { "Compression Methods Length", prefix ".handshake.comp_methods_length", \
+        FT_UINT8, BASE_DEC, NULL, 0x0,                                  \
+        "Length of compression methods field", HFILL }                  \
+    },                                                                  \
+    { & name .hf.hs_comp_methods,                                       \
+      { "Compression Methods", prefix ".handshake.comp_methods",        \
+        FT_NONE, BASE_NONE, NULL, 0x0,                                  \
+        "List of compression methods supported by client", HFILL }      \
+    },                                                                  \
     { & name .hf.hs_comp_method,                                        \
       { "Compression Method", prefix ".handshake.comp_method",          \
         FT_UINT8, BASE_DEC, VALS(ssl_31_compression_method), 0x0,       \
@@ -1248,6 +1296,8 @@ ssl_common_dissect_t name = {   \
         & name .ett.cert_types,                     \
         & name .ett.dnames,                         \
         & name .ett.hs_random,                      \
+        & name .ett.cipher_suites,                  \
+        & name .ett.comp_methods,                   \
 /* }}} */
 
 /* {{{ */
@@ -1260,6 +1310,10 @@ ssl_common_dissect_t name = {   \
         { prefix ".handshake.sig_hash_alg_len.mult2", PI_MALFORMED, PI_ERROR, \
         "Signature Hash Algorithm length must be a multiple of 2", EXPFILL } \
     }, \
+    { & name .ei.hs_cipher_suites_len_bad, \
+        { prefix ".handshake.cipher_suites_length.mult2", PI_MALFORMED, PI_ERROR, \
+        "Cipher suite length must be a multiple of 2", EXPFILL } \
+    }
 /* }}} */
 
 typedef struct ssl_common_options {
diff --git a/epan/dissectors/packet-ssl.c b/epan/dissectors/packet-ssl.c
index 4fd64286d4..e09cf1428d 100644
--- a/epan/dissectors/packet-ssl.c
+++ b/epan/dissectors/packet-ssl.c
@@ -147,11 +147,6 @@ static gint hf_ssl_alert_message_description  = -1;
 static gint hf_ssl_handshake_protocol         = -1;
 static gint hf_ssl_handshake_type             = -1;
 static gint hf_ssl_handshake_length           = -1;
-static gint hf_ssl_handshake_client_version   = -1;
-static gint hf_ssl_handshake_cipher_suites_len = -1;
-static gint hf_ssl_handshake_cipher_suites    = -1;
-static gint hf_ssl_handshake_comp_methods_len = -1;
-static gint hf_ssl_handshake_comp_methods     = -1;
 static gint hf_ssl_handshake_session_ticket_lifetime_hint = -1;
 static gint hf_ssl_handshake_session_ticket_len = -1;
 static gint hf_ssl_handshake_session_ticket = -1;
@@ -218,8 +213,6 @@ static gint ett_ssl_record            = -1;
 static gint ett_ssl_alert             = -1;
 static gint ett_ssl_handshake         = -1;
 static gint ett_ssl_heartbeat         = -1;
-static gint ett_ssl_cipher_suites     = -1;
-static gint ett_ssl_comp_methods      = -1;
 static gint ett_ssl_certs             = -1;
 static gint ett_ssl_new_ses_ticket    = -1;
 static gint ett_ssl_cli_sig           = -1;
@@ -232,7 +225,6 @@ static gint ett_pct_exch_suites       = -1;
 static gint ett_ssl_segments          = -1;
 static gint ett_ssl_segment           = -1;
 
-static expert_field ei_ssl_handshake_cipher_suites_mult2 = EI_INIT;
 static expert_field ei_ssl2_handshake_session_id_len_error = EI_INIT;
 static expert_field ei_ssl3_heartbeat_payload_length = EI_INIT;
 
@@ -471,12 +463,6 @@ static void dissect_ssl3_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
                                    const SslSession *session, guint32 record_length,
                                    gboolean decrypted);
 
-static void dissect_ssl3_hnd_cli_hello(tvbuff_t *tvb, packet_info *pinfo,
-                                       proto_tree *tree,
-                                       guint32 offset, guint32 length,
-                                       SslSession *session,
-                                       SslDecryptSession *ssl);
-
 static void dissect_ssl3_hnd_new_ses_ticket(tvbuff_t *tvb,
                                        proto_tree *tree,
                                        guint32 offset, guint32 length,
@@ -1946,7 +1932,15 @@ dissect_ssl3_handshake(tvbuff_t *tvb, packet_info *pinfo,
                 break;
 
             case SSL_HND_CLIENT_HELLO:
-                dissect_ssl3_hnd_cli_hello(tvb, pinfo, ssl_hand_tree, offset, length, session, ssl);
+                if (ssl) {
+                    /* ClientHello is first packet so set direction and try to
+                     * find a private key matching the server port */
+                    ssl_set_server(ssl, &pinfo->dst, pinfo->ptype, pinfo->destport);
+                    ssl_find_private_key(ssl, ssl_key_hash, ssl_associations, pinfo);
+                }
+                ssl_dissect_hnd_cli_hello(&dissect_ssl3_hf, tvb, pinfo,
+                                          ssl_hand_tree, offset, length, session, ssl,
+                                          NULL);
                 break;
 
             case SSL_HND_SERVER_HELLO:
@@ -2115,135 +2109,6 @@ dissect_ssl3_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
 }
 
 static void
-dissect_ssl3_hnd_cli_hello(tvbuff_t *tvb, packet_info *pinfo,
-       proto_tree *tree, guint32 offset, guint32 length,
-       SslSession *session, SslDecryptSession*ssl)
-{
-    /* struct {
-     *     ProtocolVersion client_version;
-     *     Random random;
-     *     SessionID session_id;
-     *     CipherSuite cipher_suites<2..2^16-1>;
-     *     CompressionMethod compression_methods<1..2^8-1>;
-     *     Extension client_hello_extension_list<0..2^16-1>;
-     * } ClientHello;
-     *
-     */
-    proto_item *ti;
-    proto_tree *cs_tree;
-    gint        cipher_suite_length;
-    guint8      compression_methods_length;
-    guint8      compression_method;
-    guint16     start_offset;
-
-    start_offset = offset;
-
-    if (ssl) {
-        ssl_set_server(ssl, &pinfo->dst, pinfo->ptype, pinfo->destport);
-        ssl_find_private_key(ssl, ssl_key_hash, ssl_associations, pinfo);
-    }
-    if (tree || ssl)
-    {
-        /* show the client version */
-        if (tree)
-            proto_tree_add_item(tree, hf_ssl_handshake_client_version, tvb,
-                            offset, 2, ENC_BIG_ENDIAN);
-        offset += 2;
-        /* show the fields in common with server hello */
-        offset = ssl_dissect_hnd_hello_common(&dissect_ssl3_hf, tvb, tree, offset, ssl, FALSE);
-        /* tell the user how many cipher suites there are */
-        cipher_suite_length = tvb_get_ntohs(tvb, offset);
-
-        /* even if there's no tree, we'll have to dissect the whole record to get to the extensions.
-         * we will continue with tree==NULL */
-
-        proto_tree_add_uint(tree, hf_ssl_handshake_cipher_suites_len,
-                        tvb, offset, 2, cipher_suite_length);
-        offset += 2;            /* skip opaque length */
-        if (cipher_suite_length > 0)
-        {
-            tvb_ensure_bytes_exist(tvb, offset, cipher_suite_length);
-            ti = proto_tree_add_none_format(tree,
-                                            hf_ssl_handshake_cipher_suites,
-                                            tvb, offset, cipher_suite_length,
-                                            "Cipher Suites (%d suite%s)",
-                                            cipher_suite_length / 2,
-                                            plurality(cipher_suite_length/2, "", "s"));
-            if (cipher_suite_length % 2) {
-                proto_tree_add_text(tree, tvb, offset, 2,
-                    "Invalid cipher suite length: %d", cipher_suite_length);
-                expert_add_info_format(pinfo, NULL, &ei_ssl_handshake_cipher_suites_mult2,
-                    "Cipher suite length (%d) must be a multiple of 2",
-                    cipher_suite_length);
-                return;
-            }
-
-            /* make this a subtree */
-            cs_tree = proto_item_add_subtree(ti, ett_ssl_cipher_suites);
-            if (!cs_tree)
-            {
-                cs_tree = tree; /* failsafe */
-            }
-
-            while (cipher_suite_length > 0)
-            {
-                proto_tree_add_item(cs_tree, dissect_ssl3_hf.hf.hs_cipher_suite,
-                                    tvb, offset, 2, ENC_BIG_ENDIAN);
-                offset += 2;
-                cipher_suite_length -= 2;
-            }
-        }
-        /* tell the user how many compression methods there are */
-        compression_methods_length = tvb_get_guint8(tvb, offset);
-        proto_tree_add_uint(tree, hf_ssl_handshake_comp_methods_len,
-                            tvb, offset, 1, compression_methods_length);
-        offset += 1;
-        if (compression_methods_length > 0)
-        {
-            tvb_ensure_bytes_exist(tvb, offset, compression_methods_length);
-            ti = proto_tree_add_none_format(tree,
-                                            hf_ssl_handshake_comp_methods,
-                                            tvb, offset, compression_methods_length,
-                                            "Compression Methods (%u method%s)",
-                                            compression_methods_length,
-                                            plurality(compression_methods_length,
-                                              "", "s"));
-
-            /* make this a subtree */
-            cs_tree = proto_item_add_subtree(ti, ett_ssl_comp_methods);
-            if (!cs_tree)
-            {
-                cs_tree = tree; /* failsafe */
-            }
-
-            while (compression_methods_length > 0)
-            {
-                compression_method = tvb_get_guint8(tvb, offset);
-                if (compression_method < 64)
-                    proto_tree_add_uint(cs_tree, dissect_ssl3_hf.hf.hs_comp_method,
-                                        tvb, offset, 1, compression_method);
-                else if (compression_method > 63 && compression_method < 193)
-                    proto_tree_add_text(cs_tree, tvb, offset, 1,
-                                        "Compression Method: Reserved - to be assigned by IANA (%u)",
-                                        compression_method);
-                else
-                    proto_tree_add_text(cs_tree, tvb, offset, 1,
-                                        "Compression Method: Private use range (%u)",
-                                        compression_method);
-                offset++;
-                compression_methods_length--;
-            }
-        }
-        if (length > offset - start_offset)
-        {
-            ssl_dissect_hnd_hello_ext(&dissect_ssl3_hf, tvb, tree, offset,
-                                      length - (offset - start_offset), TRUE,
-                                      session, ssl);
-        }
-    }
-}
-
-static void
 dissect_ssl3_hnd_new_ses_ticket(tvbuff_t *tvb, proto_tree *tree,
                               guint32 offset, guint32 length, SslDecryptSession *ssl)
 {
@@ -2803,7 +2668,7 @@ dissect_ssl2_hnd_client_hello(tvbuff_t *tvb, packet_info *pinfo,
     {
         /* show the version */
         if (tree)
-            proto_tree_add_item(tree, hf_ssl_handshake_client_version, tvb,
+            proto_tree_add_item(tree, dissect_ssl3_hf.hf.hs_client_version, tvb,
                             offset, 2, ENC_BIG_ENDIAN);
         offset += 2;
 
@@ -2837,13 +2702,13 @@ dissect_ssl2_hnd_client_hello(tvbuff_t *tvb, packet_info *pinfo,
         {
             /* tell the user how many cipher specs they've won */
             tvb_ensure_bytes_exist(tvb, offset, cipher_spec_length);
-            ti = proto_tree_add_none_format(tree, hf_ssl_handshake_cipher_suites,
+            ti = proto_tree_add_none_format(tree, dissect_ssl3_hf.hf.hs_cipher_suites,
                                             tvb, offset, cipher_spec_length,
                                             "Cipher Specs (%u specs)",
                                             cipher_spec_length/3);
 
             /* make this a subtree and expand the actual specs below */
-            cs_tree = proto_item_add_subtree(ti, ett_ssl_cipher_suites);
+            cs_tree = proto_item_add_subtree(ti, dissect_ssl3_hf.ett.cipher_suites);
             if (!cs_tree)
             {
                 cs_tree = tree;     /* failsafe */
@@ -3415,12 +3280,12 @@ dissect_ssl2_hnd_server_hello(tvbuff_t *tvb,
         /* provide a collapsing node for the cipher specs */
         tvb_ensure_bytes_exist(tvb, offset, cipher_spec_length);
         ti = proto_tree_add_none_format(tree,
-                                        hf_ssl_handshake_cipher_suites,
+                                        dissect_ssl3_hf.hf.hs_cipher_suites,
                                         tvb, offset, cipher_spec_length,
                                         "Cipher Specs (%u spec%s)",
                                         cipher_spec_length/3,
                                         plurality(cipher_spec_length/3, "", "s"));
-        subtree = proto_item_add_subtree(ti, ett_ssl_cipher_suites);
+        subtree = proto_item_add_subtree(ti, dissect_ssl3_hf.ett.cipher_suites);
         if (!subtree)
         {
             subtree = tree;
@@ -3997,36 +3862,11 @@ proto_register_ssl(void)
             FT_UINT24, BASE_DEC, NULL, 0x0,
             "Length of handshake message", HFILL }
         },
-        { &hf_ssl_handshake_client_version,
-          { "Version", "ssl.handshake.version",
-            FT_UINT16, BASE_HEX, VALS(ssl_versions), 0x0,
-            "Maximum version supported by client", HFILL }
-        },
-        { &hf_ssl_handshake_cipher_suites_len,
-          { "Cipher Suites Length", "ssl.handshake.cipher_suites_length",
-            FT_UINT16, BASE_DEC, NULL, 0x0,
-            "Length of cipher suites field", HFILL }
-        },
-        { &hf_ssl_handshake_cipher_suites,
-          { "Cipher Suites", "ssl.handshake.ciphersuites",
-            FT_NONE, BASE_NONE, NULL, 0x0,
-            "List of cipher suites supported by client", HFILL }
-        },
         { &hf_ssl2_handshake_cipher_spec,
           { "Cipher Spec", "ssl.handshake.cipherspec",
             FT_UINT24, BASE_HEX|BASE_EXT_STRING, &ssl_20_cipher_suites_ext, 0x0,
             "Cipher specification", HFILL }
         },
-        { &hf_ssl_handshake_comp_methods_len,
-          { "Compression Methods Length", "ssl.handshake.comp_methods_length",
-            FT_UINT8, BASE_DEC, NULL, 0x0,
-            "Length of compression methods field", HFILL }
-        },
-        { &hf_ssl_handshake_comp_methods,
-          { "Compression Methods", "ssl.handshake.comp_methods",
-            FT_NONE, BASE_NONE, NULL, 0x0,
-            "List of compression methods supported by client", HFILL }
-        },
         { &hf_ssl_handshake_session_ticket_lifetime_hint,
           { "Session Ticket Lifetime Hint", "ssl.handshake.session_ticket_lifetime_hint",
             FT_UINT32, BASE_DEC, NULL, 0x0,
@@ -4319,8 +4159,6 @@ proto_register_ssl(void)
         &ett_ssl_alert,
         &ett_ssl_handshake,
         &ett_ssl_heartbeat,
-        &ett_ssl_cipher_suites,
-        &ett_ssl_comp_methods,
         &ett_ssl_certs,
         &ett_ssl_new_ses_ticket,
         &ett_ssl_cli_sig,
@@ -4336,7 +4174,6 @@ proto_register_ssl(void)
     };
 
     static ei_register_info ei[] = {
-        { &ei_ssl_handshake_cipher_suites_mult2, { "ssl.handshake.cipher_suites_length.mult2", PI_MALFORMED, PI_ERROR, "Cipher suite length must be a multiple of 2", EXPFILL }},
         { &ei_ssl2_handshake_session_id_len_error, { "ssl.handshake.session_id_length.error", PI_MALFORMED, PI_ERROR, "Session ID length error", EXPFILL }},
         { &ei_ssl3_heartbeat_payload_length, {"ssl.heartbeat_message.payload_length.invalid", PI_MALFORMED, PI_ERROR, "Invalid heartbeat payload length", EXPFILL }},
         SSL_COMMON_EI_LIST(dissect_ssl3_hf, "ssl")