ssl,dtls: use ProtocolVersion from Server Hello

A DTLS capture from Jitsi Videobridge for Windows x64 (v519) using a
(patched?) BouncyCastle 1.51.0 exposed the odd behavior where the
ProtocolVersion from the record layer was always fixed to DTLSv1.2 while
the server agrees to use DTLSv1.0.

This resulted in a Malformed packet dissection of the ServerKeyExchange
message which mistakenly expects a SignatureAndHash field. Fix this
by using the protocol version from the ServerHello. Keep the fallback
in case a capture starts in the middle of a SSL conversation.

(Also display "DTLS" instead of "SSL" when the version is not yet
determined for DTLS packets.)

Bug: 11709
Change-Id: I0719977e3b2208da1960121b01dc109fa76bfcb6
Reviewed-on: https://code.wireshark.org/review/11821
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Peter Wu <peter@lekensteyn.nl>

4 files changed, 78 insertions, 80 deletions

diff --git a/epan/dissectors/packet-dtls.c b/epan/dissectors/packet-dtls.c
index 3d15e6d9fa..e352dd29f1 100644
--- a/epan/dissectors/packet-dtls.c
+++ b/epan/dissectors/packet-dtls.c
@@ -327,8 +327,6 @@ static int dissect_dtls_hnd_hello_verify_request(tvbuff_t *tvb,
  *
  */
 
-static gint  dtls_is_authoritative_version_message(guint8 content_type,
-                                                   guint8 next_byte);
 static gint  looks_like_dtls(tvbuff_t *tvb, guint32 offset);
 
 /*********************************************************************
@@ -388,8 +386,7 @@ dissect_dtls(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
     ssl_session = NULL;
 
   /* Initialize the protocol column; we'll set it later when we
-   * figure out what flavor of DTLS it is (actually only one
-   version exists). */
+   * figure out what flavor of DTLS it is */
   col_set_str(pinfo->cinfo, COL_PROTOCOL, "DTLS");
 
   /* clear the the info column */
@@ -751,21 +748,10 @@ dissect_dtls_record(tvbuff_t *tvb, packet_info *pinfo,
    * structure and print the column version
    */
   next_byte = tvb_get_guint8(tvb, offset);
-  if (session->version == SSL_VER_UNKNOWN
-      && dtls_is_authoritative_version_message(content_type, next_byte))
-  {
-    if (version == DTLSV1DOT0_VERSION ||
-        version == DTLSV1DOT0_OPENSSL_VERSION ||
-        version == DTLSV1DOT2_VERSION)
-    {
-      session->version = version;
-      if (ssl) {
-        ssl->state |= SSL_VERSION;
-      }
-    }
-  }
+  if (session->version == SSL_VER_UNKNOWN)
+    ssl_try_set_version(session, ssl, content_type, next_byte, TRUE, version);
   col_set_str(pinfo->cinfo, COL_PROTOCOL,
-      val_to_str_const(version, ssl_version_short_names, "DTLS"));
+      val_to_str_const(session->version, ssl_version_short_names, "DTLS"));
 
   /*
    * now dissect the next layer
@@ -860,7 +846,7 @@ dissect_dtls_record(tvbuff_t *tvb, packet_info *pinfo,
 
     proto_item_set_text(dtls_record_tree,
                         "%s Record Layer: %s Protocol: %s",
-                        val_to_str_const(session->version, ssl_version_short_names, "SSL"),
+                        val_to_str_const(session->version, ssl_version_short_names, "DTLS"),
                         val_to_str_const(content_type, ssl_31_content_type, "unknown"),
                         session->app_handle
                         ? dissector_handle_get_dissector_name(session->app_handle)
@@ -999,7 +985,7 @@ dissect_dtls_alert(tvbuff_t *tvb, packet_info *pinfo,
         {
           proto_item_set_text(tree, "%s Record Layer: Alert "
                               "(Level: %s, Description: %s)",
-                              val_to_str_const(session->version, ssl_version_short_names, "SSL"),
+                              val_to_str_const(session->version, ssl_version_short_names, "DTLS"),
                               level, desc);
           proto_tree_add_item(ssl_alert_tree, hf_dtls_alert_message_level,
                               tvb, offset++, 1, ENC_BIG_ENDIAN);
@@ -1011,7 +997,7 @@ dissect_dtls_alert(tvbuff_t *tvb, packet_info *pinfo,
         {
           proto_item_set_text(tree,
                               "%s Record Layer: Encrypted Alert",
-                              val_to_str_const(session->version, ssl_version_short_names, "SSL"));
+                              val_to_str_const(session->version, ssl_version_short_names, "DTLS"));
           proto_item_set_text(ssl_alert_tree,
                               "Alert Message: Encrypted Alert");
         }
@@ -1233,7 +1219,7 @@ dissect_dtls_handshake(tvbuff_t *tvb, packet_info *pinfo,
           if (first_iteration)
             {
               proto_item_set_text(tree, "%s Record Layer: %s Protocol: %s%s",
-                                  val_to_str_const(session->version, ssl_version_short_names, "SSL"),
+                                  val_to_str_const(session->version, ssl_version_short_names, "DTLS"),
                                   val_to_str_const(content_type, ssl_31_content_type, "unknown"),
                                   (msg_type_str!=NULL) ? msg_type_str :
                                   "Encrypted Handshake Message",
@@ -1242,7 +1228,7 @@ dissect_dtls_handshake(tvbuff_t *tvb, packet_info *pinfo,
           else
             {
               proto_item_set_text(tree, "%s Record Layer: %s Protocol: %s%s",
-                                  val_to_str_const(session->version, ssl_version_short_names, "SSL"),
+                                  val_to_str_const(session->version, ssl_version_short_names, "DTLS"),
                                   val_to_str_const(content_type, ssl_31_content_type, "unknown"),
                                   "Multiple Handshake Messages",
                                   (frag_str!=NULL) ? frag_str : "");
@@ -1291,7 +1277,7 @@ dissect_dtls_handshake(tvbuff_t *tvb, packet_info *pinfo,
 
           case SSL_HND_SERVER_HELLO:
             ssl_dissect_hnd_srv_hello(&dissect_dtls_hf, sub_tvb, pinfo, ssl_hand_tree,
-                                      0, length, session, ssl);
+                                      0, length, session, ssl, TRUE);
             break;
 
           case SSL_HND_HELLO_VERIFY_REQUEST:
@@ -1406,7 +1392,7 @@ dissect_dtls_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
     if (type && ((payload_length <= record_length - 16 - 3) || decrypted)) {
       proto_item_set_text(tree, "%s Record Layer: Heartbeat "
                                 "%s",
-                                val_to_str_const(session->version, ssl_version_short_names, "SSL"),
+                                val_to_str_const(session->version, ssl_version_short_names, "DTLS"),
                                 type);
       proto_tree_add_item(dtls_heartbeat_tree, hf_dtls_heartbeat_message_type,
                           tvb, offset, 1, ENC_BIG_ENDIAN);
@@ -1437,7 +1423,7 @@ dissect_dtls_heartbeat(tvbuff_t *tvb, packet_info *pinfo,
     } else {
       proto_item_set_text(tree,
                          "%s Record Layer: Encrypted Heartbeat",
-                         val_to_str_const(session->version, ssl_version_short_names, "SSL"));
+                         val_to_str_const(session->version, ssl_version_short_names, "DTLS"));
       proto_item_set_text(dtls_heartbeat_tree,
                           "Encrypted Heartbeat Message");
     }
@@ -1489,22 +1475,6 @@ dissect_dtls_hnd_hello_verify_request(tvbuff_t *tvb, proto_tree *tree,
  *
  *********************************************************************/
 
-static gint
-dtls_is_authoritative_version_message(guint8 content_type, guint8 next_byte)
-{
-  if (content_type == SSL_ID_HANDSHAKE
-      && ssl_is_valid_handshake_type(next_byte, TRUE))
-    {
-      return (next_byte != SSL_HND_CLIENT_HELLO);
-    }
-  else if (ssl_is_valid_content_type(content_type)
-           && content_type != SSL_ID_HANDSHAKE)
-    {
-      return 1;
-    }
-  return 0;
-}
-
 /* this applies a heuristic to determine whether
  * or not the data beginning at offset looks like a
  * valid dtls record.
diff --git a/epan/dissectors/packet-ssl-utils.c b/epan/dissectors/packet-ssl-utils.c
index 6ebf46108a..f510a5cdc4 100644
--- a/epan/dissectors/packet-ssl-utils.c
+++ b/epan/dissectors/packet-ssl-utils.c
@@ -5533,7 +5533,7 @@ ssl_dissect_hnd_hello_ext_ec_point_formats(ssl_common_dissect_t *hf, tvbuff_t *t
 }
 /** TLS Extensions (in Client Hello and Server Hello). }}} */
 
-/* Whether the Content Type and Handshake Type bytes are valid. {{{ */
+/* Whether the Content and Handshake Types are valid; handle Protocol Version. {{{ */
 gboolean
 ssl_is_valid_content_type(guint8 type)
 {
@@ -5575,6 +5575,55 @@ ssl_is_valid_handshake_type(guint8 hs_type, gboolean is_dtls)
     }
     return FALSE;
 }
+
+static gboolean
+ssl_is_authoritative_version_message(guint8 content_type, guint8 handshake_type,
+                                     gboolean is_dtls)
+{
+    /* Consider all valid Handshake messages (except for Client Hello) and
+     * all other valid record types (other than Handshake) */
+    return (content_type == SSL_ID_HANDSHAKE &&
+            ssl_is_valid_handshake_type(handshake_type, is_dtls) &&
+            handshake_type != SSL_HND_CLIENT_HELLO) ||
+           (content_type != SSL_ID_HANDSHAKE &&
+            ssl_is_valid_content_type(content_type));
+}
+
+void
+ssl_try_set_version(SslSession *session, SslDecryptSession *ssl,
+                    guint8 content_type, guint8 handshake_type,
+                    gboolean is_dtls, guint16 version)
+{
+    if (!ssl_is_authoritative_version_message(content_type, handshake_type,
+                is_dtls))
+        return;
+
+    switch (version) {
+    case SSLV3_VERSION:
+    case TLSV1_VERSION:
+    case TLSV1DOT1_VERSION:
+    case TLSV1DOT2_VERSION:
+        if (is_dtls)
+            return;
+        break;
+
+    case DTLSV1DOT0_VERSION:
+    case DTLSV1DOT0_OPENSSL_VERSION:
+    case DTLSV1DOT2_VERSION:
+        if (!is_dtls)
+            return;
+        break;
+
+    default: /* invalid version number */
+        return;
+    }
+
+    session->version = version;
+    if (ssl) {
+        ssl->state |= SSL_VERSION;
+        ssl_debug_printf("%s found version 0x%04X -> state 0x%02X\n", G_STRFUNC, version, ssl->state);
+    }
+}
 /* }}} */
 
 
@@ -5704,7 +5753,8 @@ ssl_dissect_hnd_cli_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
 void
 ssl_dissect_hnd_srv_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
                           packet_info* pinfo, proto_tree *tree, guint32 offset, guint32 length,
-                          SslSession *session, SslDecryptSession *ssl)
+                          SslSession *session, SslDecryptSession *ssl,
+                          gboolean is_dtls)
 {
     /* struct {
      *     ProtocolVersion server_version;
@@ -5717,6 +5767,10 @@ ssl_dissect_hnd_srv_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
      */
     guint16 start_offset = offset;
 
+    /* This version is always better than the guess at the Record Layer */
+    ssl_try_set_version(session, ssl, SSL_ID_HANDSHAKE, SSL_HND_SERVER_HELLO,
+            is_dtls, tvb_get_ntohs(tvb, offset));
+
     /* show the server version */
     proto_tree_add_item(tree, hf->hf.hs_server_version, tvb,
                         offset, 2, ENC_BIG_ENDIAN);
diff --git a/epan/dissectors/packet-ssl-utils.h b/epan/dissectors/packet-ssl-utils.h
index 5ccaf8abd0..3bbf44f858 100644
--- a/epan/dissectors/packet-ssl-utils.h
+++ b/epan/dissectors/packet-ssl-utils.h
@@ -585,6 +585,11 @@ extern gboolean
 ssl_is_valid_handshake_type(guint8 hs_type, gboolean is_dtls);
 
 extern void
+ssl_try_set_version(SslSession *session, SslDecryptSession *ssl,
+                    guint8 content_type, guint8 handshake_type,
+                    gboolean is_dtls, guint16 version);
+
+extern void
 ssl_calculate_handshake_hash(SslDecryptSession *ssl_session, tvbuff_t *tvb, guint32 offset, guint32 length);
 
 /* common header fields, subtrees and expert info for SSL and DTLS dissectors */
@@ -761,7 +766,8 @@ ssl_dissect_hnd_cli_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb,
 extern void
 ssl_dissect_hnd_srv_hello(ssl_common_dissect_t *hf, tvbuff_t *tvb, packet_info* pinfo,
                           proto_tree *tree, guint32 offset, guint32 length,
-                          SslSession *session, SslDecryptSession *ssl);
+                          SslSession *session, SslDecryptSession *ssl,
+                          gboolean is_dtls);
 
 extern void
 ssl_dissect_hnd_new_ses_ticket(ssl_common_dissect_t *hf, tvbuff_t *tvb,
diff --git a/epan/dissectors/packet-ssl.c b/epan/dissectors/packet-ssl.c
index 0ce1ec8295..b873d99c96 100644
--- a/epan/dissectors/packet-ssl.c
+++ b/epan/dissectors/packet-ssl.c
@@ -548,8 +548,6 @@ static void dissect_pct_msg_error(tvbuff_t *tvb,
  *
  */
 static gint  ssl_is_valid_ssl_version(const guint16 version);
-static gint  ssl_is_authoritative_version_message(const guint8 content_type,
-                                                  const guint8 next_byte);
 static gint  ssl_is_v2_client_hello(tvbuff_t *tvb, const guint32 offset);
 static gint  ssl_looks_like_sslv2(tvbuff_t *tvb, const guint32 offset);
 static gint  ssl_looks_like_sslv3(tvbuff_t *tvb, const guint32 offset);
@@ -1541,21 +1539,8 @@ dissect_ssl3_record(tvbuff_t *tvb, packet_info *pinfo,
      * structure and print the column version
      */
     next_byte = tvb_get_guint8(tvb, offset);
-    if (session->version == SSL_VER_UNKNOWN
-        && ssl_is_authoritative_version_message(content_type, next_byte))
-    {
-        switch (version) {
-        case SSLV3_VERSION:
-        case TLSV1_VERSION:
-        case TLSV1DOT1_VERSION:
-        case TLSV1DOT2_VERSION:
-            session->version = version;
-            if (ssl) {
-                ssl->state |= SSL_VERSION;
-                ssl_debug_printf("dissect_ssl3_record found version 0x%04X -> state 0x%02X\n", version, ssl->state);
-            }
-        }
-    }
+    if (session->version == SSL_VER_UNKNOWN)
+        ssl_try_set_version(session, ssl, content_type, next_byte, FALSE, version);
 
     /* on second and subsequent records per frame
      * add a delimiter on info column
@@ -1926,7 +1911,7 @@ dissect_ssl3_handshake(tvbuff_t *tvb, packet_info *pinfo,
 
             case SSL_HND_SERVER_HELLO:
                 ssl_dissect_hnd_srv_hello(&dissect_ssl3_hf, tvb, pinfo, ssl_hand_tree,
-                        offset, length, session, ssl);
+                        offset, length, session, ssl, FALSE);
                 break;
 
             case SSL_HND_HELLO_VERIFY_REQUEST:
@@ -3276,23 +3261,6 @@ ssl_is_valid_ssl_version(const guint16 version)
 }
 
 static gint
-ssl_is_authoritative_version_message(const guint8 content_type,
-                                     const guint8 next_byte)
-{
-    if (content_type == SSL_ID_HANDSHAKE
-        && ssl_is_valid_handshake_type(next_byte, FALSE))
-    {
-        return (next_byte != SSL_HND_CLIENT_HELLO);
-    }
-    else if (ssl_is_valid_content_type(content_type)
-             && content_type != SSL_ID_HANDSHAKE)
-    {
-        return 1;
-    }
-    return 0;
-}
-
-static gint
 ssl_is_v2_client_hello(tvbuff_t *tvb, const guint32 offset)
 {
     guint8 byte;