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 <nhofm...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Vadim Yanitskiy <axilira...@gmail.com>
