<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6413">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Richard Mudgett: Looks good to me, approved
Joshua Colp: 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, 432 insertions(+), 104 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 f6a0ec0..1440eb0 100644<br>--- a/res/res_rtp_asterisk.c<br>+++ b/res/res_rtp_asterisk.c<br>@@ -125,7 +125,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>@@ -226,9 +228,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>@@ -266,7 +270,7 @@<br> unsigned int ssrc; /*!< Synchronization source, RFC 3550, page 10. */<br> char cname[AST_UUID_STR_LEN]; /*!< Our local CNAME */<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>@@ -2036,7 +2040,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>@@ -2078,8 +2082,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>@@ -2828,7 +2832,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>@@ -2845,14 +2849,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>@@ -2862,7 +2869,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>@@ -3123,10 +3146,7 @@<br> {<br> int x, startplace;<br> <br>- rtp->strict_rtp_state = (strictrtp ? STRICT_RTP_LEARN : STRICT_RTP_OPEN);<br>- if (strictrtp) {<br>- rtp_learning_seq_init(&rtp->rtp_source_learn, (uint16_t)rtp->seqno);<br>- }<br>+ rtp->strict_rtp_state = (strictrtp ? STRICT_RTP_CLOSED : STRICT_RTP_OPEN);<br> <br> /* Create a new socket for us to listen on and use */<br> if ((rtp->s =<br>@@ -3762,7 +3782,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>@@ -3782,7 +3802,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>@@ -3792,7 +3812,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>@@ -4179,6 +4199,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>@@ -4778,93 +4802,293 @@<br> return found;<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_instance *transport = instance;<br> struct ast_rtp *transport_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>+ transport_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>+ transport_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>+ transport_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 = transport_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> struct ast_rtp_instance *child;<br> struct ast_rtp *rtp;<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>+ /* 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>+ transport_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>+ 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>+ transport_rtp, ast_sockaddr_stringify(addr), pt, rtcp_payload_type2str(pt),<br>+ min_length - 1, length - 1);<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>+ /* 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>- return &ast_null_frame;<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\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>+ 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> /* Determine the appropriate instance for this */<br>- child = rtp_find_instance_by_ssrc(transport, transport_rtp, rtcp_report->ssrc);<br>- if (child != transport) {<br>- /* It is safe to hold the child lock while holding the parent lock, we guarantee that the locking order<br>- * is always parent->child or that the child lock is not held when acquiring the parent lock.<br>- */<br>- ao2_lock(child);<br>- instance = child;<br>- rtp = ast_rtp_instance_get_data(instance);<br>+ if (ssrc_valid) {<br>+ child = rtp_find_instance_by_ssrc(transport, transport_rtp, ssrc);<br>+ if (child != transport) {<br>+ /*<br>+ * It is safe to hold the child lock while holding the parent lock.<br>+ * We guarantee that the locking order is always parent->child or<br>+ * that the child lock is not held when acquiring the parent lock.<br>+ */<br>+ ao2_lock(child);<br>+ instance = child;<br>+ rtp = ast_rtp_instance_get_data(instance);<br>+ } else {<br>+ /* The child is the parent! We don't need to unlock it. */<br>+ child = NULL;<br>+ rtp = transport_rtp;<br>+ }<br> } else {<br>- /* The child is the parent! We don't need to unlock it. */<br> child = NULL;<br> rtp = transport_rtp;<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>- continue;<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>+ if (child) {<br>+ ao2_unlock(child);<br>+ }<br>+ continue;<br>+ }<br>+ ssrc_seen = 1;<br> }<br> <br>- if (ast_rtp_instance_get_prop(instance, AST_RTP_PROPERTY_NAT)) {<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>- 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>@@ -4888,7 +5112,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>@@ -4948,9 +5172,9 @@<br> */<br> <br> message_blob = ast_json_pack("{s: s, s: s, s: f}",<br>- "from", ast_sockaddr_stringify(&transport_rtp->rtcp->them),<br>- "to", transport_rtp->rtcp->local_addr_str,<br>- "rtt", rtp->rtcp->rtt);<br>+ "from", ast_sockaddr_stringify(addr),<br>+ "to", transport_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>@@ -4996,21 +5220,19 @@<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(&transport_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(&transport_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(&transport_rtp->rtcp->them));<br> break;<br> }<br>- position += (length + 1);<br>+ position += length;<br> rtp->rtcp->rtcp_info = 1;<br> <br> if (child) {<br>@@ -5019,7 +5241,6 @@<br> }<br> <br> return f;<br>-<br> }<br> <br> /*! \pre instance is locked */<br>@@ -5343,31 +5564,133 @@<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>-<br>- ast_verb(4, "%p -- Probation passed - setting RTP source address to %s\n", rtp, ast_sockaddr_stringify(&addr));<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>@@ -5404,7 +5727,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>@@ -5432,8 +5755,10 @@<br> rtp->rtcp->received_prior = 0;<br> }<br> }<br>-<br>- rtp->rxssrc = ssrc;<br>+ /* Bundled children cannot change/learn their SSRC implicitly. */<br>+ ast_assert(!child || (rtp->themssrc_valid && rtp->themssrc == 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>@@ -5486,10 +5811,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> <br> /* If we are directly bridged to another instance send the audio directly out,<br>@@ -5899,13 +6220,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>@@ -6189,11 +6511,12 @@<br> return rtp->cname;<br> }<br> <br>+/*! \pre instance is locked */<br> static void ast_rtp_set_remote_ssrc(struct ast_rtp_instance *instance, unsigned int ssrc)<br> {<br> struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);<br> <br>- if (rtp->themssrc == ssrc) {<br>+ if (rtp->themssrc_valid && rtp->themssrc == ssrc) {<br> return;<br> }<br> <br>@@ -6201,6 +6524,8 @@<br> if (rtp->bundled) {<br> struct ast_rtp *bundled_rtp;<br> int index;<br>+<br>+ ast_assert(rtp->themssrc_valid);<br> <br> ao2_unlock(instance);<br> <br>@@ -6223,6 +6548,7 @@<br> }<br> <br> rtp->themssrc = ssrc;<br>+ rtp->themssrc_valid = 1;<br> }<br> <br> static void ast_rtp_set_stream_num(struct ast_rtp_instance *instance, int stream_num)<br>@@ -6232,6 +6558,7 @@<br> rtp->stream_num = stream_num;<br> }<br> <br>+/*! \pre child is locked */<br> static int ast_rtp_bundle(struct ast_rtp_instance *child, struct ast_rtp_instance *parent)<br> {<br> struct ast_rtp *child_rtp = ast_rtp_instance_get_data(child);<br>@@ -6274,6 +6601,7 @@<br> /* Children maintain a reference to the parent to guarantee that the transport doesn't go away on them */<br> child_rtp->bundled = ao2_bump(parent);<br> <br>+ ast_assert(child_rtp->themssrc_valid);<br> mapping.ssrc = child_rtp->themssrc;<br> mapping.instance = child;<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6413">change 6413</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/6413"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </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: 6413 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </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>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>