dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/43953?usp=email )

Change subject: gprs_sndcp_xid: fix length check in gprs_sndcp_compile_xid ......................................................................

gprs_sndcp_xid: fix length check in gprs_sndcp_compile_xid

The function gprs_sndcp_compile_xid gets an output buffer (dst) and a length (dst_maxlen), but unfortunately, the length is not checked before data is written to the output buffer (tlv_put).

The problem was reported by osmocom.org user tsato (see related ticket)

Related: OS#7095 Change-Id: I977946a036f96d4dee2656bef048e2618198db40 --- M src/sgsn/gprs_sndcp_xid.c M tests/sndcp_xid/sndcp_xid_test.c M tests/sndcp_xid/sndcp_xid_test.ok 3 files changed, 41 insertions(+), 2 deletions(-)

Approvals: fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved Jenkins Builder: Verified

diff --git a/src/sgsn/gprs_sndcp_xid.c b/src/sgsn/gprs_sndcp_xid.c index 344fda4..0b981b5 100644 --- a/src/sgsn/gprs_sndcp_xid.c +++ b/src/sgsn/gprs_sndcp_xid.c @@ -593,8 +593,10 @@ OSMO_ASSERT(rc >= 0);

if (rc > 0) { + byte_counter += TLV_GROSS_LEN(rc); + if (byte_counter > dst_maxlen) + return -ENOMEM; dst = tlv_put(dst, SNDCP_XID_DATA_COMPRESSION, rc, comp_bytes); - byte_counter += rc + 2; }

/* Add header compression fields */ @@ -604,9 +606,11 @@ OSMO_ASSERT(rc >= 0);

if (rc > 0) { + byte_counter += TLV_GROSS_LEN(rc); + if (byte_counter > dst_maxlen) + return -ENOMEM; dst = tlv_put(dst, SNDCP_XID_PROTOCOL_COMPRESSION, rc, comp_bytes); - byte_counter += rc + 2; }

/* Return generated length */ diff --git a/tests/sndcp_xid/sndcp_xid_test.c b/tests/sndcp_xid/sndcp_xid_test.c index 92f04ba..5cb7817 100644 --- a/tests/sndcp_xid/sndcp_xid_test.c +++ b/tests/sndcp_xid/sndcp_xid_test.c @@ -244,6 +244,38 @@ talloc_free(comp_fields_dec); }

+/* Encode fields SNDCP-XID fields into a destination buffer that is to small */ +static void test_xid_encode_overflow(const void *ctx) +{ + printf("Testing SNDCP XID-Encoder with too small output buffer (overflow)\n"); + + LLIST_HEAD(comp_fields); + struct gprs_sndcp_pcomp_rfc1144_params rfc1144_params; + struct gprs_sndcp_comp_field rfc1144_comp_field; + + uint8_t xid[16]; + unsigned int xid_len = sizeof(xid); + int rc; + + /* Setup dummy rfc1144 compression fields. This field is artificially enlarged and the encoding result will + not fit into the xid buffer above. (we will leave all rfc1144_comp_field.comp values at zero) */ + memset(&rfc1144_comp_field, 0, sizeof(struct gprs_sndcp_comp_field)); + rfc1144_params.nsapi[0] = 5; + rfc1144_params.nsapi_len = 1; + rfc1144_params.s01 = 7; + rfc1144_comp_field.p = 1; + rfc1144_comp_field.entity = 0; + rfc1144_comp_field.algo.pcomp = RFC_1144; + rfc1144_comp_field.comp_len = 15; + rfc1144_comp_field.rfc1144_params = &rfc1144_params; + llist_add(&rfc1144_comp_field.list, &comp_fields); + + rc = gprs_sndcp_compile_xid(xid, xid_len, &comp_fields, + DEFAULT_SNDCP_VERSION); + printf("rc: %d\n", rc); + OSMO_ASSERT(rc < 0); +} + static struct log_info_cat gprs_categories[] = { [DSNDCP] = { .name = "DSNDCP", @@ -269,6 +301,7 @@

test_xid_decode_realworld(xid_ctx); test_xid_encode_decode(xid_ctx); + test_xid_encode_overflow(xid_ctx);

printf("Done\n");

diff --git a/tests/sndcp_xid/sndcp_xid_test.ok b/tests/sndcp_xid/sndcp_xid_test.ok index f357282..f23b119 100644 --- a/tests/sndcp_xid/sndcp_xid_test.ok +++ b/tests/sndcp_xid/sndcp_xid_test.ok @@ -8,4 +8,6 @@ Test input data: Encoded: 000100011a83010dab00208003012c012c02580258830007a000200308001402408000041200200781010c3456700040010005a80f000f82022789ffe0000f00a80000000101010002010200030103000401040005010500060007010700080108 (97 bytes) Decoded: +Testing SNDCP XID-Encoder with too small output buffer (overflow) +rc: -12 Done