fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/43191?usp=email ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: gsm0480: fix out-of-bounds read in parse_process_uss_req() ...................................................................... gsm0480: fix out-of-bounds read in parse_process_uss_req() The USSD-String octet count (uss_req_data[6]) was only checked against the 160-byte GSM0480_USSD_OCTET_STRING_LEN destination limit, never against the number of bytes actually remaining in the received message. A truncated ProcessUnstructuredSS-Request (as short as 8 bytes) with a length byte up to 160 caused the subsequent memcpy() (and, for the default alphabet, gsm_7bit_decode_n_ussd()) to read up to ~159 bytes past the end of the message buffer. uss_req_data/length come straight from a received L3 message via the public gsm0480_decode_ss_request() API, so this is reachable with network-supplied input. Add the missing length check, mirroring the one already present in the sibling parse_process_uss_data(), and add a regression test with an oversized length byte. Change-Id: I62a2ea539238d8c2dbab897ce2f80b9ab905a108 Fixes: OS#7045 --- M src/gsm/gsm0480.c M tests/ussd/ussd_test.c M tests/ussd/ussd_test.ok 3 files changed, 34 insertions(+), 0 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/gsm/gsm0480.c b/src/gsm/gsm0480.c index 7a7f71f..a8eac6b 100644 --- a/src/gsm/gsm0480.c +++ b/src/gsm/gsm0480.c @@ -704,6 +704,8 @@ dcs = uss_req_data[4]; /* Get the amount of bytes */ num_chars = uss_req_data[6]; + if (num_chars > length - 7) + return 0; /* Drop messages with incorrect length */ if (num_chars > GSM0480_USSD_OCTET_STRING_LEN) { diff --git a/tests/ussd/ussd_test.c b/tests/ussd/ussd_test.c index 2b8321d..b131a89 100644 --- a/tests/ussd/ussd_test.c +++ b/tests/ussd/ussd_test.c @@ -41,6 +41,16 @@ 0x05, 0x02, 0x01, 0x24 }; +/* Same REGISTER/ProcessUssReq message as ussd_request[], except the + * USSD-String octet count (index 18) claims 100 bytes while the message + * only ever carries 6. Must be rejected, not read past the buffer end. */ +static const uint8_t ussd_process_uss_req_overflow[] = { + 0x0b, 0x7b, 0x1c, 0x15, 0xa1, 0x13, 0x02, 0x01, + 0x03, 0x02, 0x01, 0x3b, 0x30, 0x0b, 0x04, 0x01, + 0x0f, 0x04, 0x64, 0x2a, 0xd5, 0x4c, 0x16, 0x1b, + 0x01, 0x7f, 0x01, 0x00 +}; + static const uint8_t interrogate_ss[] = { 0x0b, 0x7b, 0x1c, 0x0d, 0xa1, 0x0b, 0x02, 0x01, 0x03, 0x02, 0x01, 0x0e, 0x30, 0x03, 0x04, 0x01, @@ -221,6 +231,23 @@ printf("\n"); } +/* parse_process_uss_req() must reject a USSD-String octet count that + * exceeds the bytes actually remaining in the message, rather than only + * capping it against GSM0480_USSD_OCTET_STRING_LEN and reading past the + * end of the buffer. */ +static void test_process_uss_req_overflow(void) +{ + int rc; + + printf("[i] Testing parse_process_uss_req() against an oversized " + "USSD-String length\n"); + + rc = parse_ussd(ussd_process_uss_req_overflow, sizeof(ussd_process_uss_req_overflow)); + OSMO_ASSERT(rc == 0); + + printf("\n"); +} + int main(int argc, char **argv) { struct ss_request req; @@ -237,6 +264,9 @@ /* Test gsm0480_parse_facility_ie() */ test_parse_facility_ie(); + /* Test parse_process_uss_req() against an oversized length byte */ + test_process_uss_req_overflow(); + memset(&req, 0, sizeof(req)); gsm0480_decode_ss_request((struct gsm48_hdr *) ussd_request, sizeof(ussd_request), &req); diff --git a/tests/ussd/ussd_test.ok b/tests/ussd/ussd_test.ok index 1137080..6e23da3 100644 --- a/tests/ussd/ussd_test.ok +++ b/tests/ussd/ussd_test.ok @@ -12,6 +12,8 @@ [?] Data length: expected 0x01, decoded 0x01 [?] Data: expected 32, decoded 32 +[i] Testing parse_process_uss_req() against an oversized USSD-String length + Tested if it still works. Text was: **321# interrogateSS CFU text..'' code 33 Testing parsing a USSD request and truncated versions -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/43191?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I62a2ea539238d8c2dbab897ce2f80b9ab905a108 Gerrit-Change-Number: 43191 Gerrit-PatchSet: 2 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: pespin <pespin@sysmocom.de>