Change in osmo-mgw[master]: network: check packets before further processing

[Harald Welte]

Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/10330 )

Change subject: network: check packets before further processing
......................................................................

network: check packets before further processing

When we receive a packet, we do not really check the contents. However,
we should at least do some basic checks.

- Check for short RTP packets
- Check if the length field of RTCP packets seems plausible
- Check if the packet type of RTCP packets makes sense (IANA)

Change-Id: Id47b9eee2164c542e6b673db24974859dd0a7618
Related: OS#3444
---
M src/libosmo-mgcp/mgcp_network.c
1 file changed, 69 insertions(+), 0 deletions(-)

Approvals:
  Jenkins Builder: Verified
  Harald Welte: Looks good to me, approved



diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c
index 6dfc5a5..427f3c6 100644
--- a/src/libosmo-mgcp/mgcp_network.c
+++ b/src/libosmo-mgcp/mgcp_network.c
@@ -32,6 +32,7 @@
 #include <osmocom/core/msgb.h>
 #include <osmocom/core/select.h>
 #include <osmocom/core/socket.h>
+#include <osmocom/core/byteswap.h>
 #include <osmocom/netif/rtp.h>
 #include <osmocom/mgcp/mgcp.h>
 #include <osmocom/mgcp/mgcp_common.h>
@@ -939,6 +940,53 @@
 	return 0;
 }
 
+/* Do some basic checks to make sure that the RTCP packets we are going to
+ * process are not complete garbage */
+static int check_rtcp(char *buf, unsigned int buf_size)
+{
+	struct rtcp_hdr *hdr;
+	unsigned int len;
+	uint8_t type;
+
+	/* RTPC packets that are just a header without data do not make
+	 * any sense. */
+	if (buf_size < sizeof(struct rtcp_hdr))
+		return -EINVAL;
+
+	/* Make sure that the length of the received packet does not exceed
+	 * the available buffer size */
+	hdr = (struct rtcp_hdr *)buf;
+	len = (osmo_ntohs(hdr->length) + 1) * 4;
+	if (len > buf_size)
+		return -EINVAL;
+
+	/* Make sure we accept only packets that have a proper packet type set
+	 * See also: http://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml */
+	type = hdr->type;
+	if ((type < 192 || type > 195) && (type < 200 || type > 213))
+		return -EINVAL;
+
+	return 0;
+}
+
+/* Do some basic checks to make sure that the RTP packets we are going to
+ * process are not complete garbage */
+static int check_rtp(char *buf, unsigned int buf_size)
+{
+	/* RTP packets that are just a header without data do not make
+	 * any sense. */
+	if (buf_size < sizeof(struct rtp_hdr))
+		return -EINVAL;
+
+	/* FIXME: Add more checks, the reason why we do not check more than
+	 * the length is because we currently handle IUUP packets as RTP
+	 * packets, so they must pass this check, if we weould be more
+	 * strict here, we would possibly break 3G. (see also FIXME note
+	 * below */
+
+	return 0;
+}
+
 /* Receive RTP data from a specified source connection and dispatch it to a
  * destination connection. */
 static int mgcp_recv(int *proto, struct sockaddr_in *addr, char *buf,
@@ -959,8 +1007,29 @@
 	rc = receive_from(endp, fd->fd, addr, buf, buf_size);
 	if (rc <= 0)
 		return -1;
+
+	/* FIXME: The way how we detect the protocol looks odd. We should look
+	 * into the packet header. Also we should introduce a packet type
+	 * MGCP_PROTO_IUUP because currently we handle IUUP packets like RTP
+	 * packets which is problematic. */
 	*proto = fd == &conn->end.rtp ? MGCP_PROTO_RTP : MGCP_PROTO_RTCP;
 
+	if (*proto == MGCP_PROTO_RTP) {
+		if (check_rtp(buf, rc) < 0) {
+			LOGP(DRTP, LOGL_ERROR,
+			     "endpoint:0x%x invalid RTP packet received -- packet tossed\n",
+			     ENDPOINT_NUMBER(endp));
+			return -1;
+		}
+	} else if (*proto == MGCP_PROTO_RTCP) {
+		if (check_rtcp(buf, rc) < 0) {
+			LOGP(DRTP, LOGL_ERROR,
+			     "endpoint:0x%x invalid RTCP packet received -- packet tossed\n",
+			     ENDPOINT_NUMBER(endp));
+			return -1;
+		}
+	}
+
 	LOGP(DRTP, LOGL_DEBUG, "endpoint:0x%x ", ENDPOINT_NUMBER(endp));
 	LOGPC(DRTP, LOGL_DEBUG, "receiving from %s %s %d\n",
 	      conn->conn->name, inet_ntoa(addr->sin_addr),

-- 
To view, visit https://gerrit.osmocom.org/10330
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id47b9eee2164c542e6b673db24974859dd0a7618
Gerrit-Change-Number: 10330
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180805/7800b9b0/attachment.htm>