fixeria has uploaded this change for review.

View Change

sua: sua_parse_gt(): reject GT with num_digits exceeding blob length

The digit-parsing loop only clamped num_digits against the capacity of
gt->digits[], but never validated it against the actual length of the
given input.  A blob claiming more digits than it carries was silently
truncated to whatever data happened to be available, without any error
indication to the caller.

Add an explicit check of num_digits against datalen before the parsing
loop, alongside the existing gt->digits[] capacity check.  This keeps
the loop body free of per-iteration bounds checks, and reports -EINVAL
to the caller for such malformed input.

Change-Id: I918278b6471bc78511bf97a292204e6719d4031a
Related: OS#7046
---
M src/sua.c
M tests/xua/xua_test.c
M tests/xua/xua_test.ok
3 files changed, 34 insertions(+), 6 deletions(-)

git pull ssh://gerrit.osmocom.org:29418/libosmo-sigtran refs/changes/49/43349/1

diff --git a/src/sua.c b/src/sua.c
index 7c6d57f..be18d5c 100644
--- a/src/sua.c
+++ b/src/sua.c
@@ -412,16 +412,20 @@
 		rc = -ENOSPC;
 	}
 
-	/* parse digits: bounded by num_digits (already clamped above), so this
-	 * can never write more than sizeof(gt->digits)-1 nibbles into gt->digits[] */
+	/* claimed num_digits must fit into the remaining input octets */
+	if (num_digits > (datalen - 8) * 2) {
+		/* Parse as much as we can; return -EINVAL */
+		num_digits = (datalen - 8) * 2;
+		rc = -EINVAL;
+	}
+
+	/* parse digits: bounded by num_digits
+	 * (already clamped above to fit both gt->digits[] and the input data) */
 	out_digits = gt->digits;
 	for (i = 0; i < num_digits; i++) {
 		unsigned int byte_off = 8 + i / 2;
-		uint8_t byte;
+		uint8_t byte = data[byte_off];
 
-		if (byte_off >= datalen)
-			break;
-		byte = data[byte_off];
 		if (i & 1)
 			*out_digits++ = osmo_bcd2char(byte >> 4);
 		else
diff --git a/tests/xua/xua_test.c b/tests/xua/xua_test.c
index dcd2089..76fe9db 100644
--- a/tests/xua/xua_test.c
+++ b/tests/xua/xua_test.c
@@ -397,6 +397,27 @@
 	printf("OUT:%s\n", osmo_sccp_gt_dump(&gt));
 }
 
+static void test_sua_parse_gt_truncated(void)
+{
+	/* 8-byte header + only 2 digit octets (4 digits worth of data) */
+	uint8_t data[8 + 2];
+	struct osmo_sccp_gt gt = {};
+
+	memset(data, 0x11, sizeof(data));
+	data[3] = 0x42;		/* gti */
+	data[5] = 0x00;		/* tt */
+	data[6] = 0x01;		/* npi */
+	data[7] = 0x04;		/* nai */
+
+	/* num_digits fits into gt->digits[], but claims more digits than the
+	 * given blob actually carries */
+	data[4] = 10;
+	printf("Testing sua_parse_gt() with num_digits=%u, datalen=%zu\n", data[4], sizeof(data));
+	OSMO_ASSERT(sua_parse_gt(&gt, data, sizeof(data)) == -EINVAL);
+	OSMO_ASSERT(strlen(gt.digits) == (sizeof(data) - 8) * 2);
+	printf("OUT:%s\n", osmo_sccp_gt_dump(&gt));
+}
+
 /* SCCP Message Transcoding */
 
 struct sccp2sua_testcase {
@@ -705,6 +726,7 @@
 	test_sccp_addr_parser();
 	test_helpers();
 	test_sua_parse_gt_overflow();
+	test_sua_parse_gt_truncated();
 	test_sccp2sua();
 	test_rkm();
 	test_sccp_addr_encdec();
diff --git a/tests/xua/xua_test.ok b/tests/xua/xua_test.ok
index f6f30d6..b3bc8e3 100644
--- a/tests/xua/xua_test.ok
+++ b/tests/xua/xua_test.ok
@@ -20,6 +20,8 @@
 OUT:DIG=1111111111111111111111111111111
 Testing sua_parse_gt() with num_digits=255
 OUT:DIG=1111111111111111111111111111111
+Testing sua_parse_gt() with num_digits=10, datalen=10
+OUT:DIG=1111
 
 => BSSMAP-RESET
 SCCP Input: [L2]> 09 00 03 05 07 02 42 fe 02 42 fe 06 00 04 30 04 01 20 

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