Avoid buffer-overflow on short RTCP/SRTCP packets

In `srtp_unprotect_rtcp()` we are not validating that the packet
length is as long as the minimum required.  This would cause
`enc_octet_len` to underflow, which would cause us to try to decrypt
data past the end of the packet in memory -- a buffer over-read and
buffer overflow.

In `srtp_protect_rtcp()`, we were similarly not validating the packet
length.  Here we were also polluting the address of the SRTCP
encrypted flag and index (the `trailer`), causing us to write one word
to a bogus memory address before getting to the encryption where we
would also overflow.

In this commit we add checks to appropriately validate the RTCP/SRTCP
packet lengths.

`srtp_unprotect_rtcp_aead()` (but not protect) did correctly validate
the packet length; this check would now be redundant as the check in
`srtcp_unprotect_rtcp()` will also run first, so it has been removed.

diff --git a/libs/srtp/srtp/srtp.c b/libs/srtp/srtp/srtp.c
index 43bf574bde809ed47ca427cd07f78f967f50ac4b..0d2eabffcc55f37b985da90e7cb041f670ebda2c 100644 (file)
--- a/libs/srtp/srtp/srtp.c
+++ b/libs/srtp/srtp/srtp.c
@@ -2342,12 +2342,6 @@ srtp_unprotect_rtcp_aead (srtp_t ctx, srtp_stream_ctx_t *stream,
     /* get tag length from stream context */
     tag_len = auth_get_tag_length(stream->rtcp_auth);
 
-    /* Validate packet length */
-    if (*pkt_octet_len < (octets_in_rtcp_header + tag_len + 
-                          sizeof(srtcp_trailer_t))) {
-        return err_status_bad_param;
-    }
-
     /*
      * set encryption start, encryption length, and trailer
      */
@@ -2520,6 +2514,11 @@ srtp_protect_rtcp(srtp_t ctx, void *rtcp_hdr, int *pkt_octet_len) {
   uint32_t seq_num;
 
   /* we assume the hdr is 32-bit aligned to start */
+
+  /* check the packet length - it must at least contain a full header */
+  if (*pkt_octet_len < octets_in_rtcp_header)
+    return err_status_bad_param;
+
   /*
    * look up ssrc in srtp_stream list, and process the packet with 
    * the appropriate stream.  if we haven't seen this stream before,
@@ -2753,6 +2752,16 @@ srtp_unprotect_rtcp(srtp_t ctx, void *srtcp_hdr, int *pkt_octet_len) {
     } 
   }
   
+  /* get tag length from stream context */
+  tag_len = auth_get_tag_length(stream->rtcp_auth);
+
+  /* check the packet length - it must contain at least a full RTCP
+     header, an auth tag (if applicable), and the SRTCP encrypted flag
+     and 31-bit index value */
+  if (*pkt_octet_len < (octets_in_rtcp_header + tag_len +
+                        sizeof(srtcp_trailer_t)))
+    return err_status_bad_param;
+
   /*
    * Check if this is an AEAD stream (GCM mode).  If so, then dispatch
    * the request to our AEAD handler.
@@ -2765,9 +2774,6 @@ srtp_unprotect_rtcp(srtp_t ctx, void *srtcp_hdr, int *pkt_octet_len) {
   sec_serv_confidentiality = stream->rtcp_services == sec_serv_conf ||
       stream->rtcp_services == sec_serv_conf_and_auth;
 
-  /* get tag length from stream context */
-  tag_len = auth_get_tag_length(stream->rtcp_auth); 
-
   /*
    * set encryption start, encryption length, and trailer
    */