[Asterisk-code-review] res rtp asterisk.c: Validate RTCP packets before processing ... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/6443 )

Change subject: res_rtp_asterisk.c: Validate RTCP packets before processing them.
......................................................................


Patch Set 4: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4644
PS4, Line 4644: #define RTCP_VALID_MASK (RTCP_VERSION_MASK | (0xfe << 16))
              : #define RTCP_VALID_VALUE (RTCP_VERSION | (RTCP_PT_SR << 16))
I take it that your intent here by using 0xfe is to include both SR(200) and RR(201) as valid but it took me 15 minutes to figure that out.  Comments would be good.


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4688
PS4, Line 4688: first_word & RTCP_VERSION_MASK) == RTCP_VERSION
Is just checking 2 bits enough to determine that it's a valid rtcp packet?  I'd at least add the packet type.  You could use rtcp_payload_type2str as a check.


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4707
PS4, Line 4707: 	/*
              : 	 * Require SSRC to match for strictrtp before we use the
              : 	 * sender's address for symmetrical RTP to send our
              : 	 * RTCP reports.
              : 	 */
              : 	ssrc_seen = rtp->strict_rtp_state == STRICT_RTP_OPEN;
The comment and the statement don't really match.


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4720
PS4, Line 4720: ssrc_valid
This implies it's a "valid" ssrc but an ssrc isn't really valid or invalid.  You might want to rename this (and themssrc_valid) to something like "use_ssrc", etc.


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4729
PS4, Line 4729: 		pt = (first_word & 0xff0000) >> 16;
              : 		rc = (first_word & 0x1f000000) >> 24;
Since you made masks for version and "valid" how about masks for these as well.


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4755
PS4, Line 4755: 			/* We don't know what min_length should be so disable the check */
              : 			min_length = length;
Isn't this also going to catch an unknown packet type?  In that case, shouldn't we skip it?


https://gerrit.asterisk.org/#/c/6443/4/res/res_rtp_asterisk.c@4766
PS4, Line 4766: 		/* Get the RTCP record SSRC if defined for the record */
              : 		ssrc_valid = 1;
              : 		switch (pt) {
              : 		case RTCP_PT_SR:
              : 		case RTCP_PT_RR:
              : 			rtcp_report = ast_rtp_rtcp_report_alloc(rc);
              : 			if (!rtcp_report) {
              : 				return &ast_null_frame;
              : 			}
              : 			rtcp_report->reception_report_count = rc;
              : 
              : 			ssrc = ntohl(rtcpheader[i + 1]);
              : 			rtcp_report->ssrc = ssrc;
              : 			break;
              : 		case RTCP_PT_FUR:
              : 		case RTCP_PT_PSFB:
              : 			ssrc = ntohl(rtcpheader[i + 1]);
              : 			break;
              : 		case RTCP_PT_SDES:
              : 		case RTCP_PT_BYE:
              : 		default:
              : 			ssrc = 0;
              : 			ssrc_valid = 0;
              : 			break;
              : 		}
Why not combine this switch with the earlier one?



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I67d89e3c27db83efa0e6b52734f73c88ac2939e2
Gerrit-Change-Number: 6443
Gerrit-PatchSet: 4
Gerrit-Owner: George Joseph <gjoseph 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-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Wed, 13 Sep 2017 12:41:52 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170913/48fe3275/attachment.html>