After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 751883 - rtcpbuffer: Fix validation of packets with padding
rtcpbuffer: Fix validation of packets with padding
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.5.2
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-07-03 09:19 UTC by Stian Selnes (stianse)
Modified: 2015-08-16 13:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch with fix and tests (7.77 KB, patch)
2015-07-03 09:19 UTC, Stian Selnes (stianse)
none Details | Review
Patch with fix and tests (8.66 KB, patch)
2015-07-03 22:45 UTC, Stian Selnes (stianse)
committed Details | Review

Description Stian Selnes (stianse) 2015-07-03 09:19:34 UTC
Created attachment 306687 [details] [review]
Patch with fix and tests

Attached patch fixes the validation of rtcp buffers with padding. It also adds a few tests for both compound and reduzed size rtcp packets. The padding (if any) is included in the length of the last packet.

Extracts from RFC 3550:

Section 6.4.1:
   padding (P): 1 bit
      If the padding bit is set, this individual RTCP packet contains
      some additional padding octets at the end which are not part of
      the control information but are included in the length field.

Section A.2:
   *  The padding bit (P) should be zero for the first packet of a
      compound RTCP packet because padding should only be applied, if it
      is needed, to the last packet.

   *  The length fields of the individual RTCP packets must add up to
      the overall length of the compound RTCP packet as received.
Comment 1 Sebastian Dröge (slomo) 2015-07-03 09:33:42 UTC
Review of attachment 306687 [details] [review]:

::: gst-libs/gst/rtp/gstrtcpbuffer.c
@@ +150,2 @@
     /* get padding */
+    pad_bytes = data[-1];

Don't use negative array indices please

@@ +150,3 @@
     /* get padding */
+    pad_bytes = data[-1];
+    if (pad_bytes == 0 || pad_bytes >= header_len)

The value of the padding should be the length of the packet? Or ...?

::: tests/check/libs/rtp.c
@@ +824,3 @@
+
+  fail_unless (gst_rtcp_buffer_validate_data (rtcp_pkt,
+          sizeof (rtcp_pkt)) == TRUE);

Don't compare to TRUE or FALSE, just do fail_if() or fail_unless()
Comment 2 Stian Selnes (stianse) 2015-07-03 22:45:13 UTC
Created attachment 306786 [details] [review]
Patch with fix and tests

Changed previous patch according to code review and also changed the wrong_padding check.

The commit message includes a description of what the value of the so called pad byte should be. It's not the length of the packet as the previous code assumed, but it's the number of padded bytes at the end of the packet (including itself). I also changed the check for a valid pad byte value to be more in line what is explicitly stated in the RFC. It must be a multiple of 4, and the value 0 cannot be allowed since the count includes the pad byte itself. (Previous patch only check the upper and lower limit that made sense, but I realize that was a bit confusing).
Comment 3 Sebastian Dröge (slomo) 2015-07-06 09:09:22 UTC
commit 1586981b1bf88016ae2e39bde18e111306fbf7f2
Author: Stian Selnes <stian@pexip.com>
Date:   Thu Jul 2 20:50:00 2015 +0200

    rtcpbuffer: Fix validation of packets with padding
    
    The padding (if any) is included in the length of the last packet, see
    RFC 3550.
    
    Section 6.4.1:
       padding (P): 1 bit
          If the padding bit is set, this individual RTCP packet contains
          some additional padding octets at the end which are not part of
          the control information but are included in the length field. The
          last octet of the padding is a count of how many padding octets
          should be ignored, including itself (it will be a multiple of
          four).
    
    Section A.2:
       *  The padding bit (P) should be zero for the first packet of a
          compound RTCP packet because padding should only be applied, if it
          is needed, to the last packet.
    
       *  The length fields of the individual RTCP packets must add up to
          the overall length of the compound RTCP packet as received.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=751883