dexter has uploaded this change for review.

View Change

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: I38844155027673d90079c84df5cfd85053f48656
---
M src/sndcp/sndcp_xid.c
M tests/sndcp/sndcp_xid_test.c
M tests/sndcp/sndcp_xid_test.ok
3 files changed, 41 insertions(+), 2 deletions(-)

git pull ssh://gerrit.osmocom.org:29418/libosmo-gprs refs/changes/62/43962/1

diff --git a/src/sndcp/sndcp_xid.c b/src/sndcp/sndcp_xid.c
index 4442f18..28e8928 100644
--- a/src/sndcp/sndcp_xid.c
+++ b/src/sndcp/sndcp_xid.c
@@ -587,8 +587,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 */
@@ -598,9 +600,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/sndcp_xid_test.c b/tests/sndcp/sndcp_xid_test.c
index 63a479a..a9f576a 100644
--- a/tests/sndcp/sndcp_xid_test.c
+++ b/tests/sndcp/sndcp_xid_test.c
@@ -246,6 +246,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",
@@ -276,6 +308,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/sndcp_xid_test.ok b/tests/sndcp/sndcp_xid_test.ok
index f357282..f23b119 100644
--- a/tests/sndcp/sndcp_xid_test.ok
+++ b/tests/sndcp/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

To view, visit change 43962. To unsubscribe, or for help writing mail filters, visit settings.