fixeria has submitted this change. (
https://gerrit.osmocom.org/c/libosmocore/+/43190?usp=email )
Change subject: gsm29205: fix out-of-bounds read in osmo_dec_gcr()
......................................................................
gsm29205: fix out-of-bounds read in osmo_dec_gcr()
The length check at the top of the function only verified that the
input buffer was at least 13 bytes, which is the minimum needed for
.net_len == 3. For .net_len == 4 or 5 (also valid per the length
check further down), the actual minimum required length is
10 + net_len, i.e. 14 or 15 bytes. With a shorter buffer, the
subsequent osmo_load16be(), elem[] access and memcpy() read
past the end of the caller-supplied buffer.
elem/len are taken directly from a received BSSMAP Global Call
Reference IE (gsm0808_utils.c), so this is reachable with
network-supplied input.
Add a length check depending on the actual .net_len, and a test.
Change-Id: I6855d6c810c2b4274ccfd7bc861405f9b4e09343
Fixes: OS#7044
---
M src/gsm/gsm29205.c
M tests/gsm29205/gsm29205_test.c
M tests/gsm29205/gsm29205_test.ok
3 files changed, 35 insertions(+), 0 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, approved
laforge: Looks good to me, but someone else must approve
diff --git a/src/gsm/gsm29205.c b/src/gsm/gsm29205.c
index 8fed020..8fe91f7 100644
--- a/src/gsm/gsm29205.c
+++ b/src/gsm/gsm29205.c
@@ -73,6 +73,8 @@
gcr->net_len = elem[0];
if (gcr->net_len < 3 || gcr->net_len > 5)
return -EINVAL;
+ if (len < 10 + gcr->net_len)
+ return -EBADMSG;
memcpy(gcr->net, elem + parsed, gcr->net_len);
/* +1 for ignored Node ID length field */
diff --git a/tests/gsm29205/gsm29205_test.c b/tests/gsm29205/gsm29205_test.c
index 6598f89..f020aee 100644
--- a/tests/gsm29205/gsm29205_test.c
+++ b/tests/gsm29205/gsm29205_test.c
@@ -95,6 +95,34 @@
msgb_free(msg);
}
+/* osmo_dec_gcr() must reject buffers that are too short for the announced
+ * .net_len (3..5), not just shorter than the 13-byte minimum for
+ * .net_len == 3. Otherwise it reads past the end of 'elem'. */
+static void test_gcr_dec_short_buf(void)
+{
+ static const uint8_t res[] = {
+ 0x05, /* .net_len */
+ 0x51, 0x52, 0x53, 0x54, 0x55, /* .net */
+ 0x02, /* .node length */
+ 0xde, 0xad, /* .node */
+ 0x05, /* length of Call. Ref. */
+ 0x41, 0x42, 0x43, 0x44, 0x45 /* .cr - Call. Ref. */
+ };
+ struct osmo_gcr_parsed p;
+ uint8_t len;
+ int rc;
+
+ printf("Testing Global Call Reference decoder against short
buffers...\n");
+
+ /* net_len == 5 requires 15 bytes, feed it 13 and 14
+ * the full buffer must still decode successfully */
+ for (len = 13; len <= ARRAY_SIZE(res); len++) {
+ rc = osmo_dec_gcr(&p, res, len);
+ printf("\tosmo_dec_gcr(len=%u) -> %s\n",
+ len, rc == len ? "OK" : "FAIL");
+ }
+}
+
int main(int argc, char **argv)
{
osmo_init_logging2(talloc_named_const(NULL, 0, "gsm29205 test"), NULL);
@@ -102,6 +130,7 @@
printf("Testing 3GPP TS 29.205 routines...\n");
test_gcr();
+ test_gcr_dec_short_buf();
printf("Done.\n");
diff --git a/tests/gsm29205/gsm29205_test.ok b/tests/gsm29205/gsm29205_test.ok
index bddd88a..cc9b7d2 100644
--- a/tests/gsm29205/gsm29205_test.ok
+++ b/tests/gsm29205/gsm29205_test.ok
@@ -2,4 +2,8 @@
Testing Global Call Reference encoder...
13 bytes added: OK
decoded 13 bytes: OK
+Testing Global Call Reference decoder against short buffers...
+ osmo_dec_gcr(len=13) -> FAIL
+ osmo_dec_gcr(len=14) -> FAIL
+ osmo_dec_gcr(len=15) -> OK
Done.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/43190?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: I6855d6c810c2b4274ccfd7bc861405f9b4e09343
Gerrit-Change-Number: 43190
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: laforge <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
