From 885850b6aaabf089f422b1b015481a0ccff4f90e Mon Sep 17 00:00:00 2001
From: Noel Power <noel.power@suse.com>
Date: Thu, 8 Feb 2024 14:05:43 +0000
Subject: [PATCH] s3/rpc_client: Fix array offset check

Previous to this commit we were modifying the offset before
the array offset check. This was causing a spurious debug
message indicating the offset was out of bounds. An second
problem is that upon detecting the error we don't exit the loop.
A third problem was that when reading the offset the check
didn't cater for the size of the integer address about to be read.

This commit moves the offset check to before the first read,
additionally when an error is detected now we actually exit the loop
and the offset have been corrected to include the size of the
integer to be read

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15579
Signed-off-by: Noel Power <noel.power@suse.com>
Reviewed-by: Volker Lendecke <vl@samba.org>

Autobuild-User(master): Volker Lendecke <vl@samba.org>
Autobuild-Date(master): Sat Feb 17 17:58:43 UTC 2024 on atb-devel-224
---
 source3/rpc_client/wsp_cli.c | 61 ++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c
index 39d1f7868d0..15b6e36007e 100644
--- a/source3/rpc_client/wsp_cli.c
+++ b/source3/rpc_client/wsp_cli.c
@@ -938,6 +938,15 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx,
 		count = 1;
 	}
 
+	/* ensure count is at least within buffer range */
+	if (count >= MAX_ROW_BUFF_SIZE || count >= rows_buf->length) {
+		DBG_ERR("count %"PRIu64" either exceeds max buffer size "
+			"or buffer size (%zu)",
+			count,  rows_buf->length);
+		err = NDR_ERR_VALIDATE;
+		goto out;
+	}
+
 	/* read address */
 	if (is_64bit) {
 		err = ndr_pull_udlong(ndr_pull,
@@ -974,30 +983,64 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx,
 		goto out;
 	}
 
+	/*
+	 * non vector case addr points to value
+	 * otherwise addr points to list of addresses
+	 * for the values in vector
+	 */
 	if (is_vector == false) {
 		vec_address[0] = addr;
 	} else {
 		uint64_t array_offset = addr - baseaddress;
 		uint64_t i;
+		uint32_t intsize;
+
+		if (is_64bit) {
+			intsize = 8;
+		} else {
+			intsize = 4;
+		}
+
+		if (array_offset >= MAX_ROW_BUFF_SIZE
+		    || array_offset >= rows_buf->length) {
+			DBG_ERR("offset %"PRIu64" either exceeds max buf size "
+				"or buffer size (%zu)",
+				array_offset,  rows_buf->length);
+			err = NDR_ERR_VALIDATE;
+			goto out;
+		}
+
+		/* addr points to a list of int32 or int64 addresses */
 		for (i = 0; i < count; i++) {
+			/*
+			 * read the addresses of the vector elements
+			 * note: we can safely convert the uint64_t
+			 *       values here to uint32_t values as
+			 *       we are sure they are within range
+			 *       due to previous checks above.
+			 */
+			if (smb_buffer_oob((uint32_t)rows_buf->length,
+					   (uint32_t)array_offset,
+					   intsize)) {
+				DBG_ERR("offset %"PRIu64" will be outside "
+					"buffer range (buf len - %zu) after "
+					"reading %s address\n",
+					array_offset,
+					rows_buf->length,
+					is_64bit ? "64 bit" : "32 bit");
+				err = NDR_ERR_VALIDATE;
+				goto out;
+			}
 			if (is_64bit) {
 				vec_address[i] =
 					PULL_LE_I64(rows_buf->data,
 						array_offset);
-				array_offset = array_offset + 8;
 			} else {
 				vec_address[i] =
 					(uint32_t)PULL_LE_I32(rows_buf->data,
 							array_offset);
-				array_offset = array_offset + 4;
-			}
-			if (array_offset >= rows_buf->length) {
-				DBG_ERR("offset %"PRIu64" outside buffer range "
-					"(buf len - %zu)\n",
-					array_offset,
-					rows_buf->length);
-				err = NDR_ERR_VALIDATE;
 			}
+			array_offset += intsize;
 		}
 	}
 	err  = NDR_ERR_SUCCESS;