This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15242 ) Change subject: fix crashes: don't assert on incoming RTP packet size ...................................................................... fix crashes: don't assert on incoming RTP packet size Remove various OSMO_ASSERT() on size of incoming packets. Doing an assert on incoming data is a DoS attack vector, absolute no-go. Instead, return -EINVAL and keep running. Change some return values to be able to distinguish successful operation from invalid RTP sizes. In rtp_data_net(), make sure to return negative if the RTP packet was invalid. Some of the error return codes implemented here will only be used in upcoming patch Iba115a0b1d74e7cefba5dcdd777e98ddea9eba8c. Change-Id: I6bc6ee950ce07bcc2c585c30fad02b81153bdde2 --- M src/libosmo-mgcp/mgcp_network.c 1 file changed, 32 insertions(+), 10 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c index bb29d2b..e163d23 100644 --- a/src/libosmo-mgcp/mgcp_network.c +++ b/src/libosmo-mgcp/mgcp_network.c @@ -475,7 +475,9 @@ uint8_t pt_in; int pt_out; - OSMO_ASSERT(len >= sizeof(struct rtp_hdr)); + if (len < sizeof(struct rtp_hdr)) + return -EINVAL; + rtp_hdr = (struct rtp_hdr *)data; pt_in = rtp_hdr->payload_type; @@ -640,8 +642,9 @@ /* There are different dialects used to format and transfer voice data. When * the receiving end expects GSM-HR data to be formated after RFC 5993, this * function is used to convert between RFC 5993 and TS 101318, which we normally - * use. */ -static void rfc5993_hr_convert(struct mgcp_endpoint *endp, char *data, int *len) + * use. + * Return 0 on sucess, negative on errors like invalid data length. */ +static int rfc5993_hr_convert(struct mgcp_endpoint *endp, char *data, int *len) { /* NOTE: *data has an overall length of RTP_BUF_SIZE, so there is * plenty of space available to store the slightly larger, converted @@ -649,7 +652,12 @@ struct rtp_hdr *rtp_hdr; - OSMO_ASSERT(*len >= sizeof(struct rtp_hdr)); + if (*len < sizeof(struct rtp_hdr)) { + LOGPENDP(endp, DRTP, LOGL_ERROR, "AMR RTP packet too short (%d < %zu)\n", + *len, sizeof(struct rtp_hdr)); + return -EINVAL; + } + rtp_hdr = (struct rtp_hdr *)data; if (*len == GSM_HR_BYTES + sizeof(struct rtp_hdr)) { @@ -667,7 +675,9 @@ * packet. This is not supported yet. */ LOGPENDP(endp, DRTP, LOGL_ERROR, "cannot figure out how to convert RTP packet\n"); + return -ENOTSUP; } + return 0; } /* For AMR RTP two framing modes are defined RFC3267. There is a bandwith @@ -685,7 +695,11 @@ unsigned int payload_len; int rc; - OSMO_ASSERT(*len >= sizeof(struct rtp_hdr)); + if (*len < sizeof(struct rtp_hdr)) { + LOGPENDP(endp, DRTP, LOGL_ERROR, "AMR RTP packet too short (%d < %zu)\n", *len, sizeof(struct rtp_hdr)); + return -EINVAL; + } + rtp_hdr = (struct rtp_hdr *)data; payload_len = *len - sizeof(struct rtp_hdr); @@ -735,18 +749,23 @@ } -/* Check if a given RTP with AMR payload for octet-aligned mode */ -static bool amr_oa_check(char *data, int len) +/* Return whether an RTP packet with AMR payload is in octet-aligned mode. + * Return 0 if in bandwidth-efficient mode, 1 for octet-aligned mode, and negative if the RTP data is invalid. */ +static int amr_oa_check(char *data, int len) { struct rtp_hdr *rtp_hdr; unsigned int payload_len; - OSMO_ASSERT(len >= sizeof(struct rtp_hdr)); + if (len < sizeof(struct rtp_hdr)) + return -EINVAL; + rtp_hdr = (struct rtp_hdr *)data; payload_len = len - sizeof(struct rtp_hdr); + if (payload_len < sizeof(struct amr_hdr)) + return -EINVAL; - return osmo_amr_is_oa(rtp_hdr->data, payload_len); + return osmo_amr_is_oa(rtp_hdr->data, payload_len) ? 1 : 0; } /* Forward data to a debug tap. This is debug function that is intended for @@ -1340,7 +1359,10 @@ * define, then we check if the incoming payload matches that * expectation. */ if (amr_oa_bwe_convert_indicated(conn_src->end.codec)) { - if (amr_oa_check(buf, len) != conn_src->end.codec->param.amr_octet_aligned) + int oa = amr_oa_check(buf, len); + if (oa < 0) + return -1; + if (((bool)oa) != conn_src->end.codec->param.amr_octet_aligned) return -1; } -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15242 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I6bc6ee950ce07bcc2c585c30fad02b81153bdde2 Gerrit-Change-Number: 15242 Gerrit-PatchSet: 3 Gerrit-Owner: neels <nhofmeyr at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge at gnumonks.org> Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de> Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-MessageType: merged -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190826/c931744e/attachment.htm>