fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/43191?usp=email )
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(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/91/43191/1
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