[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: Code-Review-1

(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 learn the PartyB stream when it starts."


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 right?  Then what happens?  What's the user experience in this case?


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 the state for the new source so it can't reach the minimum packets and be locked to, right?

So I'd add "...and no more packets have been received from the original source."


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?



-- 
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-Comment-Date: Fri, 15 Sep 2017 14:41:00 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170915/2ba13f54/attachment.html>