<p>laforge has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/osmo-sgsn/+/21492">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">gb_proxy: Use TLVP_PRES_LEN instead of TLVP_PRESENT<br><br>With TLVP_PRESENT we only check if a tiven 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: I1519cff0f6b2fe77f9a91eee17e0055d9df1bce6<br>---<br>M src/gbproxy/gb_proxy.c<br>M src/gbproxy/gb_proxy_peer.c<br>2 files changed, 17 insertions(+), 17 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/92/21492/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gbproxy/gb_proxy.c b/src/gbproxy/gb_proxy.c</span><br><span>index 109a539..3776634 100644</span><br><span>--- a/src/gbproxy/gb_proxy.c</span><br><span>+++ b/src/gbproxy/gb_proxy.c</span><br><span>@@ -1017,7 +1017,7 @@</span><br><span> uint16_t bvci;</span><br><span> uint8_t cause;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) || !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) || !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {</span><br><span> rate_ctr_inc(&cfg->ctrg->ctr[GBPROX_GLOB_CTR_PROTO_ERR_BSS]);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);</span><br><span> }</span><br><span>@@ -1093,7 +1093,7 @@</span><br><span> gbproxy_peer_move(from_peer, nse_new);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_CELL_ID)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8)) {</span><br><span> struct gprs_ra_id raid;</span><br><span> /* We have a Cell Identifier present in this</span><br><span> * PDU, this means we can extend our local</span><br><span>@@ -1149,7 +1149,7 @@</span><br><span> * area identification. The snooped information is then used</span><br><span> * for routing the {SUSPEND,RESUME}_[N]ACK back to the correct</span><br><span> * BSSGP */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6))</span><br><span> goto err_mand_ie;</span><br><span> from_peer = gbproxy_peer_by_nsei(cfg, nsei);</span><br><span> if (!from_peer)</span><br><span>@@ -1205,7 +1205,7 @@</span><br><span> </span><br><span> LOGP(DGPRS, LOGL_INFO, "NSE(%05u/SGSN) BSSGP PAGING\n",</span><br><span> nsei);</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) {</span><br><span> uint16_t bvci = ntohs(tlvp_val16_unal(tp, BSSGP_IE_BVCI));</span><br><span> errctr = GBPROX_GLOB_CTR_OTHER_ERR;</span><br><span> peer = gbproxy_peer_by_bvci(cfg, bvci);</span><br><span>@@ -1217,7 +1217,7 @@</span><br><span> }</span><br><span> LOGPBVC(peer, LOGL_INFO, "routing by BVCI\n");</span><br><span> return gbprox_relay2peer(msg, peer, ns_bvci);</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) {</span><br><span> errctr = GBPROX_GLOB_CTR_INV_RAI;</span><br><span> /* iterate over all peers and dispatch the paging to each matching one */</span><br><span> llist_for_each_entry(nse, &cfg->nse_peers, list) {</span><br><span>@@ -1231,7 +1231,7 @@</span><br><span> }</span><br><span> }</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(tp, BSSGP_IE_LOCATION_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(tp, BSSGP_IE_LOCATION_AREA, 5)) {</span><br><span> errctr = GBPROX_GLOB_CTR_INV_LAI;</span><br><span> /* iterate over all peers and dispatch the paging to each matching one */</span><br><span> llist_for_each_entry(nse, &cfg->nse_peers, list) {</span><br><span>@@ -1245,7 +1245,7 @@</span><br><span> }</span><br><span> }</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(tp, BSSGP_IE_BSS_AREA_ID)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(tp, BSSGP_IE_BSS_AREA_ID, 1)) {</span><br><span> /* iterate over all peers and dispatch the paging to each matching one */</span><br><span> llist_for_each_entry(nse, &cfg->nse_peers, list) {</span><br><span> llist_for_each_entry(peer, &nse->bts_peers, list) {</span><br><span>@@ -1281,7 +1281,7 @@</span><br><span> struct gbproxy_peer *peer;</span><br><span> uint16_t ptp_bvci;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) {</span><br><span> rate_ctr_inc(&cfg->ctrg-></span><br><span> ctr[GBPROX_GLOB_CTR_PROTO_ERR_SGSN]);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE,</span><br><span>@@ -1364,7 +1364,7 @@</span><br><span> if (cfg->route_to_sgsn2 && nsei == cfg->nsip_sgsn2_nsei)</span><br><span> break;</span><br><span> /* simple case: BVCI IE is mandatory */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2))</span><br><span> goto err_mand_ie;</span><br><span> bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI));</span><br><span> if (bvci == BVCI_SIGNALLING) {</span><br><span>@@ -1375,7 +1375,7 @@</span><br><span> break;</span><br><span> case BSSGP_PDUT_FLUSH_LL:</span><br><span> /* simple case: BVCI IE is mandatory */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2))</span><br><span> goto err_mand_ie;</span><br><span> bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI));</span><br><span> rc = gbprox_relay2bvci(cfg, msg, bvci, ns_bvci);</span><br><span>@@ -1389,7 +1389,7 @@</span><br><span> /* Some exception has occurred */</span><br><span> LOGP(DGPRS, LOGL_NOTICE,</span><br><span> "NSE(%05u/SGSN) BSSGP STATUS ", nsei);</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_CAUSE, 1)) {</span><br><span> LOGPC(DGPRS, LOGL_NOTICE, "\n");</span><br><span> goto err_mand_ie;</span><br><span> }</span><br><span>@@ -1397,7 +1397,7 @@</span><br><span> LOGPC(DGPRS, LOGL_NOTICE,</span><br><span> "cause=0x%02x(%s) ", *TLVP_VAL(&tp, BSSGP_IE_CAUSE),</span><br><span> bssgp_cause_str(cause));</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) {</span><br><span> bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI));</span><br><span> LOGPC(DGPRS, LOGL_NOTICE, "BVCI=%05u\n", bvci);</span><br><span> </span><br><span>@@ -1412,7 +1412,7 @@</span><br><span> case BSSGP_PDUT_RESUME_ACK:</span><br><span> case BSSGP_PDUT_RESUME_NACK:</span><br><span> /* RAI IE is mandatory */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6))</span><br><span> goto err_mand_ie;</span><br><span> peer = gbproxy_peer_by_rai(cfg, TLVP_VAL(&tp, BSSGP_IE_ROUTEING_AREA));</span><br><span> if (!peer)</span><br><span>@@ -1421,7 +1421,7 @@</span><br><span> break;</span><br><span> case BSSGP_PDUT_BVC_BLOCK_ACK:</span><br><span> case BSSGP_PDUT_BVC_UNBLOCK_ACK:</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2))</span><br><span> goto err_mand_ie;</span><br><span> bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI));</span><br><span> if (bvci == 0) {</span><br><span>diff --git a/src/gbproxy/gb_proxy_peer.c b/src/gbproxy/gb_proxy_peer.c</span><br><span>index e63e259..8493447 100644</span><br><span>--- a/src/gbproxy/gb_proxy_peer.c</span><br><span>+++ b/src/gbproxy/gb_proxy_peer.c</span><br><span>@@ -164,7 +164,7 @@</span><br><span> struct gbproxy_peer *gbproxy_peer_by_bssgp_tlv(struct gbproxy_config *cfg,</span><br><span> struct tlv_parsed *tp)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) {</span><br><span> uint16_t bvci;</span><br><span> </span><br><span> bvci = ntohs(tlvp_val16_unal(tp, BSSGP_IE_BVCI));</span><br><span>@@ -173,7 +173,7 @@</span><br><span> }</span><br><span> </span><br><span> /* FIXME: this doesn't make sense, as RA can span multiple peers! */</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) {</span><br><span> uint8_t *rai = (uint8_t *)TLVP_VAL(tp, BSSGP_IE_ROUTEING_AREA);</span><br><span> /* Only compare LAC part, since MCC/MNC are possibly patched.</span><br><span> * Since the LAC of different BSS must be different when</span><br><span>@@ -182,7 +182,7 @@</span><br><span> }</span><br><span> </span><br><span> /* FIXME: this doesn't make sense, as LA can span multiple peers! */</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_LOCATION_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(tp, BSSGP_IE_LOCATION_AREA, 5)) {</span><br><span> uint8_t *lai = (uint8_t *)TLVP_VAL(tp, BSSGP_IE_LOCATION_AREA);</span><br><span> return gbproxy_peer_by_lac(cfg, lai);</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-sgsn/+/21492">change 21492</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/osmo-sgsn/+/21492"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-sgsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I1519cff0f6b2fe77f9a91eee17e0055d9df1bce6 </div>
<div style="display:none"> Gerrit-Change-Number: 21492 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>