pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-msc/+/43253?usp=email )
Change subject: smpp: Fix potential write buffer overflow on sms->user_data ......................................................................
smpp: Fix potential write buffer overflow on sms->user_data
The writes to sms->user_data in submit_to_sms() were not being validated against the maximum size of the buffer, which could lead into writing past the buffer limits. In order to assure safe encdoing of septets into the buffer, the new libosmocore gsm_septet_pack2() is required.
Related: OS#7059 Reported-By: Adam Bedard adam.bedard@gmail.com Change-Id: Iee701a3b033d78244fc10dd8563a094b8de385c0 --- M TODO-RELEASE M src/libsmpputil/smpp_msc.c 2 files changed, 25 insertions(+), 5 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/53/43253/1
diff --git a/TODO-RELEASE b/TODO-RELEASE index c8858d8..84a2985 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -9,3 +9,4 @@ #library what description / commit summary line libosmovty >=1.12.1 working optional-multi-choice libosmocore >1.12.0 log_get_context(), log_{get,set}_filter(_data)() +libosmogsm >1.14.1 gsm_septet_pack2() diff --git a/src/libsmpputil/smpp_msc.c b/src/libsmpputil/smpp_msc.c index d68dcc0..5fdba36 100644 --- a/src/libsmpputil/smpp_msc.c +++ b/src/libsmpputil/smpp_msc.c @@ -132,6 +132,7 @@ struct tlv_t *t; int mode; int can_store_sms = ((submit->esm_class & SMPP34_MSG_MODE_MASK) != 2); /* != forward mode */ + int rc;
dest = subscr_by_dst(net, submit->dest_addr_npi, submit->dest_addr_ton, @@ -247,21 +248,39 @@ ud_len = *sms_msg + 1; if (ud_len > sms_msg_len) { sms_free(sms); - LOGP(DLSMS, LOGL_ERROR, "invalid ud_len=%u > sms_msg_len=%u\n", ud_len, - sms_msg_len); + LOGP(DLSMS, LOGL_ERROR, "invalid ud_len=%u > sms_msg_len=%u\n", + ud_len, sms_msg_len); + return ESME_RINVPARLEN; + } + if (ud_len > sizeof(sms->user_data)) { + sms_free(sms); + LOGP(DLSMS, LOGL_ERROR, "invalid sms_msg_len=%u > %zu\n", + sms_msg_len, sizeof(sms->user_data)); return ESME_RINVPARLEN; } printf("copying %u bytes user data...\n", ud_len); - memcpy(sms->user_data, sms_msg, - OSMO_MIN(ud_len, sizeof(sms->user_data))); + memcpy(sms->user_data, sms_msg, ud_len); sms_msg += ud_len; sms_msg_len -= ud_len; padbits = 7 - (ud_len % 7); } - gsm_septet_pack(sms->user_data+ud_len, sms_msg, sms_msg_len, padbits); + rc = gsm_septet_pack2(sms->user_data + ud_len, sizeof(sms->user_data) - ud_len, + sms_msg, sms_msg_len, padbits); + if (rc < 0) { + sms_free(sms); + LOGP(DLSMS, LOGL_ERROR, "invalid ud_len=%u + sms_msg_len=%u > %zu\n", + ud_len, sms_msg_len, sizeof(sms->user_data)); + return ESME_RINVPARLEN; + } sms->user_data_len = (ud_len*8 + padbits)/7 + sms_msg_len;/* SEPTETS */ /* FIXME: sms->text */ } else { + if (sms_msg_len > sizeof(sms->user_data)) { + sms_free(sms); + LOGP(DLSMS, LOGL_ERROR, "invalid sms_msg_len=%u > %zu\n", + sms_msg_len, sizeof(sms->user_data)); + return ESME_RINVPARLEN; + } memcpy(sms->user_data, sms_msg, sms_msg_len); sms->user_data_len = sms_msg_len; }