<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21494">View Change</a></p><div style="white-space:pre-wrap">Approvals:
daniel: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">gprs_ns2: Use TLVP_PRES_LEN instead of TLVP_PRESENT<br><br>With TLVP_PRESENT we only check if a given TLV/IE is present,<br>but don't verify that it's length matches our expectation. This can<br>lead to out-of-bounds reads, so let's always use TLVP_PRES_LEN.<br><br>Change-Id: I4c438bc82ea6a48243db568f96a234adf784dc0b<br>---<br>M src/gb/gprs_ns2.c<br>M src/gb/gprs_ns2_message.c<br>2 files changed, 14 insertions(+), 13 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gb/gprs_ns2.c b/src/gb/gprs_ns2.c</span><br><span>index 93807f0..d90ba85 100644</span><br><span>--- a/src/gb/gprs_ns2.c</span><br><span>+++ b/src/gb/gprs_ns2.c</span><br><span>@@ -590,7 +590,7 @@</span><br><span> if (!msg)</span><br><span> return -ENOMEM;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, NS_IE_NSEI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, NS_IE_NSEI, 2)) {</span><br><span> nsei = tlvp_val16be(tp, NS_IE_NSEI);</span><br><span> </span><br><span> LOGP(DLNS, LOGL_NOTICE, "NSEI=%u Rejecting message without NSVCI. Tx NS STATUS (cause=%s)\n",</span><br><span>@@ -602,7 +602,7 @@</span><br><span> nsh->pdu_type = NS_PDUT_STATUS;</span><br><span> </span><br><span> msgb_tvlv_put(msg, NS_IE_CAUSE, 1, &_cause);</span><br><span style="color: hsl(0, 100%, 40%);">- have_vci = TLVP_PRESENT(tp, NS_IE_VCI);</span><br><span style="color: hsl(120, 100%, 40%);">+ have_vci = TLVP_PRES_LEN(tp, NS_IE_VCI, 2);</span><br><span> </span><br><span> /* Section 9.2.7.1: Static conditions for NS-VCI */</span><br><span> if (cause == NS_CAUSE_NSVC_BLOCKED ||</span><br><span>@@ -822,8 +822,8 @@</span><br><span> return GPRS_NS2_CS_REJECTED;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, NS_IE_CAUSE) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(&tp, NS_IE_VCI) || !TLVP_PRESENT(&tp, NS_IE_NSEI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, NS_IE_CAUSE, 1) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(&tp, NS_IE_VCI, 2) || !TLVP_PRES_LEN(&tp, NS_IE_NSEI, 2)) {</span><br><span> LOGP(DLNS, LOGL_ERROR, "NS RESET Missing mandatory IE\n");</span><br><span> rc = reject_status_msg(msg, &tp, reject, NS_CAUSE_MISSING_ESSENT_IE);</span><br><span> return GPRS_NS2_CS_REJECTED;</span><br><span>diff --git a/src/gb/gprs_ns2_message.c b/src/gb/gprs_ns2_message.c</span><br><span>index 69c833e..eb9a198 100644</span><br><span>--- a/src/gb/gprs_ns2_message.c</span><br><span>+++ b/src/gb/gprs_ns2_message.c</span><br><span>@@ -66,7 +66,8 @@</span><br><span> </span><br><span> static int gprs_ns2_validate_reset(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_CAUSE) || !TLVP_PRESENT(tp, NS_IE_VCI) || !TLVP_PRESENT(tp, NS_IE_NSEI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_CAUSE, 1) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, NS_IE_VCI, 2) || !TLVP_PRES_LEN(tp, NS_IE_NSEI, 2)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -76,7 +77,7 @@</span><br><span> </span><br><span> static int gprs_ns2_validate_reset_ack(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_VCI) || !TLVP_PRESENT(tp, NS_IE_NSEI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_VCI, 2) || !TLVP_PRES_LEN(tp, NS_IE_NSEI, 2)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -86,7 +87,7 @@</span><br><span> </span><br><span> static int gprs_ns2_validate_block(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_VCI) || !TLVP_PRESENT(tp, NS_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_VCI, 2) || !TLVP_PRES_LEN(tp, NS_IE_CAUSE, 1)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -96,7 +97,7 @@</span><br><span> </span><br><span> static int gprs_ns2_validate_block_ack(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_VCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_VCI, 2)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -107,7 +108,7 @@</span><br><span> static int gprs_ns2_validate_status(struct gprs_ns2_vc *nsvc, struct msgb *msg, struct tlv_parsed *tp, uint8_t *cause)</span><br><span> {</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_CAUSE, 1)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -117,7 +118,7 @@</span><br><span> switch (_cause) {</span><br><span> case NS_CAUSE_NSVC_BLOCKED:</span><br><span> case NS_CAUSE_NSVC_UNKNOWN:</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_CAUSE, 1)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -127,19 +128,19 @@</span><br><span> case NS_CAUSE_PROTO_ERR_UNSPEC:</span><br><span> case NS_CAUSE_INVAL_ESSENT_IE:</span><br><span> case NS_CAUSE_MISSING_ESSENT_IE:</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_CAUSE, 1)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span> break;</span><br><span> case NS_CAUSE_BVCI_UNKNOWN:</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, NS_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, NS_IE_BVCI, 2)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span> break;</span><br><span> case NS_CAUSE_UNKN_IP_TEST_FAILED:</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT (tp, NS_IE_IPv4_LIST) && !TLVP_PRESENT(tp, NS_IE_IPv6_LIST)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRESENT(tp, NS_IE_IPv4_LIST) && !TLVP_PRESENT(tp, NS_IE_IPv6_LIST)) {</span><br><span> *cause = NS_CAUSE_MISSING_ESSENT_IE;</span><br><span> return -1;</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21494">change 21494</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/c/libosmocore/+/21494"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4c438bc82ea6a48243db568f96a234adf784dc0b </div>
<div style="display:none"> Gerrit-Change-Number: 21494 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>