From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 12 Feb 2023 15:15:32 +0000 (-0500)
Subject: check argv[] before doing any decoding of packets.
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=70e4c1a273cb1d232c84329b08b8ba2e83517df6;p=thirdparty%2Ffreeradius-server.git

check argv[] before doing any decoding of packets.
---

diff --git a/src/protocols/tacacs/decode.c b/src/protocols/tacacs/decode.c
index 70cba504a25..3c53d9e30f6 100644
--- a/src/protocols/tacacs/decode.c
+++ b/src/protocols/tacacs/decode.c
@@ -133,12 +133,20 @@ int fr_tacacs_packet_to_code(fr_tacacs_packet_t const *pkt)
 	} \
 } while (0)
 
-#define ARG_COUNT_CHECK(_msg, _arg_cnt) do { \
-	if ((p + _arg_cnt) > end) { \
-		fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the %s packet", _arg_cnt, end - p, _msg); \
+#define ARG_COUNT_CHECK(_msg, _hdr) do { \
+	if ((p + _hdr.arg_cnt) > end) { \
+		fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the %s packet", _hdr.arg_cnt, end - p, _msg); \
 		goto fail; \
 	} \
-	p += _arg_cnt; \
+	p += _hdr.arg_cnt; \
+	data_len = 0; \
+	for (int i = 0; i < _hdr.arg_cnt; i++) { \
+		data_len += _hdr.arg_len[i]; \
+		if (data_len > (size_t) (end - p)) { \
+			fr_strerror_printf("Argument %u length %u overflows packet", i, _hdr.arg_len[i]); \
+			goto fail; \
+		} \
+	} \
 } while (0)
 
 #define DECODE_FIELD_UINT8(_da, _field) do { \
@@ -160,11 +168,11 @@ int fr_tacacs_packet_to_code(fr_tacacs_packet_t const *pkt)
 
 #define BODY(_x) (((uint8_t const *) pkt) + sizeof(pkt->hdr) + sizeof(pkt->_x))
 
-/**
- *	Decode a TACACS+ 'arg_N' fields.
+/** Decode a TACACS+ 'arg_N' fields.
+ *
  */
 static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr_t const *parent,
-			      uint8_t arg_cnt, uint8_t const *arg_list, uint8_t const *data, uint8_t const *end)
+			      uint8_t arg_cnt, uint8_t const *arg_list, uint8_t const *data, NDEBUG_UNUSED uint8_t const *end)
 {
 	uint8_t i;
 	uint8_t const *p = data;
@@ -175,24 +183,7 @@ static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr
 	 */
 	if (!arg_cnt) return 0;
 
-	if ((p + arg_cnt) > end) {
-		fr_strerror_printf("Argument count %u overflows the remaining data (%zu) in the packet", arg_cnt, p - end);
-		return -1;
-	}
-
-	/*
-	 *	Check for malformed packets before anything else.
-	 */
-	for (i = 0; i < arg_cnt; i++) {
-		if ((p + arg_list[i]) > end) {
-			fr_strerror_printf("'%s' argument %u length %u overflows the remaining data (%zu) in the packet",
-					   parent->name, i, arg_list[i], end - p);
-			return -1;
-		}
-
-		p += arg_list[i];
-	}
-	p = data;
+	fr_assert((p + arg_cnt) < end);
 
 	/*
 	 *	Then, do the dirty job of creating attributes.
@@ -202,6 +193,9 @@ static int tacacs_decode_args(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr
 		fr_dict_attr_t const *da;
 		uint8_t buffer[256];
 
+		fr_assert(p < end);
+		fr_assert((p + arg_list[i]) <= end);
+
 		if (arg_list[i] < 2) goto next; /* skip malformed */
 
 		memcpy(buffer, p, arg_list[i]);
@@ -771,7 +765,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
 			/* can't check for underflow, as we have argv[argc] */
 
 			p = BODY(author_req);
-			ARG_COUNT_CHECK("Authorization-Request", pkt->author_req.arg_cnt);
+			ARG_COUNT_CHECK("Authorization-Request", pkt->author_req);
 			DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_REQUEST);
 
 			/*
@@ -830,7 +824,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
 			/* can't check for underflow, as we have argv[argc] */
 
 			p = BODY(author_reply);
-			ARG_COUNT_CHECK("Authorization-Reply", pkt->author_reply.arg_cnt);
+			ARG_COUNT_CHECK("Authorization-Reply", pkt->author_reply);
 			DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_RESPONSE);
 
 			/*
@@ -893,7 +887,7 @@ ssize_t fr_tacacs_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *bu
 			/* can't check for underflow, as we have argv[argc] */
 
 			p = BODY(acct_req);
-			ARG_COUNT_CHECK("Accounting-Request", pkt->acct_req.arg_cnt);
+			ARG_COUNT_CHECK("Accounting-Request", pkt->acct_req);
 			DECODE_FIELD_UINT8(attr_tacacs_packet_body_type, FR_PACKET_BODY_TYPE_REQUEST);
 
 			/*
diff --git a/src/tests/unit/protocols/tacacs/base.txt b/src/tests/unit/protocols/tacacs/base.txt
index ad505253d1a..7450b934684 100644
--- a/src/tests/unit/protocols/tacacs/base.txt
+++ b/src/tests/unit/protocols/tacacs/base.txt
@@ -67,7 +67,7 @@ encode-proto -
 match c0 03 02 00 07 9b 35 d9 00 00 00 05 49 d8 e5 4a 73
 
 decode-proto c002 20ff 2020 2020 0000 0043 2009 0000 0009 000a 2120 2020 2020 2020 2020 20ff ff20 2020 2020 2020 ffff ffff 2020 4441 5461 2a30 7820 2020 2020 2020 2020 2020 2020 2020 20ff ffff 20ff ff20 2020 20
-match 'Argument-List' argument 3 length 32 overflows the remaining data (0) in the packet
+match Argument 3 length 32 overflows packet
 
 count
 match 29