<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6514">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit

</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">AST-2017-008: Improve RTP and RTCP packet processing.<br><br>Validate RTCP packets before processing them.<br><br>* Validate that the received packet is of a minimum length and apply the<br>RFC3550 RTCP packet validation checks.<br><br>* Fixed potentially reading garbage beyond the received RTCP record data.<br><br>* Fixed rtp->themssrc only being set once when the remote could change<br>the SSRC.  We would effectively stop handling the RTCP statistic records.<br><br>* Fixed rtp->themssrc to not treat a zero value as special by adding<br>rtp->themssrc_valid to indicate if rtp->themssrc is available.<br><br>ASTERISK-27274<br><br>Make strict RTP learning more flexible.<br><br>Direct media can cause strict RTP to attempt to learn a remote address<br>again before it has had a chance to learn the remote address the first<br>time.  Because of the rapid relearn requests, strict RTP could latch onto<br>the first remote address and fail to latch onto the direct media remote<br>address.  As a result, you have one way audio until the call is placed on<br>and off hold.<br><br>The new algorithm learns remote addresses for a set time (1.5 seconds)<br>before locking the remote address.  In addition, we must see a configured<br>number of remote packets from the same address in a row before switching.<br><br>* Fixed strict RTP learning from always accepting the first new address<br>packet as the new stream.<br><br>* Fixed strict RTP to initialize the expected sequence number with the<br>last received sequence number instead of the last transmitted sequence<br>number.<br><br>* Fixed the predicted next sequence number calculation in<br>rtp_learning_rtp_seq_update() to handle overflow.<br><br>ASTERISK-27252<br><br>Change-Id: Ia2d3aa6e0f22906c25971e74f10027d96525f31c<br>---<br>M res/res_rtp_asterisk.c<br>1 file changed, 415 insertions(+), 105 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c<br>index 018af0d..8bc6ca5 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -117,7 +117,9 @@<br>      STRICT_RTP_CLOSED,   /*! Drop all RTP packets not coming from source that was learned */<br> };<br> <br>-#define DEFAULT_STRICT_RTP STRICT_RTP_CLOSED<br>+#define STRICT_RTP_LEARN_TIMEOUT      1500    /*!< milliseconds */<br>+<br>+#define DEFAULT_STRICT_RTP -1      /*!< Enabled */<br> #define DEFAULT_ICESUPPORT 1<br> <br> extern struct ast_srtp_res *res_srtp;<br>@@ -218,9 +220,11 @@<br> <br> /*! \brief RTP learning mode tracking information */<br> struct rtp_learning_info {<br>-    int max_seq;     /*!< The highest sequence number received */<br>-     int packets;     /*!< The number of remaining packets before the source is accepted */<br>+    struct ast_sockaddr proposed_address;   /*!< Proposed remote address for strict RTP */<br>+    struct timeval start;   /*!< The time learning mode was started */<br>         struct timeval received; /*!< The time of the last received packet */<br>+     int max_seq;    /*!< The highest sequence number received */<br>+      int packets;    /*!< The number of remaining packets before the source is accepted */<br> };<br> <br> #ifdef HAVE_OPENSSL_SRTP<br>@@ -249,7 +253,7 @@<br>      unsigned char rawdata[8192 + AST_FRIENDLY_OFFSET];<br>    unsigned int ssrc;              /*!< Synchronization source, RFC 3550, page 10. */<br>         unsigned int themssrc;          /*!< Their SSRC */<br>-        unsigned int rxssrc;<br>+ unsigned int themssrc_valid;    /*!< True if their SSRC is available. */<br>   unsigned int lastts;<br>  unsigned int lastrxts;<br>        unsigned int lastividtimestamp;<br>@@ -1940,7 +1944,7 @@<br> #endif<br> <br> #ifdef HAVE_PJPROJECT<br>-static void rtp_learning_seq_init(struct rtp_learning_info *info, uint16_t seq);<br>+static void rtp_learning_start(struct ast_rtp *rtp);<br> <br> /* PJPROJECT ICE callback */<br> static void ast_rtp_on_ice_complete(pj_ice_sess *ice, pj_status_t status)<br>@@ -1979,8 +1983,8 @@<br>             return;<br>       }<br> <br>- rtp->strict_rtp_state = STRICT_RTP_LEARN;<br>- rtp_learning_seq_init(&rtp->rtp_source_learn, (uint16_t)rtp->seqno);<br>+       ast_verb(4, "%p -- Strict RTP learning after ICE completion\n", rtp);<br>+      rtp_learning_start(rtp);<br>      ao2_unlock(instance);<br> }<br> <br>@@ -2704,7 +2708,7 @@<br>  */<br> static void rtp_learning_seq_init(struct rtp_learning_info *info, uint16_t seq)<br> {<br>-      info->max_seq = seq - 1;<br>+  info->max_seq = seq;<br>       info->packets = learning_min_sequential;<br>   memset(&info->received, 0, sizeof(info->received));<br> }<br>@@ -2721,14 +2725,17 @@<br>  */<br> static int rtp_learning_rtp_seq_update(struct rtp_learning_info *info, uint16_t seq)<br> {<br>+  /*<br>+    * During the learning mode the minimum amount of media we'll accept is<br>+   * 10ms so give a reasonable 5ms buffer just in case we get it sporadically.<br>+  */<br>   if (!ast_tvzero(info->received) && ast_tvdiff_ms(ast_tvnow(), info->received) < 5) {<br>-                /* During the probation period the minimum amount of media we'll accept is<br>-                * 10ms so give a reasonable 5ms buffer just in case we get it sporadically.<br>+         /*<br>+            * Reject a flood of packets as acceptable for learning.<br>+              * Reset the needed packets.<br>           */<br>-          return 1;<br>-    }<br>-<br>- if (seq == info->max_seq + 1) {<br>+           info->packets = learning_min_sequential - 1;<br>+      } else if (seq == (uint16_t) (info->max_seq + 1)) {<br>                /* packet is in sequence */<br>           info->packets--;<br>   } else {<br>@@ -2738,7 +2745,23 @@<br>      info->max_seq = seq;<br>       info->received = ast_tvnow();<br> <br>-  return (info->packets == 0);<br>+      return info->packets;<br>+}<br>+<br>+/*!<br>+ * \brief Start the strictrtp learning mode.<br>+ *<br>+ * \param rtp RTP session description<br>+ *<br>+ * \return Nothing<br>+ */<br>+static void rtp_learning_start(struct ast_rtp *rtp)<br>+{<br>+  rtp->strict_rtp_state = STRICT_RTP_LEARN;<br>+ memset(&rtp->rtp_source_learn.proposed_address, 0,<br>+            sizeof(rtp->rtp_source_learn.proposed_address));<br>+  rtp->rtp_source_learn.start = ast_tvnow();<br>+        rtp_learning_seq_init(&rtp->rtp_source_learn, (uint16_t) rtp->lastrxseqno);<br> }<br> <br> #ifdef HAVE_PJPROJECT<br>@@ -3012,9 +3035,6 @@<br>   rtp->ssrc = ast_random();<br>  rtp->seqno = ast_random() & 0x7fff;<br>    rtp->strict_rtp_state = (strictrtp ? STRICT_RTP_CLOSED : STRICT_RTP_OPEN);<br>-        if (strictrtp) {<br>-             rtp_learning_seq_init(&rtp->rtp_source_learn, (uint16_t)rtp->seqno);<br>-       }<br> <br>  /* Create a new socket for us to listen on and use */<br>         if ((rtp->s =<br>@@ -3588,7 +3608,7 @@<br>       struct ast_sockaddr remote_address = { { 0, } };<br>      struct ast_rtp_rtcp_report_block *report_block = NULL;<br>        RAII_VAR(struct ast_rtp_rtcp_report *, rtcp_report,<br>-                  ast_rtp_rtcp_report_alloc(rtp->themssrc ? 1 : 0),<br>+                 ast_rtp_rtcp_report_alloc(rtp->themssrc_valid ? 1 : 0),<br>                    ao2_cleanup);<br> <br>      if (!rtp || !rtp->rtcp) {<br>@@ -3608,7 +3628,7 @@<br>   calculate_lost_packet_statistics(rtp, &lost_packets, &fraction_lost);<br> <br>      gettimeofday(&now, NULL);<br>-        rtcp_report->reception_report_count = rtp->themssrc ? 1 : 0;<br>+   rtcp_report->reception_report_count = rtp->themssrc_valid ? 1 : 0;<br>      rtcp_report->ssrc = rtp->ssrc;<br>  rtcp_report->type = sr ? RTCP_PT_SR : RTCP_PT_RR;<br>  if (sr) {<br>@@ -3618,7 +3638,7 @@<br>              rtcp_report->sender_information.octet_count = rtp->txoctetcount;<br>        }<br> <br>- if (rtp->themssrc) {<br>+      if (rtp->themssrc_valid) {<br>                 report_block = ast_calloc(1, sizeof(*report_block));<br>          if (!report_block) {<br>                  return 1;<br>@@ -3961,6 +3981,10 @@<br>                     /*<br>                     * RTCP was stopped.<br>                   */<br>+                  return 0;<br>+            }<br>+            if (!rtp->themssrc_valid) {<br>+                       /* We don't know their SSRC value so we don't know who to update. */<br>                  return 0;<br>             }<br> <br>@@ -4543,75 +4567,265 @@<br>        rtp->rtcp->reported_normdev_lost = reported_normdev_lost_current;<br> }<br> <br>+static const char *rtcp_payload_type2str(unsigned int pt)<br>+{<br>+       const char *str;<br>+<br>+  switch (pt) {<br>+        case RTCP_PT_SR:<br>+             str = "Sender Report";<br>+             break;<br>+       case RTCP_PT_RR:<br>+             str = "Receiver Report";<br>+           break;<br>+       case RTCP_PT_FUR:<br>+            /* Full INTRA-frame Request / Fast Update Request */<br>+         str = "H.261 FUR";<br>+         break;<br>+       case RTCP_PT_PSFB:<br>+           /* Payload Specific Feed Back */<br>+             str = "PSFB";<br>+              break;<br>+       case RTCP_PT_SDES:<br>+           str = "Source Description";<br>+                break;<br>+       case RTCP_PT_BYE:<br>+            str = "BYE";<br>+               break;<br>+       default:<br>+             str = "Unknown";<br>+           break;<br>+       }<br>+    return str;<br>+}<br>+<br>+/*<br>+ * Unshifted RTCP header bit field masks<br>+ */<br>+#define RTCP_LENGTH_MASK                     0xFFFF<br>+#define RTCP_PAYLOAD_TYPE_MASK         0xFF<br>+#define RTCP_REPORT_COUNT_MASK           0x1F<br>+#define RTCP_PADDING_MASK                        0x01<br>+#define RTCP_VERSION_MASK                        0x03<br>+<br>+/*<br>+ * RTCP header bit field shift offsets<br>+ */<br>+#define RTCP_LENGTH_SHIFT                 0<br>+#define RTCP_PAYLOAD_TYPE_SHIFT             16<br>+#define RTCP_REPORT_COUNT_SHIFT            24<br>+#define RTCP_PADDING_SHIFT                 29<br>+#define RTCP_VERSION_SHIFT                 30<br>+<br>+#define RTCP_VERSION                            2U<br>+#define RTCP_VERSION_SHIFTED               (RTCP_VERSION << RTCP_VERSION_SHIFT)<br>+#define RTCP_VERSION_MASK_SHIFTED  (RTCP_VERSION_MASK << RTCP_VERSION_SHIFT)<br>+<br>+/*<br>+ * RTCP first packet record validity header mask and value.<br>+ *<br>+ * RFC3550 intentionally defines the encoding of RTCP_PT_SR and RTCP_PT_RR<br>+ * such that they differ in the least significant bit.  Either of these two<br>+ * payload types MUST be the first RTCP packet record in a compound packet.<br>+ *<br>+ * RFC3550 checks the padding bit in the algorithm they use to check the<br>+ * RTCP packet for validity.  However, we aren't masking the padding bit<br>+ * to check since we don't know if it is a compound RTCP packet or not.<br>+ */<br>+#define RTCP_VALID_MASK (RTCP_VERSION_MASK_SHIFTED | (((RTCP_PAYLOAD_TYPE_MASK & ~0x1)) << RTCP_PAYLOAD_TYPE_SHIFT))<br>+#define RTCP_VALID_VALUE (RTCP_VERSION_SHIFTED | (RTCP_PT_SR << RTCP_PAYLOAD_TYPE_SHIFT))<br>+<br>+#define RTCP_SR_BLOCK_WORD_LENGTH 5<br>+#define RTCP_RR_BLOCK_WORD_LENGTH 6<br>+#define RTCP_HEADER_SSRC_LENGTH   2<br>+<br> static struct ast_frame *ast_rtcp_interpret(struct ast_rtp_instance *instance, const unsigned char *rtcpdata, size_t size, struct ast_sockaddr *addr)<br> {<br>         struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);<br>    unsigned int *rtcpheader = (unsigned int *)(rtcpdata);<br>-       int packetwords, position = 0;<br>+       unsigned int packetwords;<br>+    unsigned int position;<br>+       unsigned int first_word;<br>+     /*! True if we have seen an acceptable SSRC to learn the remote RTCP address */<br>+      unsigned int ssrc_seen;<br>       int report_counter = 0;<br>       struct ast_rtp_rtcp_report_block *report_block;<br>       struct ast_frame *f = &ast_null_frame;<br> <br>         packetwords = size / 4;<br> <br>-   ast_debug(1, "Got RTCP report of %zu bytes\n", size);<br>+      ast_debug(1, "Got RTCP report of %zu bytes from %s\n",<br>+             size, ast_sockaddr_stringify(addr));<br> <br>+      /*<br>+    * Validate the RTCP packet according to an adapted and slightly<br>+      * modified RFC3550 validation algorithm.<br>+     */<br>+  if (packetwords < RTCP_HEADER_SSRC_LENGTH) {<br>+              ast_debug(1, "%p -- RTCP from %s: Frame size (%u words) is too short\n",<br>+                   rtp, ast_sockaddr_stringify(addr), packetwords);<br>+             return &ast_null_frame;<br>+  }<br>+    position = 0;<br>+        first_word = ntohl(rtcpheader[position]);<br>+    if ((first_word & RTCP_VALID_MASK) != RTCP_VALID_VALUE) {<br>+                ast_debug(1, "%p -- RTCP from %s: Failed first packet validity check\n",<br>+                   rtp, ast_sockaddr_stringify(addr));<br>+          return &ast_null_frame;<br>+  }<br>+    do {<br>+         position += ((first_word >> RTCP_LENGTH_SHIFT) & RTCP_LENGTH_MASK) + 1;<br>+            if (packetwords <= position) {<br>+                    break;<br>+               }<br>+            first_word = ntohl(rtcpheader[position]);<br>+    } while ((first_word & RTCP_VERSION_MASK_SHIFTED) == RTCP_VERSION_SHIFTED);<br>+      if (position != packetwords) {<br>+               ast_debug(1, "%p -- RTCP from %s: Failed packet version or length check\n",<br>+                        rtp, ast_sockaddr_stringify(addr));<br>+          return &ast_null_frame;<br>+  }<br>+<br>+ /*<br>+    * Note: RFC3605 points out that true NAT (vs NAPT) can cause RTCP<br>+    * to have a different IP address and port than RTP.  Otherwise, when<br>+         * strictrtp is enabled we could reject RTCP packets not coming from<br>+  * the learned RTP IP address if it is available.<br>+     */<br>+<br>+       /*<br>+    * strictrtp safety needs SSRC to match before we use the<br>+     * sender's address for symmetrical RTP to send our RTCP<br>+  * reports.<br>+   *<br>+    * If strictrtp is not enabled then claim to have already seen<br>+        * a matching SSRC so we'll accept this packet's address for<br>+  * symmetrical RTP.<br>+   */<br>+  ssrc_seen = rtp->strict_rtp_state == STRICT_RTP_OPEN;<br>+<br>+  position = 0;<br>         while (position < packetwords) {<br>-          int i, pt, rc;<br>+               unsigned int i;<br>+              unsigned int pt;<br>+             unsigned int rc;<br>+             unsigned int ssrc;<br>+           /*! True if the ssrc value we have is valid and not garbage because it doesn't exist. */<br>+         unsigned int ssrc_valid;<br>              unsigned int length;<br>+         unsigned int min_length;<br>+<br>           struct ast_json *message_blob;<br>                RAII_VAR(struct ast_rtp_rtcp_report *, rtcp_report, NULL, ao2_cleanup);<br> <br>            i = position;<br>-                length = ntohl(rtcpheader[i]);<br>-               pt = (length & 0xff0000) >> 16;<br>-            rc = (length & 0x1f000000) >> 24;<br>-          length &= 0xffff;<br>+                first_word = ntohl(rtcpheader[i]);<br>+           pt = (first_word >> RTCP_PAYLOAD_TYPE_SHIFT) & RTCP_PAYLOAD_TYPE_MASK;<br>+             rc = (first_word >> RTCP_REPORT_COUNT_SHIFT) & RTCP_REPORT_COUNT_MASK;<br>+             /* RFC3550 says 'length' is the number of words in the packet - 1 */<br>+         length = ((first_word >> RTCP_LENGTH_SHIFT) & RTCP_LENGTH_MASK) + 1;<br> <br>-            rtcp_report = ast_rtp_rtcp_report_alloc(rc);<br>-         if (!rtcp_report) {<br>-                  return &ast_null_frame;<br>-          }<br>-            rtcp_report->reception_report_count = rc;<br>-         rtcp_report->ssrc = ntohl(rtcpheader[i + 1]);<br>-<br>-          if ((i + length) > packetwords) {<br>-                 if (rtpdebug) {<br>-                              ast_debug(1, "RTCP Read too short\n");<br>+             /* Check expected RTCP packet record length */<br>+               min_length = RTCP_HEADER_SSRC_LENGTH;<br>+                switch (pt) {<br>+                case RTCP_PT_SR:<br>+                     min_length += RTCP_SR_BLOCK_WORD_LENGTH;<br>+                     /* fall through */<br>+           case RTCP_PT_RR:<br>+                     min_length += (rc * RTCP_RR_BLOCK_WORD_LENGTH);<br>+                      break;<br>+               case RTCP_PT_FUR:<br>+            case RTCP_PT_PSFB:<br>+                   break;<br>+               case RTCP_PT_SDES:<br>+           case RTCP_PT_BYE:<br>+                    /*<br>+                    * There may not be a SSRC/CSRC present.  The packet is<br>+                       * useless but still valid if it isn't present.<br>+                   *<br>+                    * We don't know what min_length should be so disable the check<br>+                   */<br>+                  min_length = length;<br>+                 break;<br>+               default:<br>+                     ast_debug(1, "%p -- RTCP from %s: %u(%s) skipping record\n",<br>+                               rtp, ast_sockaddr_stringify(addr), pt, rtcp_payload_type2str(pt));<br>+                   if (rtcp_debug_test_addr(addr)) {<br>+                            ast_verbose("\n");<br>+                         ast_verbose("RTCP from %s: %u(%s) skipping record\n",<br>+                                      ast_sockaddr_stringify(addr), pt, rtcp_payload_type2str(pt));<br>                         }<br>-                    return &ast_null_frame;<br>-          }<br>-<br>-         if ((rtp->strict_rtp_state != STRICT_RTP_OPEN) && (rtcp_report->ssrc != rtp->themssrc)) {<br>-                   /* Skip over this RTCP record as it does not contain the correct SSRC */<br>-                     position += (length + 1);<br>-                    ast_debug(1, "%p -- Received RTCP report from %s, dropping due to strict RTP protection. Received SSRC '%u' but expected '%u'\n",<br>-                          rtp, ast_sockaddr_stringify(addr), rtcp_report->ssrc, rtp->themssrc);<br>+                  position += length;<br>                   continue;<br>             }<br>+            if (length < min_length) {<br>+                        ast_debug(1, "%p -- RTCP from %s: %u(%s) length field less than expected minimum.  Min:%u Got:%u\n",<br>+                               rtp, ast_sockaddr_stringify(addr), pt, rtcp_payload_type2str(pt),<br>+                            min_length - 1, length - 1);<br>+                 return &ast_null_frame;<br>+          }<br> <br>-         if (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_NAT)) {<br>+             /* Get the RTCP record SSRC if defined for the record */<br>+             ssrc_valid = 1;<br>+              switch (pt) {<br>+                case RTCP_PT_SR:<br>+             case RTCP_PT_RR:<br>+                     rtcp_report = ast_rtp_rtcp_report_alloc(rc);<br>+                 if (!rtcp_report) {<br>+                          return &ast_null_frame;<br>+                  }<br>+                    rtcp_report->reception_report_count = rc;<br>+<br>+                      ssrc = ntohl(rtcpheader[i + 1]);<br>+                     rtcp_report->ssrc = ssrc;<br>+                 break;<br>+               case RTCP_PT_FUR:<br>+            case RTCP_PT_PSFB:<br>+                   ssrc = ntohl(rtcpheader[i + 1]);<br>+                     break;<br>+               case RTCP_PT_SDES:<br>+           case RTCP_PT_BYE:<br>+            default:<br>+                     ssrc = 0;<br>+                    ssrc_valid = 0;<br>+                      break;<br>+               }<br>+<br>+         if (rtcp_debug_test_addr(addr)) {<br>+                    ast_verbose("\n");<br>+                 ast_verbose("RTCP from %s\n", ast_sockaddr_stringify(addr));<br>+                       ast_verbose("PT: %u(%s)\n", pt, rtcp_payload_type2str(pt));<br>+                        ast_verbose("Reception reports: %u\n", rc);<br>+                        ast_verbose("SSRC of sender: %u\n", ssrc);<br>+         }<br>+<br>+         if (ssrc_valid && rtp->themssrc_valid) {<br>+                  if (ssrc != rtp->themssrc) {<br>+                              /*<br>+                            * Skip over this RTCP record as it does not contain the<br>+                              * correct SSRC.  We should not act upon RTCP records<br>+                                 * for a different stream.<br>+                            */<br>+                          position += length;<br>+                          ast_debug(1, "%p -- RTCP from %s: Skipping record, received SSRC '%u' != expected '%u'\n",<br>+                                 rtp, ast_sockaddr_stringify(addr), ssrc, rtp->themssrc);<br>+                          continue;<br>+                    }<br>+                    ssrc_seen = 1;<br>+               }<br>+<br>+         if (ssrc_seen && ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_NAT)) {<br>                         /* Send to whoever sent to us */<br>                      if (ast_sockaddr_cmp(&rtp->rtcp->them, addr)) {<br>                             ast_sockaddr_copy(&rtp->rtcp->them, addr);<br>                          if (rtpdebug) {<br>                                       ast_debug(0, "RTCP NAT: Got RTCP from other end. Now sending to address %s\n",<br>-                                             ast_sockaddr_stringify(&rtp->rtcp->them));<br>+                                         ast_sockaddr_stringify(addr));<br>                                }<br>                     }<br>             }<br> <br>-         if (rtcp_debug_test_addr(addr)) {<br>-                    ast_verbose("\n\nGot RTCP from %s\n",<br>-                                  ast_sockaddr_stringify(addr));<br>-                   ast_verbose("PT: %d(%s)\n", pt, (pt == RTCP_PT_SR) ? "Sender Report" :<br>-                                                   (pt == RTCP_PT_RR) ? "Receiver Report" :<br>-                                                   (pt == RTCP_PT_FUR) ? "H.261 FUR" : "Unknown");<br>-                  ast_verbose("Reception reports: %d\n", rc);<br>-                        ast_verbose("SSRC of sender: %u\n", rtcp_report->ssrc);<br>-         }<br>-<br>-         i += 2; /* Advance past header and ssrc */<br>+           i += RTCP_HEADER_SSRC_LENGTH; /* Advance past header and ssrc */<br>              switch (pt) {<br>                 case RTCP_PT_SR:<br>                      gettimeofday(&rtp->rtcp->rxlsr, NULL);<br>@@ -4635,7 +4849,7 @@<br>                                               rtcp_report->sender_information.packet_count,<br>                                              rtcp_report->sender_information.octet_count);<br>                      }<br>-                    i += 5;<br>+                      i += RTCP_SR_BLOCK_WORD_LENGTH;<br>                       /* Intentional fall through */<br>                case RTCP_PT_RR:<br>                      if (rtcp_report->type != RTCP_PT_SR) {<br>@@ -4692,9 +4906,9 @@<br>                       */<br> <br>                        message_blob = ast_json_pack("{s: s, s: s, s: f}",<br>-                                 "from", ast_sockaddr_stringify(&rtp->rtcp->them),<br>-                                        "to", rtp->rtcp->local_addr_str,<br>-                                     "rtt", rtp->rtcp->rtt);<br>+                              "from", ast_sockaddr_stringify(addr),<br>+                              "to", rtp->rtcp->local_addr_str,<br>+                             "rtt", rtp->rtcp->rtt);<br>                       ast_rtp_publish_rtcp_message(instance, ast_rtp_rtcp_received_type(),<br>                                  rtcp_report,<br>                                  message_blob);<br>@@ -4717,26 +4931,23 @@<br>               case RTCP_PT_SDES:<br>                    if (rtcp_debug_test_addr(addr)) {<br>                             ast_verbose("Received an SDES from %s\n",<br>-                                      ast_sockaddr_stringify(&rtp->rtcp->them));<br>+                                     ast_sockaddr_stringify(addr));<br>                        }<br>                     break;<br>                case RTCP_PT_BYE:<br>                     if (rtcp_debug_test_addr(addr)) {<br>                             ast_verbose("Received a BYE from %s\n",<br>-                                        ast_sockaddr_stringify(&rtp->rtcp->them));<br>+                                     ast_sockaddr_stringify(addr));<br>                        }<br>                     break;<br>                default:<br>-                     ast_debug(1, "Unknown RTCP packet (pt=%d) received from %s\n",<br>-                               pt, ast_sockaddr_stringify(&rtp->rtcp->them));<br>                    break;<br>                }<br>-            position += (length + 1);<br>+            position += length;<br>   }<br>     rtp->rtcp->rtcp_info = 1;<br> <br>    return f;<br>-<br> }<br> <br> /*! \pre instance is locked */<br>@@ -5007,32 +5218,139 @@<br>                return &ast_null_frame;<br>   }<br> <br>- /* If strict RTP protection is enabled see if we need to learn the remote address or if we need to drop the packet */<br>-        if (rtp->strict_rtp_state == STRICT_RTP_LEARN) {<br>-          if (!ast_sockaddr_cmp(&rtp->strict_rtp_address, &addr)) {<br>-                 /* We are learning a new address but have received traffic from the existing address,<br>-                         * accept it but reset the current learning for the new source so it only takes over<br>-                  * once sufficient traffic has been received. */<br>-                     rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br>-         } else {<br>-                     /* Start trying to learn from the new address. If we pass a probationary period with<br>-                  * it, that means we've stopped getting RTP from the original source and we should<br>-                        * switch to it.<br>-                      */<br>-                  if (rtp_learning_rtp_seq_update(&rtp->rtp_source_learn, seqno)) {<br>-                             ast_debug(1, "%p -- Received RTP packet from %s, dropping due to strict RTP protection. Will switch to it in %d packets\n",<br>-                                        rtp, ast_sockaddr_stringify(&addr), rtp->rtp_source_learn.packets);<br>-                           return &ast_null_frame;<br>-                  }<br>-                    ast_sockaddr_copy(&rtp->strict_rtp_address, &addr);<br>+       /* If the version is not what we expected by this point then just drop the packet */<br>+ if (version != 2) {<br>+          return &ast_null_frame;<br>+  }<br> <br>-                 ast_verb(4, "%p -- Probation passed - setting RTP source address to %s\n", rtp, ast_sockaddr_stringify(&addr));<br>+        /* If strict RTP protection is enabled see if we need to learn the remote address or if we need to drop the packet */<br>+        switch (rtp->strict_rtp_state) {<br>+  case STRICT_RTP_LEARN:<br>+               /*<br>+            * Scenario setup:<br>+            * PartyA -- Ast1 -- Ast2 -- PartyB<br>+           *<br>+            * The learning timeout is necessary for Ast1 to handle the above<br>+             * setup where PartyA calls PartyB and Ast2 initiates direct media<br>+            * between Ast1 and PartyB.  Ast1 may lock onto the Ast2 stream and<br>+           * never learn the PartyB stream when it starts.  The timeout makes<br>+           * Ast1 stay in the learning state long enough to see and learn the<br>+           * RTP stream from PartyB.<br>+            *<br>+            * To mitigate against attack, the learning state cannot switch<br>+               * streams while there are competing streams.  The competing streams<br>+          * interfere with each other's qualification.  Once we accept a<br>+           * stream and reach the timeout, an attacker cannot interfere<br>+                 * anymore.<br>+           *<br>+            * Here are a few scenarios and each one assumes that the streams<br>+             * are continuous:<br>+            *<br>+            * 1) We already have a known stream source address and the known<br>+             * stream wants to change to a new source address.  An attacking<br>+              * stream will block learning the new stream source.  After the<br>+               * timeout we re-lock onto the original stream source address which<br>+           * likely went away.  The result is one way audio.<br>+            *<br>+            * 2) We already have a known stream source address and the known<br>+             * stream doesn't want to change source addresses.  An attacking<br>+          * stream will not be able to replace the known stream.  After the<br>+            * timeout we re-lock onto the known stream.  The call is not<br>+                 * affected.<br>+          *<br>+            * 3) We don't have a known stream source address.  This presumably<br>+               * is the start of a call.  Competing streams will result in staying<br>+          * in learning mode until a stream becomes the victor and we reach<br>+            * the timeout.  We cannot exit learning if we have no known stream<br>+           * to lock onto.  The result is one way audio until there is a victor.<br>+                *<br>+            * If we learn a stream source address before the timeout we will be<br>+          * in scenario 1) or 2) when a competing stream starts.<br>+               */<br>+          if (!ast_sockaddr_isnull(&rtp->strict_rtp_address)<br>+                    && STRICT_RTP_LEARN_TIMEOUT < ast_tvdiff_ms(ast_tvnow(), rtp->rtp_source_learn.start)) {<br>+                       ast_verb(4, "%p -- Strict RTP learning complete - Locking on source address %s\n",<br>+                         rtp, ast_sockaddr_stringify(&rtp->strict_rtp_address));<br>                        rtp->strict_rtp_state = STRICT_RTP_CLOSED;<br>+                } else {<br>+                     struct ast_sockaddr target_address;<br>+<br>+                       if (!ast_sockaddr_cmp(&rtp->strict_rtp_address, &addr)) {<br>+                         /*<br>+                            * We are open to learning a new address but have received<br>+                            * traffic from the current address, accept it and reset<br>+                              * the learning counts for a new source.  When no more<br>+                                * current source packets arrive a new source can take over<br>+                           * once sufficient traffic is received.<br>+                               */<br>+                          rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br>+                         break;<br>+                       }<br>+<br>+                 /*<br>+                    * We give preferential treatment to the requested target address<br>+                     * (negotiated SDP address) where we are to send our RTP.  However,<br>+                   * the other end has no obligation to send from that address even<br>+                     * though it is practically a requirement when NAT is involved.<br>+                       */<br>+                  ast_rtp_instance_get_requested_target_address(instance, &target_address);<br>+                        if (!ast_sockaddr_cmp(&target_address, &addr)) {<br>+                             /* Accept the negotiated target RTP stream as the source */<br>+                          ast_verb(4, "%p -- Strict RTP switching to RTP target address %s as source\n",<br>+                                     rtp, ast_sockaddr_stringify(&addr));<br>+                             ast_sockaddr_copy(&rtp->strict_rtp_address, &addr);<br>+                               rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br>+                         break;<br>+                       }<br>+<br>+                 /*<br>+                    * Trying to learn a new address.  If we pass a probationary period<br>+                   * with it, that means we've stopped getting RTP from the original<br>+                        * source and we should switch to it.<br>+                         */<br>+                  if (!ast_sockaddr_cmp(&rtp->rtp_source_learn.proposed_address, &addr)) {<br>+                          if (!rtp_learning_rtp_seq_update(&rtp->rtp_source_learn, seqno)) {<br>+                                    /* Accept the new RTP stream */<br>+                                      ast_verb(4, "%p -- Strict RTP switching source address to %s\n",<br>+                                           rtp, ast_sockaddr_stringify(&addr));<br>+                                     ast_sockaddr_copy(&rtp->strict_rtp_address, &addr);<br>+                                       rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br>+                                 break;<br>+                               }<br>+                            /* Not ready to accept the RTP stream candidate */<br>+                           ast_debug(1, "%p -- Received RTP packet from %s, dropping due to strict RTP protection. Will switch to it in %d packets.\n",<br>+                                       rtp, ast_sockaddr_stringify(&addr), rtp->rtp_source_learn.packets);<br>+                   } else {<br>+                             /*<br>+                            * This is either an attacking stream or<br>+                              * the start of the expected new stream.<br>+                              */<br>+                          ast_sockaddr_copy(&rtp->rtp_source_learn.proposed_address, &addr);<br>+                                rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);<br>+                         ast_debug(1, "%p -- Received RTP packet from %s, dropping due to strict RTP protection. Qualifying new stream.\n",<br>+                                 rtp, ast_sockaddr_stringify(&addr));<br>+                     }<br>+                    return &ast_null_frame;<br>           }<br>-    } else if (rtp->strict_rtp_state == STRICT_RTP_CLOSED && ast_sockaddr_cmp(&rtp->strict_rtp_address, &addr)) {<br>+          /* Fall through */<br>+   case STRICT_RTP_CLOSED:<br>+              /*<br>+            * We should not allow a stream address change if the SSRC matches<br>+            * once strictrtp learning is closed.  Any kind of address change<br>+             * like this should have happened while we were in the learning<br>+               * state.  We do not want to allow the possibility of an attacker<br>+             * interfering with the RTP stream after the learning period.<br>+                 * An attacker could manage to get an RTCP packet redirected to<br>+               * them which can contain the SSRC value.<br>+             */<br>+          if (!ast_sockaddr_cmp(&rtp->strict_rtp_address, &addr)) {<br>+                 break;<br>+               }<br>             ast_debug(1, "%p -- Received RTP packet from %s, dropping due to strict RTP protection.\n",<br>                         rtp, ast_sockaddr_stringify(&addr));<br>              return &ast_null_frame;<br>+  case STRICT_RTP_OPEN:<br>+                break;<br>        }<br> <br>  /* If symmetric RTP is enabled see if the remote side is not what we expected and change where we are sending audio */<br>@@ -5060,11 +5378,6 @@<br>                return &ast_null_frame;<br>   }<br> <br>- /* If the version is not what we expected by this point then just drop the packet */<br>- if (version != 2) {<br>-          return &ast_null_frame;<br>-  }<br>-<br>  /* Pull out the various other fields we will need */<br>  payloadtype = (seqno & 0x7f0000) >> 16;<br>     padding = seqno & (1 << 29);<br>@@ -5077,7 +5390,7 @@<br> <br>      AST_LIST_HEAD_INIT_NOLOCK(&frames);<br>       /* Force a marker bit and change SSRC if the SSRC changes */<br>- if (rtp->rxssrc && rtp->rxssrc != ssrc) {<br>+      if (rtp->themssrc_valid && rtp->themssrc != ssrc) {<br>             struct ast_frame *f, srcupdate = {<br>                    AST_FRAME_CONTROL,<br>                    .subclass.integer = AST_CONTROL_SRCCHANGE,<br>@@ -5105,8 +5418,8 @@<br>                     rtp->rtcp->received_prior = 0;<br>          }<br>     }<br>-<br>- rtp->rxssrc = ssrc;<br>+       rtp->themssrc = ssrc; /* Record their SSRC to put in future RR */<br>+ rtp->themssrc_valid = 1;<br> <br>        /* Remove any padding bytes that may be present */<br>    if (padding) {<br>@@ -5159,10 +5472,6 @@<br> <br>     prev_seqno = rtp->lastrxseqno;<br>     rtp->lastrxseqno = seqno;<br>-<br>-      if (!rtp->themssrc) {<br>-             rtp->themssrc = ntohl(rtpheader[2]); /* Record their SSRC to put in future RR */<br>-  }<br> <br>  if (rtp_debug_test_addr(&addr)) {<br>                 ast_verbose("Got  RTP packet from    %s (type %-2.2d, seq %-6.6u, ts %-6.6u, len %-6.6d)\n",<br>@@ -5538,13 +5847,14 @@<br> <br>    rtp->rxseqno = 0;<br> <br>-      if (strictrtp && rtp->strict_rtp_state != STRICT_RTP_OPEN && !ast_sockaddr_isnull(addr) &&<br>-                ast_sockaddr_cmp(addr, &rtp->strict_rtp_address)) {<br>+   if (strictrtp && rtp->strict_rtp_state != STRICT_RTP_OPEN<br>+         && !ast_sockaddr_isnull(addr) && ast_sockaddr_cmp(addr, &rtp->strict_rtp_address)) {<br>           /* We only need to learn a new strict source address if we've been told the source is<br>              * changing to something different.<br>            */<br>-          rtp->strict_rtp_state = STRICT_RTP_LEARN;<br>-         rtp_learning_seq_init(&rtp->rtp_source_learn, rtp->seqno);<br>+         ast_verb(4, "%p -- Strict RTP learning after remote address set to: %s\n",<br>+                 rtp, ast_sockaddr_stringify(addr));<br>+          rtp_learning_start(rtp);<br>      }<br> }<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6514">change 6514</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6514"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13.17 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Ia2d3aa6e0f22906c25971e74f10027d96525f31c </div>
<div style="display:none"> Gerrit-Change-Number: 6514 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>