From 0225491b541aa54a52e9253b2be551a67b0d77d9 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 14 Feb 2024 12:01:28 +0000 Subject: [PATCH 1/3] idl: Add constant for max rows buffer size BUG: https://bugzilla.samba.org/show_bug.cgi?id=15579 Signed-off-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit 01e901ef869a1a87fba0e67bce311dbeb199b717) --- librpc/idl/wsp_data.idl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/librpc/idl/wsp_data.idl b/librpc/idl/wsp_data.idl index 2a94355b0b0..fde754aef81 100644 --- a/librpc/idl/wsp_data.idl +++ b/librpc/idl/wsp_data.idl @@ -11,6 +11,11 @@ interface constants * for details of this and other language id(s) */ const uint32_t WSP_DEFAULT_LCID = 0x00000409; + /* + * Max size of rows buffer in getrowsout response + * see MS-WSP 2.2.3.11 + */ + const uint32_t MAX_ROW_BUFF_SIZE = 0x0004000; /* values for guidPropertySet */ const char* DBPROPSET_FSCIFRMWRK_EXT = "A9BD1526-6A80-11D0-8C9D-0020AF1D740E"; -- 2.35.3 From 6ccea75f5e4859c67f4f09940f8fdea4ae11dcb7 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 14 Feb 2024 11:19:39 +0000 Subject: [PATCH 2/3] s3/rpc_client: Ensure max possible row buffer size is not exceeded The max buf size of rows buffer should not exceed 0x00004000. Ensuring this value is within limits means we can safely use uint32_t offsets. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15579 Signed-off-by: Noel Power Reviewed-by: Volker Lendecke (cherry picked from commit f487211706a74d516bf447ed393222b4c0dce7b0) --- source3/rpc_client/wsp_cli.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c index b21f55c86f0..1ce28e61807 100644 --- a/source3/rpc_client/wsp_cli.c +++ b/source3/rpc_client/wsp_cli.c @@ -1311,6 +1311,19 @@ enum ndr_err_code extract_rowsarray( { uint32_t i; enum ndr_err_code err = NDR_ERR_SUCCESS; + /* + * limit check the size of rows_buf + * see MS-WSP 2.2.3.11 which describes the size + * of the rows buffer MUST not exceed 0x0004000 bytes. + * This limit will ensure we can safely check + * limits based on uint32_t offsets + */ + + if (rows_buf->length > MAX_ROW_BUFF_SIZE) { + DBG_ERR("Buffer size 0x%zx exceeds 0x%x max buffer size\n", + rows_buf->length, MAX_ROW_BUFF_SIZE); + return NDR_ERR_BUFSIZE; + } for (i = 0; i < rows; i++ ) { struct wsp_cbasestoragevariant *cols = -- 2.35.3 From 4ed6ddc2ab800253f5e965478afdb3aec8e96bab Mon Sep 17 00:00:00 2001 From: Noel Power Date: Thu, 8 Feb 2024 14:05:43 +0000 Subject: [PATCH 3/3] 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 Reviewed-by: Volker Lendecke Autobuild-User(master): Volker Lendecke Autobuild-Date(master): Sat Feb 17 17:58:43 UTC 2024 on atb-devel-224 (cherry picked from commit 885850b6aaabf089f422b1b015481a0ccff4f90e) [npower@samba.org Adjusted vector processing as this function was rewritten in master] --- source3/rpc_client/wsp_cli.c | 54 ++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c index 1ce28e61807..955bbb72963 100644 --- a/source3/rpc_client/wsp_cli.c +++ b/source3/rpc_client/wsp_cli.c @@ -939,6 +939,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, @@ -977,25 +986,66 @@ 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_addr = addr; uint64_t i; + uint32_t intsize; + + if (is_64bit) { + intsize = 8; + } else { + intsize = 4; + } + + if (array_addr >= MAX_ROW_BUFF_SIZE + || array_addr >= rows_buf->length) { + DBG_ERR("offset %"PRIu64" either exceeds max buf size " + "or buffer size (%zu)", + array_addr, 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_addr, + intsize)) { + DBG_ERR("offset %"PRIu64" will be outside " + "buffer range (buf len - %zu) after " + "reading %s address\n", + array_addr, + 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_addr); - array_addr = array_addr + 8; } else { vec_address[i] = (uint32_t)PULL_LE_I32(rows_buf->data, array_addr); - array_addr = array_addr + 4; } /* adjust address */ vec_address[i] -= offset; + array_addr += intsize; } } err = NDR_ERR_SUCCESS; -- 2.35.3