<p>Harald Welte <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/10330">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
Harald Welte: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">network: check packets before further processing<br><br>When we receive a packet, we do not really check the contents. However,<br>we should at least do some basic checks.<br><br>- Check for short RTP packets<br>- Check if the length field of RTCP packets seems plausible<br>- Check if the packet type of RTCP packets makes sense (IANA)<br><br>Change-Id: Id47b9eee2164c542e6b673db24974859dd0a7618<br>Related: OS#3444<br>---<br>M src/libosmo-mgcp/mgcp_network.c<br>1 file changed, 69 insertions(+), 0 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c</span><br><span>index 6dfc5a5..427f3c6 100644</span><br><span>--- a/src/libosmo-mgcp/mgcp_network.c</span><br><span>+++ b/src/libosmo-mgcp/mgcp_network.c</span><br><span>@@ -32,6 +32,7 @@</span><br><span> #include <osmocom/core/msgb.h></span><br><span> #include <osmocom/core/select.h></span><br><span> #include <osmocom/core/socket.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/byteswap.h></span><br><span> #include <osmocom/netif/rtp.h></span><br><span> #include <osmocom/mgcp/mgcp.h></span><br><span> #include <osmocom/mgcp/mgcp_common.h></span><br><span>@@ -939,6 +940,53 @@</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/* Do some basic checks to make sure that the RTCP packets we are going to</span><br><span style="color: hsl(120, 100%, 40%);">+ * process are not complete garbage */</span><br><span style="color: hsl(120, 100%, 40%);">+static int check_rtcp(char *buf, unsigned int buf_size)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct rtcp_hdr *hdr;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int len;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t type;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* RTPC packets that are just a header without data do not make</span><br><span style="color: hsl(120, 100%, 40%);">+ * any sense. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_size < sizeof(struct rtcp_hdr))</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make sure that the length of the received packet does not exceed</span><br><span style="color: hsl(120, 100%, 40%);">+ * the available buffer size */</span><br><span style="color: hsl(120, 100%, 40%);">+ hdr = (struct rtcp_hdr *)buf;</span><br><span style="color: hsl(120, 100%, 40%);">+ len = (osmo_ntohs(hdr->length) + 1) * 4;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (len > buf_size)</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make sure we accept only packets that have a proper packet type set</span><br><span style="color: hsl(120, 100%, 40%);">+ * See also: http://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml */</span><br><span style="color: hsl(120, 100%, 40%);">+ type = hdr->type;</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((type < 192 || type > 195) && (type < 200 || type > 213))</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/* Do some basic checks to make sure that the RTP packets we are going to</span><br><span style="color: hsl(120, 100%, 40%);">+ * process are not complete garbage */</span><br><span style="color: hsl(120, 100%, 40%);">+static int check_rtp(char *buf, unsigned int buf_size)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ /* RTP packets that are just a header without data do not make</span><br><span style="color: hsl(120, 100%, 40%);">+ * any sense. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (buf_size < sizeof(struct rtp_hdr))</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* FIXME: Add more checks, the reason why we do not check more than</span><br><span style="color: hsl(120, 100%, 40%);">+ * the length is because we currently handle IUUP packets as RTP</span><br><span style="color: hsl(120, 100%, 40%);">+ * packets, so they must pass this check, if we weould be more</span><br><span style="color: hsl(120, 100%, 40%);">+ * strict here, we would possibly break 3G. (see also FIXME note</span><br><span style="color: hsl(120, 100%, 40%);">+ * below */</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /* Receive RTP data from a specified source connection and dispatch it to a</span><br><span> * destination connection. */</span><br><span> static int mgcp_recv(int *proto, struct sockaddr_in *addr, char *buf,</span><br><span>@@ -959,8 +1007,29 @@</span><br><span> rc = receive_from(endp, fd->fd, addr, buf, buf_size);</span><br><span> if (rc <= 0)</span><br><span> return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* FIXME: The way how we detect the protocol looks odd. We should look</span><br><span style="color: hsl(120, 100%, 40%);">+ * into the packet header. Also we should introduce a packet type</span><br><span style="color: hsl(120, 100%, 40%);">+ * MGCP_PROTO_IUUP because currently we handle IUUP packets like RTP</span><br><span style="color: hsl(120, 100%, 40%);">+ * packets which is problematic. */</span><br><span> *proto = fd == &conn->end.rtp ? MGCP_PROTO_RTP : MGCP_PROTO_RTCP;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (*proto == MGCP_PROTO_RTP) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (check_rtp(buf, rc) < 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DRTP, LOGL_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ "endpoint:0x%x invalid RTP packet received -- packet tossed\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ ENDPOINT_NUMBER(endp));</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (*proto == MGCP_PROTO_RTCP) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (check_rtcp(buf, rc) < 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DRTP, LOGL_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ "endpoint:0x%x invalid RTCP packet received -- packet tossed\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ ENDPOINT_NUMBER(endp));</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> LOGP(DRTP, LOGL_DEBUG, "endpoint:0x%x ", ENDPOINT_NUMBER(endp));</span><br><span> LOGPC(DRTP, LOGL_DEBUG, "receiving from %s %s %d\n",</span><br><span> conn->conn->name, inet_ntoa(addr->sin_addr),</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/10330">change 10330</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/10330"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: Id47b9eee2164c542e6b673db24974859dd0a7618 </div>
<div style="display:none"> Gerrit-Change-Number: 10330 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>