From 4fb578cce1ba0b605423ecb3bd79a47c1fa07f60 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Mon, 8 Jan 2024 15:12:35 +0000 Subject: [PATCH 1/7] s3/rpc_client: change type of offset to uint64_t Offset can be a 32 or 64 bit address depending on the indexing addressing mode negotiated by the client With a 32 bit param we can only specify a 32 bit base address. This change alone doesn't affect anything as it is the client itself that choses and passes the base address offset and wspsearch is the only current user of this code. In this case even with 64bit addressing negotiated the address passed represents only the lower 32-bits part of the address. However, for coverage purposes it would be better for the client to use an address that covers the full 64bit range of the address (when 64 bit addressing is negotiated). This change will alow the wspsearch client in a future commit to pass a base address value with both the hi and low 32 bits values set to make up the full 64 bit address. Signed-off-by: Noel Power Reviewed-by: Andrew Bartlett (cherry picked from commit a61eb7032896265eaef3ba225aafd6f293e7569d) --- source3/rpc_client/wsp_cli.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c index b21f55c86f0..450bd2525e0 100644 --- a/source3/rpc_client/wsp_cli.c +++ b/source3/rpc_client/wsp_cli.c @@ -761,7 +761,7 @@ void create_seekat_getrows_request(TALLOC_CTX * ctx, static bool extract_rowbuf_variable_type(TALLOC_CTX *ctx, uint16_t type, - uint32_t offset, + uint64_t offset, DATA_BLOB *rows_buf, uint32_t len, struct wsp_cbasestoragevariant *val) { @@ -770,7 +770,7 @@ static bool extract_rowbuf_variable_type(TALLOC_CTX *ctx, ndr_flags_type ndr_flags = NDR_SCALARS | NDR_BUFFERS; DATA_BLOB variant_blob = data_blob_null; if (offset >= rows_buf->length) { - DBG_ERR("offset %d outside buffer range (buf len - %zu)", + DBG_ERR("offset %"PRIu64" outside buffer range (buf len - %zu)", offset, rows_buf->length); return false; @@ -902,7 +902,7 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint32_t offset, + uint64_t offset, DATA_BLOB *rows_buf, uint64_t *pcount, uint64_t **pvec_address/*, @@ -1010,7 +1010,7 @@ static enum ndr_err_code extract_crowvariant_variable(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint32_t offset, + uint64_t offset, DATA_BLOB *rows_buf, uint32_t len, struct wsp_cbasestoragevariant *val) @@ -1116,7 +1116,7 @@ static enum ndr_err_code extract_crowvariant(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint32_t offset, + uint64_t offset, DATA_BLOB *rows_buf, uint32_t len, struct wsp_cbasestoragevariant *val) { -- 2.35.3 From b739db32828e51b1861a76a98d0e119d4b300dc3 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 10 Jan 2024 10:59:23 +0000 Subject: [PATCH 2/7] s3/rpc_client: Remove stray unnecessary comment Signed-off-by: Noel Power Reviewed-by: Andrew Bartlett (cherry picked from commit efa60ff3105ac80ffff6d2a5d82dd0615ddb7578) --- source3/rpc_client/wsp_cli.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c index 450bd2525e0..7fd221dd731 100644 --- a/source3/rpc_client/wsp_cli.c +++ b/source3/rpc_client/wsp_cli.c @@ -905,8 +905,7 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, uint64_t offset, DATA_BLOB *rows_buf, uint64_t *pcount, - uint64_t **pvec_address/*, - struct wsp_cbasestoragevariant ***variant_array*/) + uint64_t **pvec_address) { bool is_vector = tablevar->vtype & VT_VECTOR; uint64_t count; -- 2.35.3 From 17e3f5556e2229f44415f72f3334a02ed90cabfe Mon Sep 17 00:00:00 2001 From: Noel Power Date: Mon, 8 Jan 2024 15:56:38 +0000 Subject: [PATCH 3/7] s3/utils: use full 64 bit address for getrows (with 64bit offsets) if 64bit offsets are used the hi 32-bits of address are stored in the ulreserved2 member of the message header field and the low 32-bits are stored in the ulclientbase member of the cpmgetrows message Signed-off-by: Noel Power Reviewed-by: Andrew Bartlett (cherry picked from commit 6ecb614b8ec6953ba15e8061fce9b395615b035a) --- source3/utils/wspsearch.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/source3/utils/wspsearch.c b/source3/utils/wspsearch.c index 2c56c97736b..063b952d468 100644 --- a/source3/utils/wspsearch.c +++ b/source3/utils/wspsearch.c @@ -350,6 +350,10 @@ static NTSTATUS create_getrows(TALLOC_CTX *ctx, uint32_t INITIAL_ROWS = 32; uint32_t requested_rows = INITIAL_ROWS; uint32_t rows_printed; + uint64_t baseaddress; + uint32_t offset_lowbits = 0xdeabd860; + uint32_t offset_hibits = 0xfeeddeaf; + TALLOC_CTX *row_ctx; bool loop_again; @@ -377,10 +381,24 @@ static NTSTATUS create_getrows(TALLOC_CTX *ctx, skip, requested_rows, 40, - 0xDEAbd860, + offset_lowbits, bindings->brow, 0); + if (is_64bit) { + /* + * MS-WSP 2.2.2 + * ulreservered holds the high 32-bits part of + * a 64-bit offset if 64-bit offsets are being used. + */ + request->header.ulreserved2 = offset_hibits; + baseaddress = request->header.ulreserved2; + baseaddress <<= 32; + baseaddress += offset_lowbits; + } else { + baseaddress = offset_lowbits; + } + status = wsp_request_response(request, wsp_ctx, request, @@ -419,7 +437,7 @@ static NTSTATUS create_getrows(TALLOC_CTX *ctx, is_64bit, disp_all_cols, bindings, 40, - 0xDEAbd860, + baseaddress, response->message.cpmgetrows.rowsreturned, &rows_printed); if (!NT_STATUS_IS_OK(status)) { -- 2.35.3 From 994f5e9e662c52707def09b61a72e9f838c4ff83 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 10 Jan 2024 14:43:58 +0000 Subject: [PATCH 4/7] s3/rpc_client: cleanup unmarshalling of variant types from row columns Prior to this change fn 'extract_variant_addresses' actually returns offsets to the variant stored not the addresses, additionally the param in the signature of the method is named offset where the param in reality is a base address. This change makes fn 'extract_variant_addresses' actually return addresses instead of offsets and also changes the name of the incoming param. The resulting changes are propaged to callers which hopefully makes what the code is actually doing a little clearer Signed-off-by: Noel Power Reviewed-by: Andrew Bartlett Autobuild-User(master): Noel Power Autobuild-Date(master): Tue Jan 30 17:22:37 UTC 2024 on atb-devel-224 (cherry picked from commit 9b2f2302ee4828ae54f5903a3bf649ffd255fb4a) --- source3/rpc_client/wsp_cli.c | 64 ++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/source3/rpc_client/wsp_cli.c b/source3/rpc_client/wsp_cli.c index 7fd221dd731..d8a9aca46ff 100644 --- a/source3/rpc_client/wsp_cli.c +++ b/source3/rpc_client/wsp_cli.c @@ -894,7 +894,7 @@ static bool convert_variant_array_to_vector(TALLOC_CTX *ctx, * an array of n elements for a vector or array of 1 element * if non-vector item. * - * addresses stored in pvec_address are adjusted by offset + * addresses stored in pvec_address * */ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, @@ -902,7 +902,7 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint64_t offset, + uint64_t baseaddress, DATA_BLOB *rows_buf, uint64_t *pcount, uint64_t **pvec_address) @@ -957,12 +957,10 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, addr = addr_32; } - addr = addr - offset; - - if (addr >= rows_buf->length) { + if ((addr - baseaddress) >= rows_buf->length) { DBG_ERR("offset %"PRIu64" outside buffer range " "(buf len - %zu)\n", - addr, + addr - baseaddress, rows_buf->length); err = NDR_ERR_VALIDATE; goto out; @@ -979,22 +977,27 @@ static enum ndr_err_code extract_variant_addresses(TALLOC_CTX *ctx, if (is_vector == false) { vec_address[0] = addr; } else { - uint64_t array_addr = addr; + uint64_t array_offset = addr - baseaddress; uint64_t i; for (i = 0; i < count; i++) { if (is_64bit) { vec_address[i] = PULL_LE_I64(rows_buf->data, - array_addr); - array_addr = array_addr + 8; + array_offset); + array_offset = array_offset + 8; } else { vec_address[i] = (uint32_t)PULL_LE_I32(rows_buf->data, - array_addr); - array_addr = array_addr + 4; + 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; } - /* adjust address */ - vec_address[i] -= offset; } } err = NDR_ERR_SUCCESS; @@ -1009,7 +1012,7 @@ static enum ndr_err_code extract_crowvariant_variable(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint64_t offset, + uint64_t baseaddress, DATA_BLOB *rows_buf, uint32_t len, struct wsp_cbasestoragevariant *val) @@ -1028,7 +1031,7 @@ static enum ndr_err_code extract_crowvariant_variable(TALLOC_CTX *ctx, is_64bit, ndr_pull, flags, - offset, + baseaddress, rows_buf, &count, &vec_address); @@ -1062,12 +1065,12 @@ static enum ndr_err_code extract_crowvariant_variable(TALLOC_CTX *ctx, for (i = 0; i < count; i++) { uint32_t tmplen = len; - uint64_t addr; - addr = vec_address[i]; - if (addr >= rows_buf->length) { + uint64_t buf_offset; + buf_offset = vec_address[i] - baseaddress; + if (buf_offset >= rows_buf->length) { DBG_ERR("offset %"PRIu64" outside buffer range " "(buf len - %zu)\n", - addr, + buf_offset, rows_buf->length); err = NDR_ERR_VALIDATE; goto out; @@ -1083,11 +1086,11 @@ static enum ndr_err_code extract_crowvariant_variable(TALLOC_CTX *ctx, * from the point the value is stored at * till the end of the buffer */ - tmplen = rows_buf->length - addr; + tmplen = rows_buf->length - buf_offset; } if (!extract_rowbuf_variable_type(ctx, tablevar->vtype & ~VT_VECTOR, - addr, + buf_offset, rows_buf, tmplen, variant_array[i])) { @@ -1115,7 +1118,7 @@ static enum ndr_err_code extract_crowvariant(TALLOC_CTX *ctx, bool is_64bit, struct ndr_pull *ndr_pull, ndr_flags_type flags, - uint64_t offset, + uint64_t baseaddress, DATA_BLOB *rows_buf, uint32_t len, struct wsp_cbasestoragevariant *val) { @@ -1135,7 +1138,7 @@ static enum ndr_err_code extract_crowvariant(TALLOC_CTX *ctx, is_64bit, ndr_pull, flags, - offset, + baseaddress, rows_buf, len, val); @@ -1158,7 +1161,6 @@ out: static enum ndr_err_code process_columns(TALLOC_CTX *ctx, bool is_64bit, - uint32_t cbreserved, uint64_t baseaddress, struct wsp_cpmsetbindingsin *bindingin, DATA_BLOB *rows_buf, @@ -1224,7 +1226,6 @@ static enum ndr_err_code process_columns(TALLOC_CTX *ctx, val_offset)); } if (tab_col->valueused) { - uint64_t offset = baseaddress + cbreserved; uint32_t len = 0; val_offset = nrow_offset + tab_col->valueoffset.value; if (val_offset >= rows_buf->length) { @@ -1284,7 +1285,7 @@ static enum ndr_err_code process_columns(TALLOC_CTX *ctx, is_64bit, ndr_pull, ndr_flags, - offset, + baseaddress, rows_buf, len, &cols[i]); @@ -1316,13 +1317,20 @@ enum ndr_err_code extract_rowsarray( talloc_zero_array(ctx, struct wsp_cbasestoragevariant, bindingsin->ccolumns); + uint64_t adjusted_address; if (cols == NULL) { return NDR_ERR_ALLOC; } + + /* + * cater for paddingrows (see MS-WSP 2.2.3.12) + * Rows buffer starts cbreserved bytes into messages + */ + adjusted_address = baseaddress + cbreserved; + err = process_columns(ctx, is_64bit, - cbreserved, - baseaddress, + adjusted_address, bindingsin, rows_buf, i, -- 2.35.3 From cff8f34d82839736bc62b2f99af3a96f4aa84848 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 14 Feb 2024 12:01:28 +0000 Subject: [PATCH 5/7] 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 4218c85fb2de8b1546a042129d6fc7b4df48d663 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Wed, 14 Feb 2024 11:19:39 +0000 Subject: [PATCH 6/7] 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 d8a9aca46ff..39d1f7868d0 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 4ff7950a898961354203f1d50f9693e60afdffaf Mon Sep 17 00:00:00 2001 From: Noel Power Date: Thu, 8 Feb 2024 14:05:43 +0000 Subject: [PATCH 7/7] 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) --- 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; -- 2.35.3