[MERGED] osmo-msc[master]: vlr_ciph_result: fix use after free of imeisv

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: vlr_ciph_result: fix use after free of imeisv
......................................................................


vlr_ciph_result: fix use after free of imeisv

Define the struct vlr_ciph_result member .imeisv not as a char* but a char[] of
appropriate length, to avoid the need to point to external memory.

Thus fix a use-after-free in msc_cipher_mode_compl(), which defined the
imeisv[] buffer in a sub-scope within that function, so that the .imeisv
pointer was already invalid when fed to vlr_subscr_rx_ciph_res().

Did you notice that the commit summary rhymes?

Closes: OS#3053
Change-Id: I90cfb952a7dec6d104200872164ebadb25d0260d
---
M include/osmocom/msc/vlr.h
M src/libmsc/osmo_msc.c
M src/libvlr/vlr_access_req_fsm.c
M src/libvlr/vlr_lu_fsm.c
4 files changed, 4 insertions(+), 6 deletions(-)

Approvals:
  Vadim Yanitskiy: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index c4b8cf6..37702a9 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -74,7 +74,7 @@
 
 struct vlr_ciph_result {
 	enum vlr_ciph_result_cause cause;
-	const char *imeisv;
+	char imeisv[GSM48_MI_SIZE];
 };
 
 enum vlr_subscr_security_context {
diff --git a/src/libmsc/osmo_msc.c b/src/libmsc/osmo_msc.c
index f6df0d2..323baf9 100644
--- a/src/libmsc/osmo_msc.c
+++ b/src/libmsc/osmo_msc.c
@@ -173,7 +173,6 @@
 		unsigned int payload_len = msgb_l3len(msg) - sizeof(*gh);
 		struct tlv_parsed tp;
 		uint8_t mi_type;
-		char imeisv[GSM48_MI_SIZE] = "";
 
 		if (!gh) {
 			LOGP(DRR, LOGL_ERROR, "invalid: msgb without l3 header\n");
@@ -187,10 +186,9 @@
 			mi_type = TLVP_VAL(&tp, GSM48_IE_MOBILE_ID)[0] & GSM_MI_TYPE_MASK;
 			if (mi_type == GSM_MI_TYPE_IMEISV
 			    && TLVP_LEN(&tp, GSM48_IE_MOBILE_ID) > 0) {
-				gsm48_mi_to_string(imeisv, sizeof(imeisv),
+				gsm48_mi_to_string(ciph_res.imeisv, sizeof(ciph_res.imeisv),
 						   TLVP_VAL(&tp, GSM48_IE_MOBILE_ID),
 						   TLVP_LEN(&tp, GSM48_IE_MOBILE_ID));
-				ciph_res.imeisv = imeisv;
 			}
 		}
 	}
diff --git a/src/libvlr/vlr_access_req_fsm.c b/src/libvlr/vlr_access_req_fsm.c
index 95a618d..3845f26 100644
--- a/src/libvlr/vlr_access_req_fsm.c
+++ b/src/libvlr/vlr_access_req_fsm.c
@@ -500,7 +500,7 @@
 	}
 
 
-	if (res.imeisv) {
+	if (*res.imeisv) {
 		LOGPFSM(fi, "got IMEISV: %s\n", res.imeisv);
 		vlr_subscr_set_imeisv(vsub, res.imeisv);
 	}
diff --git a/src/libvlr/vlr_lu_fsm.c b/src/libvlr/vlr_lu_fsm.c
index c6fd080..9a4a239 100644
--- a/src/libvlr/vlr_lu_fsm.c
+++ b/src/libvlr/vlr_lu_fsm.c
@@ -1165,7 +1165,7 @@
 		return;
 	}
 
-	if (res.imeisv) {
+	if (*res.imeisv) {
 		LOGPFSM(fi, "got IMEISV: %s\n", res.imeisv);
 		vlr_subscr_set_imeisv(vsub, res.imeisv);
 	}

-- 
To view, visit https://gerrit.osmocom.org/7264
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I90cfb952a7dec6d104200872164ebadb25d0260d
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>