[Ethereal-dev] [Patch] RTCP length checking

[Martin Mathieson <martin.mathieson@xxxxxxxxxxxx>]

Hi,

This patch includes the changes I previously sent in 
http://www.ethereal.com/lists/ethereal-dev/200605/msg00208.html (I 
assume it wasn't committed).  The combined patch:

- shows profile-specific extension data at the end of SR/RR reports (if 
packet length has not yet been reached after parsing normal data) and 
advances offset (further packets were not recognised+dissected as this 
data wasn't being skipped).
- checks that the length of the RTCP data in the whole frame matches the 
combined length from the length fields (the last check in RFC 3550, "A.2 
RTCP Header Validity Checks") with a generated field and expert info 
when wrong.
- reports the length field in all of the message types consistently (the 
length was confusingly shown multiplied by 4 only in APP packets...)

Best regards,
Martin

Index: epan/dissectors/packet-rtcp.c
===================================================================
--- epan/dissectors/packet-rtcp.c	(revision 18189)
+++ epan/dissectors/packet-rtcp.c	(working copy)
@@ -64,6 +64,7 @@
 
 #include <epan/prefs.h>
 #include <epan/emem.h>
+#include <epan/expert.h>
 
 
 /* Version is the first 2 bits of the first octet*/
@@ -300,6 +301,7 @@
 static int hf_rtcp_blp               = -1;
 static int hf_rtcp_padding_count     = -1;
 static int hf_rtcp_padding_data      = -1;
+static int hf_rtcp_profile_specific_extension = -1;
 static int hf_rtcp_app_poc1_subtype  = -1;
 static int hf_rtcp_app_poc1_sip_uri  = -1;
 static int hf_rtcp_app_poc1_disp_name = -1;
@@ -362,6 +364,7 @@
 static int hf_rtcp_xr_stats_devttl = -1;
 static int hf_rtcp_xr_lrr = -1;
 static int hf_rtcp_xr_dlrr = -1;
+static int hf_rtcp_length_check = -1;
 
 /* RTCP setup fields */
 static int hf_rtcp_setup        = -1;
@@ -1375,7 +1378,7 @@
 
 static int
 dissect_rtcp_rr( packet_info *pinfo, tvbuff_t *tvb, int offset, proto_tree *tree,
-    unsigned int count )
+    unsigned int count, unsigned int packet_length )
 {
 	unsigned int counter = 1;
 	proto_tree *ssrc_tree = (proto_tree*) NULL;
@@ -1384,6 +1387,7 @@
 	proto_item *ti = (proto_item*) NULL;
 	guint8 rr_flt;
 	unsigned int cum_nr = 0;
+	int rr_offset = offset; 
 
 	while ( counter <= count ) {
 		guint32 lsr, dlsr;
@@ -1455,16 +1459,25 @@
 		counter++;
 	}
 
+	/* If length remaining, assume profile-specific extension bytes */
+	if ((offset-rr_offset) < (int)packet_length)
+	{
+		proto_tree_add_item(tree, hf_rtcp_profile_specific_extension, tvb, offset,
+		                    packet_length - (offset - rr_offset), FALSE);
+		offset = rr_offset + packet_length;
+	}
+
 	return offset;
 }
 
 static int
 dissect_rtcp_sr( packet_info *pinfo, tvbuff_t *tvb, int offset, proto_tree *tree,
-    unsigned int count )
+    unsigned int count, unsigned int packet_length )
 {
 	proto_item* item;
 	guint32 ts_msw, ts_lsw;
 	gchar *buff;
+	int sr_offset = offset;
 
 	/* NTP timestamp */
 	ts_msw = tvb_get_ntohl(tvb, offset);
@@ -1500,7 +1513,17 @@
 
 	/* The rest of the packet is equal to the RR packet */
 	if ( count != 0 )
-		offset = dissect_rtcp_rr( pinfo, tvb, offset, tree, count );
+		offset = dissect_rtcp_rr( pinfo, tvb, offset, tree, count, packet_length-(offset-sr_offset) );
+	else
+    {
+		/* If length remaining, assume profile-specific extension bytes */
+		if ((offset-sr_offset) < (int)packet_length)
+		{
+			proto_tree_add_item(tree, hf_rtcp_profile_specific_extension, tvb, offset,
+			                    packet_length - (offset - sr_offset), FALSE);
+			offset = sr_offset + packet_length;
+		}
+	}
 
 	return offset;
 }
@@ -1822,6 +1845,7 @@
 	unsigned int packet_type = 0;
 	unsigned int offset      = 0;
 	guint16 packet_length    = 0;
+	guint16 total_packet_length = 0;
 	guint rtcp_subtype		 = 0;
 	guint32 app_length		 = 0;
 
@@ -1884,7 +1908,8 @@
         /*
          * get the packet-length for the complete RTCP packet
          */
-        packet_length = ( tvb_get_ntohs( tvb, offset + 2 ) + 1 ) * 4;
+		packet_length = ( tvb_get_ntohs( tvb, offset + 2 ) + 1 ) * 4;
+		total_packet_length += packet_length;
 
 		ti = proto_tree_add_item(tree, proto_rtcp, tvb, offset, packet_length, FALSE );
 		proto_item_append_text(ti, " (%s)",
@@ -1927,8 +1952,10 @@
                 proto_tree_add_uint( rtcp_tree, hf_rtcp_ssrc_sender, tvb, offset, 4, tvb_get_ntohl( tvb, offset ) );
                 offset += 4;
 
-                if ( packet_type == RTCP_SR ) offset = dissect_rtcp_sr( pinfo, tvb, offset, rtcp_tree, elem_count );
-                else offset = dissect_rtcp_rr( pinfo, tvb, offset, rtcp_tree, elem_count );
+                if ( packet_type == RTCP_SR )
+                    offset = dissect_rtcp_sr( pinfo, tvb, offset, rtcp_tree, elem_count, packet_length-8 );
+                else
+                    offset = dissect_rtcp_rr( pinfo, tvb, offset, rtcp_tree, elem_count, packet_length-8 );
                 break;
             case RTCP_SDES:
                 /* Source count, 5 bits */
@@ -1964,8 +1991,8 @@
                 proto_tree_add_item( rtcp_tree, hf_rtcp_pt, tvb, offset, 1, FALSE );
                 offset++;
                 /* Packet length in 32 bit words MINUS one, 16 bits */
-				app_length = tvb_get_ntohs( tvb, offset ) <<2;
-                proto_tree_add_uint( rtcp_tree, hf_rtcp_length, tvb, offset, 2, app_length );
+                app_length = tvb_get_ntohs( tvb, offset ) <<2;
+                proto_tree_add_uint( rtcp_tree, hf_rtcp_length, tvb, offset, 2, tvb_get_ntohs( tvb, offset ) );
                 offset += 2;
                 offset = dissect_rtcp_app( tvb, pinfo, offset,rtcp_tree, padding_set, packet_length - 4, rtcp_subtype, app_length);
                 break;
@@ -2007,6 +2034,29 @@
         offset += tvb_length_remaining( tvb, offset) - 1;
         proto_tree_add_item( rtcp_tree, hf_rtcp_padding_count, tvb, offset, 1, FALSE );
     }
+
+    /* offset should be total_packet_length by now... */
+    if (offset == (int)total_packet_length)
+    {
+        ti = proto_tree_add_boolean_format_value(tree, hf_rtcp_length_check, tvb,
+                                            0, 0, TRUE, "OK - %u bytes",
+                                            offset);
+        /* Hidden might be less annoying here...? */
+        PROTO_ITEM_SET_GENERATED(ti);
+    }
+    else
+    {
+        ti = proto_tree_add_boolean_format_value(tree, hf_rtcp_length_check, tvb,
+                                            0, 0, FALSE,
+                                            "Wrong (expected %u bytes, found %d)",
+                                            total_packet_length, offset);
+        PROTO_ITEM_SET_GENERATED(ti);
+
+        expert_add_info_format(pinfo, ti,
+                               PI_MALFORMED, PI_WARN,
+                               "Incorrect RTCP packet length information (expected %u bytes, found %d)",
+                               total_packet_length, offset);
+    }
 }
 
 void
@@ -2083,7 +2133,7 @@
 				BASE_DEC,
 				NULL,
 				0x0,
-				"", HFILL
+				"32-bit words (-1) in packet", HFILL
 			}
 		},
 		{
@@ -2771,6 +2821,18 @@
 			}
 		},
 		{
+			&hf_rtcp_profile_specific_extension,
+			{
+				"Profile-specific extension",
+				"rtcp.profile-specific-extension",
+				FT_BYTES,
+				BASE_NONE,
+				NULL,
+				0x0,
+				"Profile-specific extension", HFILL
+			}
+		},
+		{
 			&hf_rtcp_setup,
 			{
 				"Stream setup",
@@ -3346,6 +3408,18 @@
 				"", HFILL
 			}
 		},
+		{
+			&hf_rtcp_length_check,
+			{
+				"RTCP frame length check",
+				"rtcp.length_check",
+				FT_BOOLEAN,
+				BASE_NONE,
+				NULL,
+				0x0,
+				"RTCP frame length check", HFILL
+			}
+		},
 };
 
 	static gint *ett[] =