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

[Richard Mudgett]

Richard Mudgett 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:

(5 comments)

I'll see about improving how the code is expressed.  There is only so much that can be done if you haven't seen the RFC.

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) an
This is already an improvement over how the RFC expressed it.


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? 
This is exactly what the RFC check tests in addition to accumulating the length fields.  No other bits can be validated after the first RTCP packet header in a combination packet.


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.
It implies that we actually have an SSRC verses not having one.  Thus the ssrc variable value we have is valid.


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, shoul
Really?  Doing this does disable the length check for unknown packet types.


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?
Because it cannot be combined with the previous switch statement.  Or do you think I should duplicate the length check code and message?



-- 
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 14:50:29 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170913/eea9d614/attachment.html>