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

[George Joseph]

George Joseph 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:

(2 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@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.
> 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?

Yeah, I think what you described would be the perfect comment.


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;
              : 			}
> 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.

Gotcha.



-- 
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:55:06 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170915/0ecb93c0/attachment-0001.html>