Wireshark · Ethereal-dev: Re: [Ethereal-dev] MEGACO plugin seriously flawed?

[Ruud Linders <moztest@xxxxxxxxxxxxxxxxxxxx>]

Ariel et al,

I didn't know about SCTP NOT requiring a TPKT header but TCP indeed
DOES require a TPKT encapsulation.

Several months ago I made a quick-and-dirty patch to allow for TPKT
encapsulation and de-segmentation which is working fine for me.
However, it needs some cleaning up
- adding preference to be able to turn off/on de-segmentation,
  the global var is already there.
- correct check in the dissect_megaco_tpkt routine for the MEGACO
  string (for whatever reason I only checked for 'M' 'E' (probably
  lazines at the time!)
- proper skip/check for whitespace in same check
- registration/hook for sctp support

Attached the patch against 0.10.6, feel free to use it.

Regards,
	Ruud

Ariel Burbaickij wrote:
> Hello dear mailing list participants,
>
> following code exceprt from MEGACO plugin:
>   /*
>      * Check to see whether we're really dealing with MEGACO by looking
>      * for the MEGACO string.  This needs to be improved when supporting
>      * binary encodings.
>      */
>     if(!tvb_get_nstringz0(tvb,0,6,word)) return;
>     if (strncasecmp(word, "MEGACO", 6) != 0) return;
>
> This is incorrect as was found out today. IIUC what is going one this code
> omits the possbility of TPKT header between TCP and MEGACO which
> is reuqired as per RFC 3015 Addendum D. TPKT (RFC 1006) caters for message
> segmentation. So either documentation needs to be updated stating
> that MEGACO works only with SCTP where there is no such thing
> as message segmentation or code needs to be corrected. Would
> be glad to hear your opinion.
>
> With Best Regards
> Ariel Burbaickij
>
> _______________________________________________
> Ethereal-dev mailing list
> Ethereal-dev@xxxxxxxxxxxx
> http://www.ethereal.com/mailman/listinfo/ethereal-dev
--- packet-megaco.c.ORIG	Fri Aug 13 00:41:51 2004
+++ packet-megaco.c	Sat Sep 25 12:38:24 2004
@@ -11,6 +11,10 @@
 * Christoph Wiest,		2003/06/28
 * Modified 2003 by		Christoph Wiest
 *						<ch.wiest@xxxxxxxxxxxxx>
+*
+* Ruud Linders  2004-05-30	ruud.at.lucent.com
+* added TPKT support as per RFC3015
+*
 * Ethereal - Network traffic analyzer
 * By Gerald Combs <gerald@xxxxxxxxxxxx>
 * Copyright 1999 Gerald Combs
@@ -147,6 +151,9 @@
 #endif
 static gboolean global_megaco_raw_text = TRUE;
 static gboolean global_megaco_dissect_tree = TRUE;
+static gboolean megaco_desegment = TRUE;
+static dissector_handle_t megaco_text_handle_tpkt;
+static dissector_handle_t megaco_text_handle;
 
 /*
 * Variables to allow for proper deletion of dissector registration when
@@ -211,6 +218,85 @@
 static dissector_handle_t h245_handle;
 static proto_tree *top_tree;
 
+static int
+dissect_megaco_tpkt(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
+{
+        int lv_tpkt_len;
+        int skipws;
+        /*
+         * Check whether this looks like a TPKT-encapsulated
+         * h.248 packet.
+         *
+         * The minimum length of a MEGACO message is 6:
+         * 1 byte for the protocol discriminator,
+         * 1 for the call_reference length,
+         * and one for the message type.
+         */
+        lv_tpkt_len = is_tpkt(tvb, 6);
+        if (lv_tpkt_len == -1) {
+                /*
+                 * It's not a TPKT packet; reject it.
+                 */
+                return FALSE;
+        }
+
+        /*
+         * If this segment is *exactly* the length of a TPKT header,
+         * we assume that, as it looks like a TPKT header, it
+         * is one, and that the code put a TPKT header in one
+         * segment and the rest of the PDU in another.
+         */
+        if (tvb_length(tvb) == 4) {
+                /*
+                 * It is - call the "dissect TPKT over a TCP stream"
+                 * routine.
+                 */
+                dissect_tpkt_encap(tvb, pinfo, tree, megaco_desegment,
+                                   megaco_text_handle);
+                return TRUE;
+        }
+
+        /*
+         * Well, we have more data than just the TPKT header;
+         * check whether it looks like the beginning of a
+         * h.248 message.
+         *
+         * The minimum length of a h.248 message is 3, as per the
+         * above.
+         *
+         * Check that we have that many bytes past the TPKT header in
+         * the tvbuff; we already know that the TPKT header says we
+         * have that many bytes (as we passed 6 as the "min_len" argument
+         * to "is_tpkt()").
+         */
+        if (!tvb_bytes_exist(tvb, 4, 6))
+                return FALSE;
+
+        /* check for MEGACO??  Check the protocol discriminator */
+        if (tvb_get_guint8(tvb, 4) == 0x0a) {
+                skipws= 1;
+        } else {
+                skipws= 0;
+        }
+        if (tvb_get_guint8(tvb, 4+skipws) != 'M') {
+                /* Doesn't look like h.248 inside TPKT */
+                return FALSE;
+        }
+        if (tvb_get_guint8(tvb, 5+skipws) != 'E') {
+                /* Doesn't look like h.248 inside TPKT */
+                return FALSE;
+        }
+
+        /*
+         * OK, it looks like h.248-over-TPKT.
+         * Call the "dissect TPKT over a TCP stream" routine.
+         */
+        dissect_tpkt_encap(tvb, pinfo, tree, megaco_desegment,
+            megaco_text_handle);
+
+        return TRUE;
+}
+
 /*
 * dissect_megaco_text - The dissector for the MEGACO Protocol, using
 * text encoding.
@@ -2608,19 +2694,20 @@
 proto_reg_handoff_megaco(void)
 {
 	static int megaco_prefs_initialized = FALSE;
-	static dissector_handle_t megaco_text_handle;
 	
 	sdp_handle = find_dissector("sdp");
 	h245_handle = find_dissector("h245dg");
 	
 	if (!megaco_prefs_initialized) {
-		megaco_text_handle = create_dissector_handle(dissect_megaco_text,
-			proto_megaco);
+	  megaco_text_handle_tpkt = create_dissector_handle(dissect_megaco_tpkt,
+                                                     proto_megaco);
+	  megaco_text_handle = create_dissector_handle(dissect_megaco_text,
+                                                     proto_megaco);
 		megaco_prefs_initialized = TRUE;
 	}
 	else {
-		dissector_delete("tcp.port", txt_tcp_port, megaco_text_handle);
-		dissector_delete("udp.port", txt_udp_port, megaco_text_handle);
+		dissector_delete("tcp.port", txt_tcp_port, megaco_text_handle_tpkt);
+		dissector_delete("udp.port", txt_udp_port, megaco_text_handle_tpkt);
 #if 0
 		dissector_delete("tcp.port", bin_tcp_port, megaco_bin_handle);
 		dissector_delete("udp.port", bin_udp_port, megaco_bin_handle);
@@ -2637,8 +2724,8 @@
 	bin_udp_port = global_megaco_bin_udp_port;
 #endif
 	
-	dissector_add("tcp.port", global_megaco_txt_tcp_port, megaco_text_handle);
-	dissector_add("udp.port", global_megaco_txt_udp_port, megaco_text_handle);
+	dissector_add("tcp.port", global_megaco_txt_tcp_port, megaco_text_handle_tpkt);
+	dissector_add("udp.port", global_megaco_txt_udp_port, megaco_text_handle_tpkt);
 #if 0
 	dissector_add("tcp.port", global_megaco_bin_tcp_port, megaco_bin_handle);
 	dissector_add("udp.port", global_megaco_bin_udp_port, megaco_bin_handle);