Change in osmo-sgsn[master]: gb_proxy: Introduce more validation / constraint checks

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.org
Tue Dec 8 21:59:54 UTC 2020 

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/21560 )

Change subject: gb_proxy: Introduce more validation / constraint checks
......................................................................

gb_proxy: Introduce more validation / constraint checks

* ensure the BSSGP PDU header length before reading pdu_type field
* ensure we never process uplink PDUs in downlink and vice-versa
* ensure we never proceses PTP PDUs on SIGNALING BVCI and vice-versa

Change-Id: I6e40aed0283f1a0860ab273606605f7fb28717cf
Depends: libosmocore.git I7e4226463f3c935134b5c2c737696fbfd1dd5815
---
M src/gbproxy/gb_proxy.c
1 file changed, 63 insertions(+), 23 deletions(-)

Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, approved



diff --git a/src/gbproxy/gb_proxy.c b/src/gbproxy/gb_proxy.c
index 98fa928..a30f5ad 100644
--- a/src/gbproxy/gb_proxy.c
+++ b/src/gbproxy/gb_proxy.c
@@ -249,8 +249,26 @@
 				  struct msgb *msg, uint16_t nsei,
 				  uint16_t ns_bvci)
 {
+	struct bssgp_normal_hdr *bgph = (struct bssgp_normal_hdr *) msgb_bssgph(msg);
 	struct gbproxy_bvc *bvc;
 
+	if (ns_bvci == 0 && ns_bvci == 1) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/BSS) BVCI=%05u is not PTP\n", nsei, ns_bvci);
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(bgph->pdu_type) & BSSGP_PDUF_PTP)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/%05u) %s not allowed in PTP BVC\n",
+		     nsei, ns_bvci, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, bgph->pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(bgph->pdu_type) & BSSGP_PDUF_UL)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/%05u) %s not allowed in uplink direction\n",
+		     nsei, ns_bvci, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, bgph->pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
 	bvc = gbproxy_bvc_by_bvci(cfg, ns_bvci);
 	if (!bvc) {
 		LOGP(DGPRS, LOGL_NOTICE, "BVC(%05u/??) Didn't find bvc "
@@ -272,12 +290,27 @@
 				   struct msgb *msg, uint16_t nsei,
 				   uint16_t ns_bvci)
 {
+	struct bssgp_normal_hdr *bgph = (struct bssgp_normal_hdr *) msgb_bssgph(msg);
 	struct gbproxy_bvc *bvc;
 
+	if (ns_bvci == 0 && ns_bvci == 1) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/BSS) BVCI=%05u is not PTP\n", nsei, ns_bvci);
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(bgph->pdu_type) & BSSGP_PDUF_PTP)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/%05u) %s not allowed in PTP BVC\n",
+		     nsei, ns_bvci, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, bgph->pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(bgph->pdu_type) & BSSGP_PDUF_DL)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/%05u) %s not allowed in downlink direction\n",
+		     nsei, ns_bvci, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, bgph->pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
 	bvc = gbproxy_bvc_by_bvci(cfg, ns_bvci);
-
-	/* Send status messages before patching */
-
 	if (!bvc) {
 		LOGP(DGPRS, LOGL_INFO, "BVC(%05u/??) Didn't find bvc for "
 		     "for message from NSE(%05u/SGSN)\n",
@@ -393,18 +426,20 @@
 	int rc;
 
 	if (ns_bvci != 0 && ns_bvci != 1) {
-		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u) BVCI=%05u is not signalling\n",
-			nsei, ns_bvci);
-		return -EINVAL;
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/BSS) BVCI=%05u is not signalling\n", nsei, ns_bvci);
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
 	}
 
-	/* we actually should never see those two for BVCI == 0, but double-check
-	 * just to make sure  */
-	if (pdu_type == BSSGP_PDUT_UL_UNITDATA ||
-	    pdu_type == BSSGP_PDUT_DL_UNITDATA) {
-		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u) UNITDATA not allowed in "
-			"signalling\n", nsei);
-		return -EINVAL;
+	if (!(bssgp_pdu_type_flags(pdu_type) & BSSGP_PDUF_SIG)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/BSS) %s not allowed in signalling BVC\n",
+		     nsei, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(pdu_type) & BSSGP_PDUF_UL)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/BSS) %s not allowed in uplink direction\n",
+		     nsei, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, msg);
 	}
 
 	bssgp_tlv_parse(&tp, bgph->data, data_len);
@@ -596,18 +631,19 @@
 	int i;
 
 	if (ns_bvci != 0 && ns_bvci != 1) {
-		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) BVCI=%05u is not "
-			"signalling\n", nsei, ns_bvci);
-		/* FIXME: Send proper error message */
-		return -EINVAL;
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) BVCI=%05u is not signalling\n", nsei, ns_bvci);
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, orig_msg);
 	}
 
-	/* we actually should never see those two for BVCI == 0, but double-check
-	 * just to make sure  */
-	if (pdu_type == BSSGP_PDUT_UL_UNITDATA ||
-	    pdu_type == BSSGP_PDUT_DL_UNITDATA) {
-		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) UNITDATA not allowed in "
-			"signalling\n", nsei);
+	if (!(bssgp_pdu_type_flags(pdu_type) & BSSGP_PDUF_SIG)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) %s not allowed in signalling BVC\n",
+		     nsei, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, pdu_type));
+		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, orig_msg);
+	}
+
+	if (!(bssgp_pdu_type_flags(pdu_type) & BSSGP_PDUF_DL)) {
+		LOGP(DGPRS, LOGL_NOTICE, "NSE(%05u/SGSN) %s not allowed in downlink direction\n",
+		     nsei, osmo_tlv_prot_msg_name(&osmo_pdef_bssgp, pdu_type));
 		return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, orig_msg);
 	}
 
@@ -758,6 +794,10 @@
 
 	int remote_end_is_sgsn = gbproxy_is_sgsn_nsei(cfg, nsei);
 
+	/* ensure minimum length to decode PCU type */
+	if (msgb_bssgp_len(msg) < sizeof(struct bssgp_normal_hdr))
+		return bssgp_tx_status(BSSGP_CAUSE_SEM_INCORR_PDU, NULL, msg);
+
 	/* Only BVCI=0 messages need special treatment */
 	if (ns_bvci == 0 || ns_bvci == 1) {
 		if (remote_end_is_sgsn)

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/21560
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: I6e40aed0283f1a0860ab273606605f7fb28717cf
Gerrit-Change-Number: 21560
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201208/ea0ba95f/attachment.htm>

More information about the gerrit-log mailing list