From 62af588c2de83fa0d6127838116fd1ec6c002e44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Deschner?= 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?= 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?= 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?= 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