Currently the inner loop in show_bsc_mgcp iterates of the timeslot interval [0, 31]. Timeslot 0 is not valid, which causes mgcp_timeslot_to_endpoint to generate a corresponding warning and to return an invalid endp value. That value causes an out-of-bound read access, possibly hitting unallocated memory.
This patch fixes the loop range by starting with timeslot 1.
Note that this does not prevent mgcp_timeslot_to_endpoint from returning an invalid endpoint index when called with arguments not within its domain.
Addresses: <000b> ../../include/openbsc/mgcp.h:250 Timeslot should not be 0 [...] vty=0xb4203db0, argc=1, argv=0xbfffebb0) at bsc_nat_vty.c:256 max = 1 con = 0xb4a004f0 i = 0 j = 0 [...] ==15700== ERROR: AddressSanitizer: heap-use-after-free on address 0xb520be4f at pc 0x8062a42 bp 0xbfffeb18 sp 0xbfffeb0c
Sponsored-by: On-Waves ehf --- openbsc/src/osmo-bsc_nat/bsc_nat_vty.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c index 5f4ad28..2b7db2e 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c @@ -250,7 +250,7 @@ DEFUN(show_bsc_mgcp, show_bsc_mgcp_cmd, "show bsc mgcp NR", vty_out(vty, "MGCP Status for %d%s", con->cfg->nr, VTY_NEWLINE); max = bsc_mgcp_nr_multiplexes(con->max_endpoints); for (i = 0; i < max; ++i) { - for (j = 0; j < 32; ++j) { + for (j = 1; j < 32; ++j) { endp = mgcp_timeslot_to_endpoint(i, j); vty_out(vty, " Endpoint 0x%x %s%s", endp, con->_endpoint_status[endp] == 0
When handling an incoming GSUP cancellation request, the cancel_type if effectively ignored, such that is always handled as GPRS_GSUP_CANCEL_TYPE_UPDATE and never as WITHDRAW.
This commit fixes the expression used to set the variable is_update_procedure.
Fixes: Coverity CID 1267739 Sponsored-by: On-Waves ehf --- openbsc/src/gprs/gprs_subscriber.c | 3 ++- openbsc/tests/sgsn/sgsn_test.c | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/gprs/gprs_subscriber.c b/openbsc/src/gprs/gprs_subscriber.c index 8486834..e3da0f8 100644 --- a/openbsc/src/gprs/gprs_subscriber.c +++ b/openbsc/src/gprs/gprs_subscriber.c @@ -492,7 +492,8 @@ static int gprs_subscr_handle_loc_cancel_req(struct gsm_subscriber *subscr, struct gprs_gsup_message *gsup_msg) { struct gprs_gsup_message gsup_reply = {0}; - int is_update_procedure = !gsup_msg->cancel_type || gsup_msg->cancel_type; + int is_update_procedure = !gsup_msg->cancel_type || + gsup_msg->cancel_type == GPRS_GSUP_CANCEL_TYPE_UPDATE;
LOGGSUBSCRP(LOGL_INFO, subscr, "Cancelling MS subscriber (%s)\n", is_update_procedure ? diff --git a/openbsc/tests/sgsn/sgsn_test.c b/openbsc/tests/sgsn/sgsn_test.c index 6fc4f99..197be9d 100644 --- a/openbsc/tests/sgsn/sgsn_test.c +++ b/openbsc/tests/sgsn/sgsn_test.c @@ -440,6 +440,12 @@ static void test_subscriber_gsup(void) 0x06, 0x01, 0x00, };
+ static const uint8_t location_cancellation_req_withdraw[] = { + 0x1c, + TEST_GSUP_IMSI1_IE, + 0x06, 0x01, 0x01, + }; + static const uint8_t location_cancellation_req_other[] = { 0x1c, 0x01, 0x05, 0x11, 0x11, 0x11, 0x11, 0x01, @@ -582,6 +588,12 @@ static void test_subscriber_gsup(void) OSMO_ASSERT(s1->flags & GPRS_SUBSCRIBER_CANCELLED); OSMO_ASSERT(s1->sgsn_data->mm == NULL);
+ /* Inject LocCancelReq(withdraw) GSUP message */ + rc = rx_gsup_message(location_cancellation_req_withdraw, + sizeof(location_cancellation_req_withdraw)); + OSMO_ASSERT(rc >= 0); + OSMO_ASSERT(s1->sgsn_data->error_cause == GMM_CAUSE_IMPL_DETACHED); + /* Inject PurgeMsRes GSUP message */ rc = rx_gsup_message(purge_ms_res, sizeof(purge_ms_res));
Currently some VTY command do neither check the length of the source string before calling strncpy nor ensure NUL-termination afterwards. This can to destination string buffers whose contents are not NUL-teminated.
This commit adds checks and corresponding warnings to the VTY commands 'subscriber TYPE ID name .NAME" and "subscriber TYPE ID extension EXTENSION".
Fixes: Coverity CID 1206570, 1206569 Sponsored-by: On-Waves ehf --- openbsc/src/libmsc/vty_interface_layer3.c | 14 ++++++++++++++ openbsc/tests/vty_test_runner.py | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/openbsc/src/libmsc/vty_interface_layer3.c b/openbsc/src/libmsc/vty_interface_layer3.c index 68d9c44..558db5e 100644 --- a/openbsc/src/libmsc/vty_interface_layer3.c +++ b/openbsc/src/libmsc/vty_interface_layer3.c @@ -548,6 +548,13 @@ DEFUN(ena_subscr_name, return CMD_WARNING; }
+ if (strlen(name) > sizeof(subscr->name)-1) { + vty_out(vty, + "%% NAME is too long, max. %d characters are allowed%s", + sizeof(subscr->name)-1, VTY_NEWLINE); + return CMD_WARNING; + } + strncpy(subscr->name, name, sizeof(subscr->name)); talloc_free(name); db_sync_subscriber(subscr); @@ -574,6 +581,13 @@ DEFUN(ena_subscr_extension, return CMD_WARNING; }
+ if (strlen(ext) > sizeof(subscr->extension)-1) { + vty_out(vty, + "%% EXTENSION is too long, max. %d characters are allowed%s", + sizeof(subscr->extension)-1, VTY_NEWLINE); + return CMD_WARNING; + } + strncpy(subscr->extension, ext, sizeof(subscr->extension)); db_sync_subscriber(subscr);
diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 2b7fd19..fb4ca7d 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -286,6 +286,32 @@ class TestVTYNITB(TestVTYGenericBSC): res = self.vty.command('show subscriber imsi '+imsi) self.assert_(res != '% No subscriber found for imsi '+imsi)
+ def testSubscriberSettings(self): + self.vty.enable() + + imsi = "204300854013739" + wrong_imsi = "204300999999999" + + # Lets create one + res = self.vty.command('subscriber create imsi '+imsi) + self.assert_(res.find(" IMSI: "+imsi) > 0) + + self.vty.verify('subscriber imsi '+wrong_imsi+' name wrong', ['% No subscriber found for imsi '+wrong_imsi]) + res = self.vty.command('subscriber imsi '+imsi+' name '+('X' * 160)) + self.assert_(res.find("NAME is too long") > 0) + + self.vty.verify('subscriber imsi '+imsi+' name '+('G' * 159), ['']) + + self.vty.verify('subscriber imsi '+wrong_imsi+' extension 840', ['% No subscriber found for imsi '+wrong_imsi]) + res = self.vty.command('subscriber imsi '+imsi+' extension '+('9' * 15)) + self.assert_(res.find("EXTENSION is too long") > 0) + + self.vty.verify('subscriber imsi '+imsi+' extension '+('1' * 14), ['']) + + # Delete it + res = self.vty.command('subscriber delete imsi '+imsi) + self.assert_(res != "") + def testShowPagingGroup(self): res = self.vty.command("show paging-group 255 1234567") self.assertEqual(res, "% can't find BTS 255")
Currently the handling of the buffers is not done consistently. Some code assumes that the whole buffer may be used to store the string while at other places, the last buffer byte is left untouched in the assumption that it contains a terminating NUL-character. The latter is the correct behaviour.
This commit changes to code to not touch the last byte in the buffers and to rely on the last byte being NUL. So the maximum IMSI/IMEI length is GSM_IMSI_LENGTH-1/GSM_IMEI_LENGTH-1.
Fixes: Coverity CID 1206568, 1206567 Sponsored-by: On-Waves ehf --- openbsc/src/libcommon/gsm_subscriber_base.c | 3 +-- openbsc/src/libmsc/db.c | 2 +- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/openbsc/src/libcommon/gsm_subscriber_base.c b/openbsc/src/libcommon/gsm_subscriber_base.c index 3c56101..a455824 100644 --- a/openbsc/src/libcommon/gsm_subscriber_base.c +++ b/openbsc/src/libcommon/gsm_subscriber_base.c @@ -112,8 +112,7 @@ struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp, if (!subscr) return NULL;
- strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH); - subscr->imsi[GSM_IMSI_LENGTH - 1] = '\0'; + strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH-1); subscr->group = sgrp; return subscr; } diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index bdecbb4..ee678b6 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -802,7 +802,7 @@ static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result) const char *string; string = dbi_result_get_string(result, "imsi"); if (string) - strncpy(subscr->imsi, string, GSM_IMSI_LENGTH); + strncpy(subscr->imsi, string, GSM_IMSI_LENGTH-1);
string = dbi_result_get_string(result, "tmsi"); if (string) diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index ac5a9f5..67844b8 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -399,7 +399,7 @@ int bsc_ussd_check(struct nat_sccp_connection *con, struct bsc_nat_parsed *parse if (parsed->bssap != BSSAP_MSG_DTAP) return 0;
- if (strlen(con->imsi) > GSM_IMSI_LENGTH) + if (strlen(con->imsi) >= GSM_IMSI_LENGTH) return 0;
hdr48 = bsc_unpack_dtap(parsed, msg, &len);
On Tue, Apr 07, 2015 at 05:49:50PM +0200, Jacob Erlbeck wrote:
This commit changes to code to not touch the last byte in the buffers and to rely on the last byte being NUL. So the maximum IMSI/IMEI length is GSM_IMSI_LENGTH-1/GSM_IMEI_LENGTH-1.
For information: "rely" here means.. we assume that we allocate the structure with talloc_zero. This means we have NULed the entire imsi array and then only write sizeof - 1 characters to it. So the last byte remains NUL
Currently the handling of the buffers is not done consistently. Some code assumes that the whole buffer may be used to store the string while at other places, the last buffer byte is left untouched in the assumption that it contains a terminating NUL-character. The latter is the correct behaviour.
This commit changes to code to not touch the last byte in the buffers and to rely on the last byte being NUL. So the maximum IMSI/IMEI length is GSM_IMSI_LENGTH-1/GSM_IMEI_LENGTH-1.
For information: We assume that we allocate the structure with talloc_zero. This means we have NULed the entire imsi array and then only write sizeof - 1 characters to it. So the last byte remains NUL.
Fixes: Coverity CID 1206568, 1206567 Sponsored-by: On-Waves ehf --- openbsc/src/libcommon/gsm_subscriber_base.c | 3 +-- openbsc/src/libmsc/db.c | 4 ++-- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/openbsc/src/libcommon/gsm_subscriber_base.c b/openbsc/src/libcommon/gsm_subscriber_base.c index 3c56101..a455824 100644 --- a/openbsc/src/libcommon/gsm_subscriber_base.c +++ b/openbsc/src/libcommon/gsm_subscriber_base.c @@ -112,8 +112,7 @@ struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp, if (!subscr) return NULL;
- strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH); - subscr->imsi[GSM_IMSI_LENGTH - 1] = '\0'; + strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH-1); subscr->group = sgrp; return subscr; } diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index bdecbb4..428f99b 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -565,7 +565,7 @@ static int get_equipment_by_subscr(struct gsm_subscriber *subscr)
string = dbi_result_get_string(result, "imei"); if (string) - strncpy(equip->imei, string, sizeof(equip->imei)); + strncpy(equip->imei, string, sizeof(equip->imei)-1);
string = dbi_result_get_string(result, "classmark1"); if (string) { @@ -802,7 +802,7 @@ static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result) const char *string; string = dbi_result_get_string(result, "imsi"); if (string) - strncpy(subscr->imsi, string, GSM_IMSI_LENGTH); + strncpy(subscr->imsi, string, GSM_IMSI_LENGTH-1);
string = dbi_result_get_string(result, "tmsi"); if (string) diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index ac5a9f5..67844b8 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -399,7 +399,7 @@ int bsc_ussd_check(struct nat_sccp_connection *con, struct bsc_nat_parsed *parse if (parsed->bssap != BSSAP_MSG_DTAP) return 0;
- if (strlen(con->imsi) > GSM_IMSI_LENGTH) + if (strlen(con->imsi) >= GSM_IMSI_LENGTH) return 0;
hdr48 = bsc_unpack_dtap(parsed, msg, &len);
-
Holger Hans Peter Freyther -
Jacob Erlbeck