fixeria has uploaded this change for review. ( 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, 34 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/90/43190/1
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..6bcf602 100644 --- a/tests/gsm29205/gsm29205_test.c +++ b/tests/gsm29205/gsm29205_test.c @@ -95,6 +95,33 @@ 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("\tlen=%u: rc=%d\n", len, rc); + } +} + int main(int argc, char **argv) { osmo_init_logging2(talloc_named_const(NULL, 0, "gsm29205 test"), NULL); @@ -102,6 +129,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..d83db2e 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... + len=13: rc=-74 + len=14: rc=-74 + len=15: rc=15 Done.
-
fixeria