[Asterisk-code-review] res rtp asterisk.c: Make strict RTP learning more flexible. (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6413 )

Change subject: res_rtp_asterisk.c: Make strict RTP learning more flexible.
......................................................................


Patch Set 2:

(4 comments)

https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5374
PS2, Line 5374: Ast1 may see an initial RTP stream from
              : 		 * Ast2 and then a stream from PartyB.  Since Ast1 may not have
              : 		 * seen or learned the RTP stream from Ast2 before being told to learn
              : 		 * the stream from PartyB, Ast1 may lock onto the Ast2 stream and
              : 		 * never learn the PartyB stream.
> I'd just replace this with "Ast1 may lock onto the Ast2 stream and never le
Done


https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5382
PS2, Line 5382: 		 * To mitigate against attack, the learning state cannot switch
              : 		 * streams while there are competing streams.  The competing streams
              : 		 * interfere with each other's qualification.  Once we accept a
              : 		 * stream and reach the timeout, an attacker cannot interfere
              : 		 * anymore.
> So as long as there are competing streams, we'll lock onto none of them rig
There are a few scenarios and each one assumes that the streams are continuous:
1) We already have a known stream source and the known stream wants to change to a new source.  An attacking stream will block learning the new stream source.  After the timeout we re-lock onto the original stream source which likely went away.  The result is one way audio.

2) We already have a known stream source and the known stream doesn't want to change source addresses.  An attacking stream will not be able to replace the known stream.  After the timeout we re-lock onto the known stream.  The call is not affected.

3) We don't have a known stream.  This presumably is the start of a call.  Competing streams will result in staying in learning mode until a stream becomes the victor.  The result is one way audio until there is a victor.  We cannot timeout because we have no known stream to lock onto.

If we learn a stream before the timeout we will be in scenario 1) or 2) when a competing stream starts.


Were you wanting me to add this information to the comments?


https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5400
PS2, Line 5400: learning for the new source so it only takes over once
              : 				 * sufficient traffic has been received.
> As long as we receive packets from the original source, we keep resetting t
Done


https://gerrit.asterisk.org/#/c/6413/2/res/res_rtp_asterisk.c@5407
PS2, Line 5407: 			/*
              : 			 * We give preferential treatment to the requested target address
              : 			 * (negotiated SDP address) where we are to send our RTP.  However,
              : 			 * the other end has no obligation to send from that address even
              : 			 * though it is practically a requirement when NAT is involved.
              : 			 */
              : 			ast_rtp_instance_get_requested_target_address(instance, &target_address);
              : 			if (!ast_sockaddr_cmp(&target_address, &addr)) {
              : 				/* Accept the negotiated target RTP stream as the source */
              : 				ast_verb(4, "%p -- Strict RTP switching to RTP target address %s as source\n",
              : 					rtp, ast_sockaddr_stringify(&addr));
              : 				ast_sockaddr_copy(&rtp->strict_rtp_address, &addr);
              : 				rtp_learning_seq_init(&rtp->rtp_source_learn, seqno);
              : 				break;
              : 			}
> How preferential?   Maybe this should be before the old/new source check?
It is preferential for becoming the new source when it is not the current source.

We cannot move this check before the current source check.  If it is not the current source it becomes the current source.  If it is the current source then the check is redundant and we will spam the switching source message.



-- 
To view, visit https://gerrit.asterisk.org/6413
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2d3aa6e0f22906c25971e74f10027d96525f31c
Gerrit-Change-Number: 6413
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 15 Sep 2017 17:50:04 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170915/8ed2d799/attachment.html>