Change in osmo-msc[master]: libmsc/gsm_04_08.c: refactor CM Service Request parsing

[Harald Welte]

Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/13907 )

Change subject: libmsc/gsm_04_08.c: refactor CM Service Request parsing
......................................................................

libmsc/gsm_04_08.c: refactor CM Service Request parsing

In gsm48_rx_mm_serv_req() we need to make sure that a given message
buffer is large enough to contain both 'gsm48_hdr' and
'gsm48_service_request' structures.

Comparing msg->data_len with size of pointer if wrong because:

  - we actually need to compare with size of struct(s),
  - we need msgb_l3len(), not length of the whole buffer.

Moreover, since we have to use the pointer arithmetics in order
to keep backwards compatibility with Phase1 phones, we also
need to check the length of both Classmark2 and MI IEs.

Change-Id: I6e7454d7a6f63fd5a0e12fb90d8c58688da0951e
---
M src/libmsc/gsm_04_08.c
1 file changed, 38 insertions(+), 19 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index 280c341..418ee2a 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -700,28 +700,47 @@
 int gsm48_rx_mm_serv_req(struct msc_a *msc_a, struct msgb *msg)
 {
 	struct gsm_network *net = msc_a_net(msc_a);
-	uint8_t mi_type;
-	struct gsm48_hdr *gh = msgb_l3(msg);
-	struct gsm48_service_request *req =
-			(struct gsm48_service_request *)gh->data;
-	/* unfortunately in Phase1 the classmark2 length is variable */
-	uint8_t classmark2_len = gh->data[1];
-	uint8_t *classmark2_buf = gh->data+2;
-	struct gsm48_classmark2 *cm2 = (void*)classmark2_buf;
-	uint8_t *mi_p = classmark2_buf + classmark2_len;
-	uint8_t mi_len = *mi_p;
-	uint8_t *mi = mi_p + 1;
+	struct gsm48_hdr *gh;
+	struct gsm48_service_request *req;
+	struct gsm48_classmark2 *cm2;
+	uint8_t *cm2_buf, cm2_len;
+	uint8_t *mi_buf, mi_len;
+	uint8_t *mi, mi_type;
 	bool is_utran;
 	struct vlr_subscr *vsub;
 
-	if (msg->data_len < sizeof(struct gsm48_service_request*)) {
+	/* Make sure that both header and CM Service Request fit into the buffer */
+	if (msgb_l3len(msg) < sizeof(*gh) + sizeof(*req)) {
 		LOG_MSC_A(msc_a, LOGL_ERROR, "Rx CM SERVICE REQUEST: wrong message size (%u < %zu)\n",
-			  msg->data_len, sizeof(struct gsm48_service_request*));
+			  msgb_l3len(msg), sizeof(*gh) + sizeof(*req));
 		return msc_gsm48_tx_mm_serv_rej(msc_a, GSM48_REJECT_INCORRECT_MESSAGE);
 	}
 
-	if (msg->data_len < req->mi_len + 6) {
-		LOG_MSC_A(msc_a, LOGL_ERROR, "Rx CM SERVICE REQUEST: message does not fit in packet\n");
+	gh = (struct gsm48_hdr *) msgb_l3(msg);
+	req = (struct gsm48_service_request *) gh->data;
+
+	/* Unfortunately in Phase1 the Classmark2 length is variable, so we cannot
+	 * just use gsm48_service_request struct, and need to parse it manually. */
+	cm2_len = gh->data[1];
+	cm2_buf = gh->data + 2;
+	cm2 = (struct gsm48_classmark2 *) cm2_buf;
+
+	/* Prevent buffer overrun: check the length of Classmark2 */
+	if (cm2_buf + cm2_len > msg->tail) {
+		LOG_MSC_A(msc_a, LOGL_ERROR, "Rx CM SERVICE REQUEST: Classmark2 "
+					     "length=%u is too big\n", cm2_len);
+		return msc_gsm48_tx_mm_serv_rej(msc_a, GSM48_REJECT_INCORRECT_MESSAGE);
+	}
+
+	/* MI (Mobile Identity) LV follows the Classmark2 */
+	mi_buf = cm2_buf + cm2_len;
+	mi_len = mi_buf[0];
+	mi = mi_buf + 1;
+
+	/* Prevent buffer overrun: check the length of MI */
+	if (mi_buf + mi_len > msg->tail) {
+		LOG_MSC_A(msc_a, LOGL_ERROR, "Rx CM SERVICE REQUEST: Mobile Identity "
+					     "length=%u is too big\n", cm2_len);
 		return msc_gsm48_tx_mm_serv_rej(msc_a, GSM48_REJECT_INCORRECT_MESSAGE);
 	}
 
@@ -759,9 +778,9 @@
 		return msc_gsm48_tx_mm_serv_rej(msc_a, GSM48_REJECT_SRV_OPT_NOT_SUPPORTED);
 
 	if (msc_a_is_accepted(msc_a))
-		return cm_serv_reuse_conn(msc_a, mi_p, req->cm_service_type);
+		return cm_serv_reuse_conn(msc_a, mi_buf, req->cm_service_type);
 
-	osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_IDENTITY, mi_p);
+	osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_IDENTITY, mi_buf);
 
 	msc_a_get(msc_a, msc_a_cm_service_type_to_use(req->cm_service_type));
 
@@ -775,7 +794,7 @@
 			 is_utran || net->authentication_required,
 			 is_utran || net->a5_encryption_mask > 0x01,
 			 req->cipher_key_seq,
-			 osmo_gsm48_classmark2_is_r99(cm2, classmark2_len),
+			 osmo_gsm48_classmark2_is_r99(cm2, cm2_len),
 			 is_utran);
 
 	/* From vlr_proc_acc_req() we expect an implicit dispatch of PR_ARQ_E_START we expect
@@ -787,7 +806,7 @@
 	}
 
 	vsub->classmark.classmark2 = *cm2;
-	vsub->classmark.classmark2_len = classmark2_len;
+	vsub->classmark.classmark2_len = cm2_len;
 	return 0;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/13907
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6e7454d7a6f63fd5a0e12fb90d8c58688da0951e
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 4
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190512/f3b81dfa/attachment.htm>