Attachment 5506 Details for Bug 7258 – patch for 3.5

[patch] patch for 3.5

winreg_QueryValue-v3-5-test.patch (text/plain), 14.28 KB, created by Guenther Deschner on 2010-03-17 06:11:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Guenther Deschner

Created: 2010-03-17 06:11:01 UTC

Size: 14.28 KB

patch

obsolete

>From 62af588c2de83fa0d6127838116fd1ec6c002e44 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
>Date: Fri, 5 Mar 2010 21:56:50 +0100
>Subject: [PATCH 1/4] winreg: fix winreg_QueryValue IDL.
>
>Note that before this change pidl generated code that just dereferenced size_is
>and length_is values from unique pointers without checking whether these
>pointers were actually NULL.
>
>With this change, pidl now throws a warning like:
>
>	warning: Got pointer for `data_size', expected fully derefenced variable
>
>which is not correct, probably because pidl does not evaluate the C expression.
>
>Guenther
>(cherry picked from commit f258e98e177f0f75bab99654b9f32b10bb7ce37f)
>---
> librpc/idl/winreg.idl |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/librpc/idl/winreg.idl b/librpc/idl/winreg.idl
>index f1f4dfb..7020691 100644
>--- a/librpc/idl/winreg.idl
>+++ b/librpc/idl/winreg.idl
>@@ -256,7 +256,7 @@ import "lsa.idl", "security.idl", "misc.idl";
> 		[in,ref] policy_handle *handle,
> 		[in,ref] winreg_String *value_name,
> 		[in,out,unique] winreg_Type *type,
>-		[in,out,unique,size_is(*data_size),length_is(*data_length)] uint8 *data,
>+		[in,out,unique,size_is(data_size ? *data_size : 0),length_is(data_length ? *data_length : 0),range(0,0x4000000)] uint8 *data,
> 		[in,out,unique] uint32 *data_size,
> 		[in,out,unique] uint32 *data_length
> 	);
>-- 
>1.6.6.1
>
>
>From 162cc341409cdf75717e63c3f4e8303d7f274c18 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
>Date: Wed, 17 Mar 2010 12:06:39 +0100
>Subject: [PATCH 2/4] s3: re-run make samba3-idl.
>
>Guenther
>---
> librpc/gen_ndr/cli_winreg.c |    8 ++++----
> librpc/gen_ndr/cli_winreg.h |    4 ++--
> librpc/gen_ndr/ndr_winreg.c |   24 ++++++++++++------------
> librpc/gen_ndr/winreg.h     |    4 ++--
> 4 files changed, 20 insertions(+), 20 deletions(-)
>
>diff --git a/librpc/gen_ndr/cli_winreg.c b/librpc/gen_ndr/cli_winreg.c
>index 4264542..1c37f51 100644
>--- a/librpc/gen_ndr/cli_winreg.c
>+++ b/librpc/gen_ndr/cli_winreg.c
>@@ -2743,7 +2743,7 @@ struct tevent_req *rpccli_winreg_QueryValue_send(TALLOC_CTX *mem_ctx,
> 						 struct policy_handle *_handle /* [in] [ref] */,
> 						 struct winreg_String *_value_name /* [in] [ref] */,
> 						 enum winreg_Type *_type /* [in,out] [unique] */,
>-						 uint8_t *_data /* [in,out] [unique,length_is(*data_length),size_is(*data_size)] */,
>+						 uint8_t *_data /* [in,out] [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */,
> 						 uint32_t *_data_size /* [in,out] [unique] */,
> 						 uint32_t *_data_length /* [in,out] [unique] */)
> {
>@@ -2823,7 +2823,7 @@ static void rpccli_winreg_QueryValue_done(struct tevent_req *subreq)
> 		*state->orig.out.type = *state->tmp.out.type;
> 	}
> 	if (state->orig.out.data && state->tmp.out.data) {
>-		memcpy(state->orig.out.data, state->tmp.out.data, (*state->tmp.in.data_size) * sizeof(*state->orig.out.data));
>+		memcpy(state->orig.out.data, state->tmp.out.data, (state->tmp.in.data_size?*state->tmp.in.data_size:0) * sizeof(*state->orig.out.data));
> 	}
> 	if (state->orig.out.data_size && state->tmp.out.data_size) {
> 		*state->orig.out.data_size = *state->tmp.out.data_size;
>@@ -2869,7 +2869,7 @@ NTSTATUS rpccli_winreg_QueryValue(struct rpc_pipe_client *cli,
> 				  struct policy_handle *handle /* [in] [ref] */,
> 				  struct winreg_String *value_name /* [in] [ref] */,
> 				  enum winreg_Type *type /* [in,out] [unique] */,
>-				  uint8_t *data /* [in,out] [unique,length_is(*data_length),size_is(*data_size)] */,
>+				  uint8_t *data /* [in,out] [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */,
> 				  uint32_t *data_size /* [in,out] [unique] */,
> 				  uint32_t *data_length /* [in,out] [unique] */,
> 				  WERROR *werror)
>@@ -2904,7 +2904,7 @@ NTSTATUS rpccli_winreg_QueryValue(struct rpc_pipe_client *cli,
> 		*type = *r.out.type;
> 	}
> 	if (data && r.out.data) {
>-		memcpy(data, r.out.data, (*r.in.data_size) * sizeof(*data));
>+		memcpy(data, r.out.data, (r.in.data_size?*r.in.data_size:0) * sizeof(*data));
> 	}
> 	if (data_size && r.out.data_size) {
> 		*data_size = *r.out.data_size;
>diff --git a/librpc/gen_ndr/cli_winreg.h b/librpc/gen_ndr/cli_winreg.h
>index 2d7ac0c..bbd9e9a 100644
>--- a/librpc/gen_ndr/cli_winreg.h
>+++ b/librpc/gen_ndr/cli_winreg.h
>@@ -298,7 +298,7 @@ struct tevent_req *rpccli_winreg_QueryValue_send(TALLOC_CTX *mem_ctx,
> 						 struct policy_handle *_handle /* [in] [ref] */,
> 						 struct winreg_String *_value_name /* [in] [ref] */,
> 						 enum winreg_Type *_type /* [in,out] [unique] */,
>-						 uint8_t *_data /* [in,out] [unique,length_is(*data_length),size_is(*data_size)] */,
>+						 uint8_t *_data /* [in,out] [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */,
> 						 uint32_t *_data_size /* [in,out] [unique] */,
> 						 uint32_t *_data_length /* [in,out] [unique] */);
> NTSTATUS rpccli_winreg_QueryValue_recv(struct tevent_req *req,
>@@ -309,7 +309,7 @@ NTSTATUS rpccli_winreg_QueryValue(struct rpc_pipe_client *cli,
> 				  struct policy_handle *handle /* [in] [ref] */,
> 				  struct winreg_String *value_name /* [in] [ref] */,
> 				  enum winreg_Type *type /* [in,out] [unique] */,
>-				  uint8_t *data /* [in,out] [unique,length_is(*data_length),size_is(*data_size)] */,
>+				  uint8_t *data /* [in,out] [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */,
> 				  uint32_t *data_size /* [in,out] [unique] */,
> 				  uint32_t *data_length /* [in,out] [unique] */,
> 				  WERROR *werror);
>diff --git a/librpc/gen_ndr/ndr_winreg.c b/librpc/gen_ndr/ndr_winreg.c
>index b2147ef..6f432d3 100644
>--- a/librpc/gen_ndr/ndr_winreg.c
>+++ b/librpc/gen_ndr/ndr_winreg.c
>@@ -2484,10 +2484,10 @@ _PUBLIC_ enum ndr_err_code ndr_push_winreg_QueryValue(struct ndr_push *ndr, int
> 		}
> 		NDR_CHECK(ndr_push_unique_ptr(ndr, r->in.data));
> 		if (r->in.data) {
>-			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, *r->in.data_size));
>+			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, r->in.data_size?*r->in.data_size:0));
> 			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, 0));
>-			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, *r->in.data_length));
>-			NDR_CHECK(ndr_push_array_uint8(ndr, NDR_SCALARS, r->in.data, *r->in.data_length));
>+			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, r->in.data_length?*r->in.data_length:0));
>+			NDR_CHECK(ndr_push_array_uint8(ndr, NDR_SCALARS, r->in.data, r->in.data_length?*r->in.data_length:0));
> 		}
> 		NDR_CHECK(ndr_push_unique_ptr(ndr, r->in.data_size));
> 		if (r->in.data_size) {
>@@ -2505,10 +2505,10 @@ _PUBLIC_ enum ndr_err_code ndr_push_winreg_QueryValue(struct ndr_push *ndr, int
> 		}
> 		NDR_CHECK(ndr_push_unique_ptr(ndr, r->out.data));
> 		if (r->out.data) {
>-			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, *r->out.data_size));
>+			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, r->out.data_size?*r->out.data_size:0));
> 			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, 0));
>-			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, *r->out.data_length));
>-			NDR_CHECK(ndr_push_array_uint8(ndr, NDR_SCALARS, r->out.data, *r->out.data_length));
>+			NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, r->out.data_length?*r->out.data_length:0));
>+			NDR_CHECK(ndr_push_array_uint8(ndr, NDR_SCALARS, r->out.data, r->out.data_length?*r->out.data_length:0));
> 		}
> 		NDR_CHECK(ndr_push_unique_ptr(ndr, r->out.data_size));
> 		if (r->out.data_size) {
>@@ -2608,11 +2608,11 @@ _PUBLIC_ enum ndr_err_code ndr_pull_winreg_QueryValue(struct ndr_pull *ndr, int
> 		}
> 		if (r->in.data) {
> 			if (r->in.data_size == NULL) return ndr_pull_error(ndr, NDR_ERR_INVALID_POINTER, "NULL Pointer for size_is()");
>-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->in.data, *r->in.data_size));
>+			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->in.data, r->in.data_size?*r->in.data_size:0));
> 		}
> 		if (r->in.data) {
> 			if (r->in.data_length == NULL) return ndr_pull_error(ndr, NDR_ERR_INVALID_POINTER, "NULL Pointer for length_is()");
>-			NDR_CHECK(ndr_check_array_length(ndr, (void*)&r->in.data, *r->in.data_length));
>+			NDR_CHECK(ndr_check_array_length(ndr, (void*)&r->in.data, r->in.data_length?*r->in.data_length:0));
> 		}
> 	}
> 	if (flags & NDR_OUT) {
>@@ -2673,11 +2673,11 @@ _PUBLIC_ enum ndr_err_code ndr_pull_winreg_QueryValue(struct ndr_pull *ndr, int
> 		NDR_CHECK(ndr_pull_WERROR(ndr, NDR_SCALARS, &r->out.result));
> 		if (r->out.data) {
> 			if (r->out.data_size == NULL) return ndr_pull_error(ndr, NDR_ERR_INVALID_POINTER, "NULL Pointer for size_is()");
>-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->out.data, *r->out.data_size));
>+			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->out.data, r->out.data_size?*r->out.data_size:0));
> 		}
> 		if (r->out.data) {
> 			if (r->out.data_length == NULL) return ndr_pull_error(ndr, NDR_ERR_INVALID_POINTER, "NULL Pointer for length_is()");
>-			NDR_CHECK(ndr_check_array_length(ndr, (void*)&r->out.data, *r->out.data_length));
>+			NDR_CHECK(ndr_check_array_length(ndr, (void*)&r->out.data, r->out.data_length?*r->out.data_length:0));
> 		}
> 	}
> 	return NDR_ERR_SUCCESS;
>@@ -2711,7 +2711,7 @@ _PUBLIC_ void ndr_print_winreg_QueryValue(struct ndr_print *ndr, const char *nam
> 		ndr->depth++;
> 		if (r->in.data) {
> 			if (r->in.data_length == NULL) return;
>-			ndr_print_array_uint8(ndr, "data", r->in.data, *r->in.data_length);
>+			ndr_print_array_uint8(ndr, "data", r->in.data, r->in.data_length?*r->in.data_length:0);
> 		}
> 		ndr->depth--;
> 		ndr_print_ptr(ndr, "data_size", r->in.data_size);
>@@ -2741,7 +2741,7 @@ _PUBLIC_ void ndr_print_winreg_QueryValue(struct ndr_print *ndr, const char *nam
> 		ndr->depth++;
> 		if (r->out.data) {
> 			if (r->out.data_length == NULL) return;
>-			ndr_print_array_uint8(ndr, "data", r->out.data, *r->out.data_length);
>+			ndr_print_array_uint8(ndr, "data", r->out.data, r->out.data_length?*r->out.data_length:0);
> 		}
> 		ndr->depth--;
> 		ndr_print_ptr(ndr, "data_size", r->out.data_size);
>diff --git a/librpc/gen_ndr/winreg.h b/librpc/gen_ndr/winreg.h
>index 7116810..53d9a35 100644
>--- a/librpc/gen_ndr/winreg.h
>+++ b/librpc/gen_ndr/winreg.h
>@@ -362,14 +362,14 @@ struct winreg_QueryValue {
> 		struct policy_handle *handle;/* [ref] */
> 		struct winreg_String *value_name;/* [ref] */
> 		enum winreg_Type *type;/* [unique] */
>-		uint8_t *data;/* [unique,length_is(*data_length),size_is(*data_size)] */
>+		uint8_t *data;/* [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */
> 		uint32_t *data_size;/* [unique] */
> 		uint32_t *data_length;/* [unique] */
> 	} in;
> 
> 	struct {
> 		enum winreg_Type *type;/* [unique] */
>-		uint8_t *data;/* [unique,length_is(*data_length),size_is(*data_size)] */
>+		uint8_t *data;/* [unique,range(0,0x4000000),length_is(data_length?*data_length:0),size_is(data_size?*data_size:0)] */
> 		uint32_t *data_size;/* [unique] */
> 		uint32_t *data_length;/* [unique] */
> 		WERROR result;
>-- 
>1.6.6.1
>
>
>From ff13e603a9ba427505893ccd9d2931133451b32b Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
>Date: Wed, 10 Mar 2010 14:17:23 +0100
>Subject: [PATCH 3/4] s3-winreg: add some debug statements to _winreg_QueryValue().
>
>Guenther
>(cherry picked from commit c5ba525748fdab6b182e35673983719b7c235127)
>---
> source3/rpc_server/srv_winreg_nt.c |    8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/source3/rpc_server/srv_winreg_nt.c b/source3/rpc_server/srv_winreg_nt.c
>index dcfe2b9..15c79be 100644
>--- a/source3/rpc_server/srv_winreg_nt.c
>+++ b/source3/rpc_server/srv_winreg_nt.c
>@@ -236,8 +236,8 @@ WERROR _winreg_QueryValue(pipes_struct *p, struct winreg_QueryValue *r)
> 
> 	*r->out.data_length = *r->out.type = REG_NONE;
> 
>-	DEBUG(7,("_reg_info: policy key name = [%s]\n", regkey->key->name));
>-	DEBUG(7,("_reg_info: policy key type = [%08x]\n", regkey->key->type));
>+	DEBUG(7,("_winreg_QueryValue: policy key name = [%s]\n", regkey->key->name));
>+	DEBUG(7,("_winreg_QueryValue: policy key type = [%08x]\n", regkey->key->type));
> 
> 	/* Handle QueryValue calls on HKEY_PERFORMANCE_DATA */
> 	if(regkey->key->type == REG_KEY_HKPD)
>@@ -287,6 +287,10 @@ WERROR _winreg_QueryValue(pipes_struct *p, struct winreg_QueryValue *r)
> 		status = reg_queryvalue(p->mem_ctx, regkey, r->in.value_name->name,
> 					&val);
> 		if (!W_ERROR_IS_OK(status)) {
>+
>+			DEBUG(10,("_winreg_QueryValue: reg_queryvalue failed with: %s\n",
>+				win_errstr(status)));
>+
> 			if (r->out.data_size) {
> 				*r->out.data_size = 0;
> 			}
>-- 
>1.6.6.1
>
>
>From ca6546e2962438cbc85ee04872c72c996f2b593f Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <gd@samba.org>
>Date: Thu, 11 Mar 2010 12:21:08 +0100
>Subject: [PATCH 4/4] s3-winreg: Fix _winreg_QueryValue crash bugs and implement windows behavior.
>
>Found by RPC-WINREG smbtorture test.
>
>Guenther
>(cherry picked from commit cddc542ba5f30316ff4ee8fa591a54808b7be4c8)
>---
> source3/rpc_server/srv_winreg_nt.c |   19 ++++++++-----------
> 1 files changed, 8 insertions(+), 11 deletions(-)
>
>diff --git a/source3/rpc_server/srv_winreg_nt.c b/source3/rpc_server/srv_winreg_nt.c
>index 15c79be..5912322 100644
>--- a/source3/rpc_server/srv_winreg_nt.c
>+++ b/source3/rpc_server/srv_winreg_nt.c
>@@ -230,12 +230,10 @@ WERROR _winreg_QueryValue(pipes_struct *p, struct winreg_QueryValue *r)
> 	if ( !regkey )
> 		return WERR_BADFID;
> 
>-	if ((r->out.data_length == NULL) || (r->out.type == NULL)) {
>+	if ((r->out.data_length == NULL) || (r->out.type == NULL) || (r->out.data_size == NULL)) {
> 		return WERR_INVALID_PARAM;
> 	}
> 
>-	*r->out.data_length = *r->out.type = REG_NONE;
>-
> 	DEBUG(7,("_winreg_QueryValue: policy key name = [%s]\n", regkey->key->name));
> 	DEBUG(7,("_winreg_QueryValue: policy key type = [%08x]\n", regkey->key->type));
> 
>@@ -310,19 +308,18 @@ WERROR _winreg_QueryValue(pipes_struct *p, struct winreg_QueryValue *r)
> 		*r->out.type = val->type;
> 	}
> 
>-	*r->out.data_length = outbuf_size;
>+	status = WERR_BADFILE;
> 
>-	if ( *r->in.data_size == 0 || !r->out.data ) {
>-		status = WERR_OK;
>-	} else if ( *r->out.data_length > *r->in.data_size ) {
>-		status = WERR_MORE_DATA;
>+	if (*r->in.data_size < outbuf_size) {
>+		*r->out.data_size = outbuf_size;
>+		status = r->in.data ? WERR_MORE_DATA : WERR_OK;
> 	} else {
>-		memcpy( r->out.data, outbuf, *r->out.data_length );
>+		*r->out.data_length = outbuf_size;
>+		*r->out.data_size = outbuf_size;
>+		memcpy(r->out.data, outbuf, outbuf_size);
> 		status = WERR_OK;
> 	}
> 
>-	*r->out.data_size = *r->out.data_length;
>-
> 	if (free_prs) prs_mem_free(&prs_hkpd);
> 	if (free_buf) SAFE_FREE(outbuf);
> 
>-- 
>1.6.6.1
>

Flags:

obnox: review+
jra: review+

Actions: View

Attachments on bug 7258: 5506