This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/21492 ) Change subject: gb_proxy: Use TLVP_PRES_LEN instead of TLVP_PRESENT ...................................................................... gb_proxy: Use TLVP_PRES_LEN instead of TLVP_PRESENT With TLVP_PRESENT we only check if a tiven TLV/IE is present, but don't verify that it's length matches our expectation. This can lead to out-of-bounds reads, so let's always use TLVP_PRES_LEN. Change-Id: I1519cff0f6b2fe77f9a91eee17e0055d9df1bce6 --- M src/gbproxy/gb_proxy.c M src/gbproxy/gb_proxy_peer.c 2 files changed, 17 insertions(+), 17 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/92/21492/1 diff --git a/src/gbproxy/gb_proxy.c b/src/gbproxy/gb_proxy.c index 109a539..3776634 100644 --- a/src/gbproxy/gb_proxy.c +++ b/src/gbproxy/gb_proxy.c @@ -1017,7 +1017,7 @@ uint16_t bvci; uint8_t cause; - if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) || !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) { + if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) || !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) { rate_ctr_inc(&cfg->ctrg->ctr[GBPROX_GLOB_CTR_PROTO_ERR_BSS]); return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg); } @@ -1093,7 +1093,7 @@ gbproxy_peer_move(from_peer, nse_new); } - if (TLVP_PRESENT(tp, BSSGP_IE_CELL_ID)) { + if (TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8)) { struct gprs_ra_id raid; /* We have a Cell Identifier present in this * PDU, this means we can extend our local @@ -1149,7 +1149,7 @@ * area identification. The snooped information is then used * for routing the {SUSPEND,RESUME}_[N]ACK back to the correct * BSSGP */ - if (!TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA)) + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6)) goto err_mand_ie; from_peer = gbproxy_peer_by_nsei(cfg, nsei); if (!from_peer) @@ -1205,7 +1205,7 @@ LOGP(DGPRS, LOGL_INFO, "NSE(%05u/SGSN) BSSGP PAGING\n", nsei); - if (TLVP_PRESENT(tp, BSSGP_IE_BVCI)) { + if (TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) { uint16_t bvci = ntohs(tlvp_val16_unal(tp, BSSGP_IE_BVCI)); errctr = GBPROX_GLOB_CTR_OTHER_ERR; peer = gbproxy_peer_by_bvci(cfg, bvci); @@ -1217,7 +1217,7 @@ } LOGPBVC(peer, LOGL_INFO, "routing by BVCI\n"); return gbprox_relay2peer(msg, peer, ns_bvci); - } else if (TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) { + } else if (TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) { errctr = GBPROX_GLOB_CTR_INV_RAI; /* iterate over all peers and dispatch the paging to each matching one */ llist_for_each_entry(nse, &cfg->nse_peers, list) { @@ -1231,7 +1231,7 @@ } } } - } else if (TLVP_PRESENT(tp, BSSGP_IE_LOCATION_AREA)) { + } else if (TLVP_PRES_LEN(tp, BSSGP_IE_LOCATION_AREA, 5)) { errctr = GBPROX_GLOB_CTR_INV_LAI; /* iterate over all peers and dispatch the paging to each matching one */ llist_for_each_entry(nse, &cfg->nse_peers, list) { @@ -1245,7 +1245,7 @@ } } } - } else if (TLVP_PRESENT(tp, BSSGP_IE_BSS_AREA_ID)) { + } else if (TLVP_PRES_LEN(tp, BSSGP_IE_BSS_AREA_ID, 1)) { /* iterate over all peers and dispatch the paging to each matching one */ llist_for_each_entry(nse, &cfg->nse_peers, list) { llist_for_each_entry(peer, &nse->bts_peers, list) { @@ -1281,7 +1281,7 @@ struct gbproxy_peer *peer; uint16_t ptp_bvci; - if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI)) { + if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) { rate_ctr_inc(&cfg->ctrg-> ctr[GBPROX_GLOB_CTR_PROTO_ERR_SGSN]); return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, @@ -1364,7 +1364,7 @@ if (cfg->route_to_sgsn2 && nsei == cfg->nsip_sgsn2_nsei) break; /* simple case: BVCI IE is mandatory */ - if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) goto err_mand_ie; bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI)); if (bvci == BVCI_SIGNALLING) { @@ -1375,7 +1375,7 @@ break; case BSSGP_PDUT_FLUSH_LL: /* simple case: BVCI IE is mandatory */ - if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) goto err_mand_ie; bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI)); rc = gbprox_relay2bvci(cfg, msg, bvci, ns_bvci); @@ -1389,7 +1389,7 @@ /* Some exception has occurred */ LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) BSSGP STATUS ", nsei); - if (!TLVP_PRESENT(&tp, BSSGP_IE_CAUSE)) { + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_CAUSE, 1)) { LOGPC(DGPRS, LOGL_NOTICE, "\n"); goto err_mand_ie; } @@ -1397,7 +1397,7 @@ LOGPC(DGPRS, LOGL_NOTICE, "cause=0x%02x(%s) ", *TLVP_VAL(&tp, BSSGP_IE_CAUSE), bssgp_cause_str(cause)); - if (TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) { + if (TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) { bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI)); LOGPC(DGPRS, LOGL_NOTICE, "BVCI=%05u\n", bvci); @@ -1412,7 +1412,7 @@ case BSSGP_PDUT_RESUME_ACK: case BSSGP_PDUT_RESUME_NACK: /* RAI IE is mandatory */ - if (!TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA)) + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6)) goto err_mand_ie; peer = gbproxy_peer_by_rai(cfg, TLVP_VAL(&tp, BSSGP_IE_ROUTEING_AREA)); if (!peer) @@ -1421,7 +1421,7 @@ break; case BSSGP_PDUT_BVC_BLOCK_ACK: case BSSGP_PDUT_BVC_UNBLOCK_ACK: - if (!TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) + if (!TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) goto err_mand_ie; bvci = ntohs(tlvp_val16_unal(&tp, BSSGP_IE_BVCI)); if (bvci == 0) { diff --git a/src/gbproxy/gb_proxy_peer.c b/src/gbproxy/gb_proxy_peer.c index e63e259..8493447 100644 --- a/src/gbproxy/gb_proxy_peer.c +++ b/src/gbproxy/gb_proxy_peer.c @@ -164,7 +164,7 @@ struct gbproxy_peer *gbproxy_peer_by_bssgp_tlv(struct gbproxy_config *cfg, struct tlv_parsed *tp) { - if (TLVP_PRESENT(tp, BSSGP_IE_BVCI)) { + if (TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) { uint16_t bvci; bvci = ntohs(tlvp_val16_unal(tp, BSSGP_IE_BVCI)); @@ -173,7 +173,7 @@ } /* FIXME: this doesn't make sense, as RA can span multiple peers! */ - if (TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) { + if (TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) { uint8_t *rai = (uint8_t *)TLVP_VAL(tp, BSSGP_IE_ROUTEING_AREA); /* Only compare LAC part, since MCC/MNC are possibly patched. * Since the LAC of different BSS must be different when @@ -182,7 +182,7 @@ } /* FIXME: this doesn't make sense, as LA can span multiple peers! */ - if (TLVP_PRESENT(tp, BSSGP_IE_LOCATION_AREA)) { + if (TLVP_PRES_LEN(tp, BSSGP_IE_LOCATION_AREA, 5)) { uint8_t *lai = (uint8_t *)TLVP_VAL(tp, BSSGP_IE_LOCATION_AREA); return gbproxy_peer_by_lac(cfg, lai); } -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/21492 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I1519cff0f6b2fe77f9a91eee17e0055d9df1bce6 Gerrit-Change-Number: 21492 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge at osmocom.org> Gerrit-MessageType: newchange -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201203/7732f283/attachment.htm>