Change in libosmocore[master]: tlv_parser: Fix various out-of-bounds accesses

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

laforge gerrit-no-reply at lists.osmocom.org
Tue Jan 12 22:16:24 UTC 2021


laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/22113 )

Change subject: tlv_parser: Fix various out-of-bounds accesses
......................................................................

tlv_parser: Fix various out-of-bounds accesses

The libosmocore TLV parser had a number of insufficient bounds checks
leading to reads beyond the end of the respective input buffer.

This patch
* adds proper out-of-bounds checks to all TLV types
* simplifies some of the existing checks
* introduces test cases to test all the corner cases
  where either TAG, or length, or value are not fully contained
  in the input buffer.

Thanks to Ilja Van Sprundel for reporting these problems.

Change-Id: I98b02c914c9e3ecf56050af846292aa6979d7508
---
M src/gsm/tlv_parser.c
M tests/tlv/tlv_test.c
M tests/tlv/tlv_test.ok
3 files changed, 115 insertions(+), 15 deletions(-)

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



diff --git a/src/gsm/tlv_parser.c b/src/gsm/tlv_parser.c
index 7930d64..967539e 100644
--- a/src/gsm/tlv_parser.c
+++ b/src/gsm/tlv_parser.c
@@ -232,7 +232,10 @@
 		  const uint8_t *buf, int buf_len)
 {
 	uint8_t tag;
-	int len;
+	int len; /* number of bytes consumed by TLV entry */
+
+	if (buf_len < 1)
+		return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 
 	tag = *buf;
 	*o_tag = tag;
@@ -265,56 +268,54 @@
 		break;
 	case TLV_TYPE_TLV:
 tlv:		/* GSM TS 04.07 11.2.4: Type 4 TLV */
-		if (buf + 1 > buf + buf_len)
+		if (buf_len < 2)
 			return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 		*o_val = buf+2;
 		*o_len = *(buf+1);
 		len = *o_len + 2;
-		if (len > buf_len)
-			return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;
 		break;
 	case TLV_TYPE_vTvLV_GAN:	/* 44.318 / 11.1.4 */
 		/* FIXME: variable-length TAG! */
+		if (buf_len < 2)
+			return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 		if (*(buf+1) & 0x80) {
-			/* like TL16Vbut without highest bit of len */
-			if (2 > buf_len)
+			if (buf_len < 3)
 				return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
+			/* like TL16Vbut without highest bit of len */
 			*o_val = buf+3;
 			*o_len = (*(buf+1) & 0x7F) << 8 | *(buf+2);
 			len = *o_len + 3;
-			if (len > buf_len)
-				return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;
 		} else {
 			/* like TLV */
 			goto tlv;
 		}
 		break;
 	case TLV_TYPE_TvLV:
+		if (buf_len < 2)
+			return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 		if (*(buf+1) & 0x80) {
 			/* like TLV, but without highest bit of len */
-			if (buf + 1 > buf + buf_len)
-				return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 			*o_val = buf+2;
 			*o_len = *(buf+1) & 0x7f;
 			len = *o_len + 2;
-			if (len > buf_len)
-				return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;
 			break;
 		}
 		/* like TL16V, fallthrough */
 	case TLV_TYPE_TL16V:
-		if (2 > buf_len)
+		if (buf_len < 3)
 			return OSMO_TLVP_ERR_OFS_BEYOND_BUFFER;
 		*o_val = buf+3;
 		*o_len = *(buf+1) << 8 | *(buf+2);
 		len = *o_len + 3;
-		if (len > buf_len)
-			return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;
 		break;
 	default:
 		return OSMO_TLVP_ERR_UNKNOWN_TLV_TYPE;
 	}
 
+	if (buf_len < len) {
+		*o_val = NULL;
+		return OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER;
+	}
 	return len;
 }
 
diff --git a/tests/tlv/tlv_test.c b/tests/tlv/tlv_test.c
index 925d762..3b4f441 100644
--- a/tests/tlv/tlv_test.c
+++ b/tests/tlv/tlv_test.c
@@ -332,6 +332,97 @@
 	msgb_free(msg);
 }
 
+static void test_tlv_parser_bounds()
+{
+	struct tlv_definition tdef;
+	struct tlv_parsed dec;
+	uint8_t buf[32];
+
+	memset(&tdef, 0, sizeof(tdef));
+
+	printf("Testing TLV_TYPE_T decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_T;
+	buf[0] = 0x23;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == 1);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 0, 0, 0) == 0);
+
+	printf("Testing TLV_TYPE_TV decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_TV;
+	buf[0] = 0x23;
+	buf[1] = 0x42;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == 1);
+	OSMO_ASSERT(*TLVP_VAL(&dec, 0x23) == buf[1]);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+
+	printf("Testing TLV_TYPE_FIXED decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_FIXED;
+	tdef.def[0x23].fixed_len = 2;
+	buf[0] = 0x23;
+	buf[1] = 0x42;
+	buf[2] = 0x55;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == 1);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+
+	printf("Testing TLV_TYPE_TLV decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_TLV;
+	buf[0] = 0x23;
+	buf[1] = 0x02;
+	buf[2] = 0x55;
+	buf[3] = 0xAA;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[2]);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+
+	printf("Testing TLV_TYPE_vTvLV_GAN decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_vTvLV_GAN;
+	buf[0] = 0x23;
+	buf[1] = 0x80;
+	buf[2] = 0x01;
+	buf[3] = 0xAA;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[3]);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+
+	printf("Testing TLV_TYPE_TvLV decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_TvLV;
+	buf[0] = 0x23;
+	buf[1] = 0x81;
+	buf[2] = 0xAA;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == 1);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[2]);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+
+	printf("Testing TLV_TYPE_TL16V decoder for out-of-bounds\n");
+	tdef.def[0x23].type = TLV_TYPE_TL16V;
+	buf[0] = 0x23;
+	buf[1] = 0x00;
+	buf[2] = 0x01;
+	buf[3] = 0xAA;
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 4, 0, 0) == 1);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == &buf[3]);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 3, 0, 0) == OSMO_TLVP_ERR_OFS_LEN_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 2, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+	OSMO_ASSERT(tlv_parse(&dec, &tdef, buf, 1, 0, 0) == OSMO_TLVP_ERR_OFS_BEYOND_BUFFER);
+	OSMO_ASSERT(TLVP_VAL(&dec, 0x23) == NULL);
+}
+
 int main(int argc, char **argv)
 {
 	//osmo_init_logging2(ctx, &info);
@@ -339,6 +430,7 @@
 	test_tlv_shift_functions();
 	test_tlv_repeated_ie();
 	test_tlv_encoder();
+	test_tlv_parser_bounds();
 
 	printf("Done.\n");
 	return EXIT_SUCCESS;
diff --git a/tests/tlv/tlv_test.ok b/tests/tlv/tlv_test.ok
index f3f0fd4..e24b889 100644
--- a/tests/tlv/tlv_test.ok
+++ b/tests/tlv/tlv_test.ok
@@ -1,4 +1,11 @@
 Test shift functions
 Testing TLV encoder by decoding + re-encoding binary
 Testing TLV encoder with IE ordering
+Testing TLV_TYPE_T decoder for out-of-bounds
+Testing TLV_TYPE_TV decoder for out-of-bounds
+Testing TLV_TYPE_FIXED decoder for out-of-bounds
+Testing TLV_TYPE_TLV decoder for out-of-bounds
+Testing TLV_TYPE_vTvLV_GAN decoder for out-of-bounds
+Testing TLV_TYPE_TvLV decoder for out-of-bounds
+Testing TLV_TYPE_TL16V decoder for out-of-bounds
 Done.

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/22113
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I98b02c914c9e3ecf56050af846292aa6979d7508
Gerrit-Change-Number: 22113
Gerrit-PatchSet: 3
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210112/2d8e71d5/attachment.htm>


More information about the gerrit-log mailing list