Change in ...osmo-mgw[master]: fix crashes: don't assert on incoming RTP packet size

laforge gerrit-no-reply at lists.osmocom.org
Mon Aug 26 18:53:07 UTC 2019


laforge 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/1d1195a7/attachment.html>


More information about the gerrit-log mailing list