From 596b2b892bb6e1c45fbc0fe97dfd436a3737f3bf Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 18 Nov 2019 10:38:01 +1300 Subject: [PATCH 1/7] libndr: Do not overwrite token list with NULL on allocation failure BUG: https://bugzilla.samba.org/show_bug.cgi?id=13876 Signed-off-by: Andrew Bartlett --- librpc/ndr/ndr.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c index 53f9a816f94..93f47e9b1c6 100644 --- a/librpc/ndr/ndr.c +++ b/librpc/ndr/ndr.c @@ -972,6 +972,7 @@ _PUBLIC_ enum ndr_err_code ndr_token_store(TALLOC_CTX *mem_ctx, NDR_ERR_HAVE_NO_MEMORY(list->tokens); } } else { + struct ndr_token *new_tokens = NULL; uint32_t alloc_count = talloc_array_length(list->tokens); if (list->count == alloc_count) { unsigned new_alloc; @@ -980,11 +981,10 @@ _PUBLIC_ enum ndr_err_code ndr_token_store(TALLOC_CTX *mem_ctx, if (new_alloc < alloc_count) { return NDR_ERR_RANGE; } - list->tokens = talloc_realloc(mem_ctx, list->tokens, - struct ndr_token, new_alloc); - if (list->tokens == NULL) { - NDR_ERR_HAVE_NO_MEMORY(list->tokens); - } + new_tokens = talloc_realloc(mem_ctx, list->tokens, + struct ndr_token, new_alloc); + NDR_ERR_HAVE_NO_MEMORY(new_tokens); + list->tokens = new_tokens; } } list->tokens[list->count].key = key; -- 2.17.1 From f98d040aedaf37e18c0f3a81ed9dfbc59586fd68 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sat, 16 Nov 2019 07:59:58 +1300 Subject: [PATCH 2/7] ndr: Restrict size of ndr_token lists to avoid memory abuse by malicious clients This is designed to stop a very large number of tokens from being stored for arrays of structures containing relative pointers in particular. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13876 Signed-off-by: Andrew Bartlett --- librpc/ndr/ndr.c | 119 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 108 insertions(+), 11 deletions(-) diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c index 93f47e9b1c6..0272509a01f 100644 --- a/librpc/ndr/ndr.c +++ b/librpc/ndr/ndr.c @@ -36,6 +36,17 @@ #define NDR_BASE_MARSHALL_SIZE 1024 +/* + * This value is arbitary, but designed to reduce the memory a client + * can allocate and the work the client can force in processing a + * malicious packet. + * + * In an ideal world this would be controlled by range() restrictions + * on array sizes and careful IDL construction to avoid arbitary + * linked lists, but this is a backstop for now. + */ +#define NDR_TOKEN_MAX_LIST_SIZE 65535 + /* this guid indicates NDR encoding in a protocol tower */ const struct ndr_syntax_id ndr_transfer_syntax_ndr = { { 0x8a885d04, 0x1ceb, 0x11c9, {0x9f, 0xe8}, {0x08,0x00,0x2b,0x10,0x48,0x60} }, @@ -974,8 +985,21 @@ _PUBLIC_ enum ndr_err_code ndr_token_store(TALLOC_CTX *mem_ctx, } else { struct ndr_token *new_tokens = NULL; uint32_t alloc_count = talloc_array_length(list->tokens); + + /* + * Check every time we have not allocated too many + * tokens. This ensures developer sanity when + * debugging the boundary condition + */ + if (list->count >= NDR_TOKEN_MAX_LIST_SIZE) { + return NDR_ERR_RANGE; + } if (list->count == alloc_count) { unsigned new_alloc; + /* + * Double the list, until we start in chunks + * of 1000 + */ unsigned increment = MIN(list->count, 1000); new_alloc = alloc_count + increment; if (new_alloc < alloc_count) { @@ -1059,9 +1083,16 @@ _PUBLIC_ uint32_t ndr_token_peek(struct ndr_token_list *list, const void *key) */ _PUBLIC_ enum ndr_err_code ndr_pull_array_size(struct ndr_pull *ndr, const void *p) { + enum ndr_err_code ret; uint32_t size; NDR_CHECK(ndr_pull_uint3264(ndr, NDR_SCALARS, &size)); - return ndr_token_store(ndr, &ndr->array_size_list, p, size); + ret = ndr_token_store(ndr, &ndr->array_size_list, p, size); + if (ret == NDR_ERR_RANGE) { + return ndr_pull_error(ndr, ret, + "More than %d NDR tokens stored for array_size", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* @@ -1092,6 +1123,7 @@ _PUBLIC_ enum ndr_err_code ndr_check_array_size(struct ndr_pull *ndr, void *p, u */ _PUBLIC_ enum ndr_err_code ndr_pull_array_length(struct ndr_pull *ndr, const void *p) { + enum ndr_err_code ret; uint32_t length, offset; NDR_CHECK(ndr_pull_uint3264(ndr, NDR_SCALARS, &offset)); if (offset != 0) { @@ -1099,7 +1131,13 @@ _PUBLIC_ enum ndr_err_code ndr_pull_array_length(struct ndr_pull *ndr, const voi "non-zero array offset %u\n", offset); } NDR_CHECK(ndr_pull_uint3264(ndr, NDR_SCALARS, &length)); - return ndr_token_store(ndr, &ndr->array_length_list, p, length); + ret = ndr_token_store(ndr, &ndr->array_length_list, p, length); + if (ret == NDR_ERR_RANGE) { + return ndr_pull_error(ndr, ret, + "More than %d NDR tokens stored for array_length_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* @@ -1164,12 +1202,27 @@ _PUBLIC_ enum ndr_err_code ndr_check_pipe_chunk_trailer(struct ndr_pull *ndr, in */ _PUBLIC_ enum ndr_err_code ndr_push_set_switch_value(struct ndr_push *ndr, const void *p, uint32_t val) { - return ndr_token_store(ndr, &ndr->switch_list, p, val); + enum ndr_err_code ret = + ndr_token_store(ndr, &ndr->switch_list, p, val); + if (ret == NDR_ERR_RANGE) { + return ndr_push_error(ndr, ret, + "More than %d NDR tokens stored for switch_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } _PUBLIC_ enum ndr_err_code ndr_pull_set_switch_value(struct ndr_pull *ndr, const void *p, uint32_t val) { - return ndr_token_store(ndr, &ndr->switch_list, p, val); + + enum ndr_err_code ret = + ndr_token_store(ndr, &ndr->switch_list, p, val); + if (ret == NDR_ERR_RANGE) { + return ndr_pull_error(ndr, ret, + "More than %d NDR tokens stored for switch_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } _PUBLIC_ enum ndr_err_code ndr_print_set_switch_value(struct ndr_print *ndr, const void *p, uint32_t val) @@ -1482,8 +1535,15 @@ _PUBLIC_ void ndr_push_restore_relative_base_offset(struct ndr_push *ndr, uint32 */ _PUBLIC_ enum ndr_err_code ndr_push_setup_relative_base_offset1(struct ndr_push *ndr, const void *p, uint32_t offset) { + enum ndr_err_code ret; ndr->relative_base_offset = offset; - return ndr_token_store(ndr, &ndr->relative_base_list, p, offset); + ret = ndr_token_store(ndr, &ndr->relative_base_list, p, offset); + if (ret == NDR_ERR_RANGE) { + return ndr_push_error(ndr, ret, + "More than %d NDR tokens stored for relative_base_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* @@ -1501,12 +1561,19 @@ _PUBLIC_ enum ndr_err_code ndr_push_setup_relative_base_offset2(struct ndr_push */ _PUBLIC_ enum ndr_err_code ndr_push_relative_ptr1(struct ndr_push *ndr, const void *p) { + enum ndr_err_code ret; if (p == NULL) { NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 0)); return NDR_ERR_SUCCESS; } NDR_CHECK(ndr_push_align(ndr, 4)); - NDR_CHECK(ndr_token_store(ndr, &ndr->relative_list, p, ndr->offset)); + ret = ndr_token_store(ndr, &ndr->relative_list, p, ndr->offset); + if (ret == NDR_ERR_RANGE) { + return ndr_push_error(ndr, ret, + "More than %d NDR tokens stored for relative_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + NDR_CHECK(ret); return ndr_push_uint32(ndr, NDR_SCALARS, 0xFFFFFFFF); } @@ -1516,12 +1583,19 @@ _PUBLIC_ enum ndr_err_code ndr_push_relative_ptr1(struct ndr_push *ndr, const vo */ _PUBLIC_ enum ndr_err_code ndr_push_short_relative_ptr1(struct ndr_push *ndr, const void *p) { + enum ndr_err_code ret; if (p == NULL) { NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, 0)); return NDR_ERR_SUCCESS; } NDR_CHECK(ndr_push_align(ndr, 2)); - NDR_CHECK(ndr_token_store(ndr, &ndr->relative_list, p, ndr->offset)); + ret = ndr_token_store(ndr, &ndr->relative_list, p, ndr->offset); + if (ret == NDR_ERR_RANGE) { + return ndr_push_error(ndr, ret, + "More than %d NDR tokens stored for relative_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + NDR_CHECK(ret); return ndr_push_uint16(ndr, NDR_SCALARS, 0xFFFF); } /* @@ -1617,6 +1691,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_short_relative_ptr2(struct ndr_push *ndr, co */ _PUBLIC_ enum ndr_err_code ndr_push_relative_ptr2_start(struct ndr_push *ndr, const void *p) { + enum ndr_err_code ret; if (p == NULL) { return NDR_ERR_SUCCESS; } @@ -1655,8 +1730,16 @@ _PUBLIC_ enum ndr_err_code ndr_push_relative_ptr2_start(struct ndr_push *ndr, co "ndr_push_relative_ptr2_start RELATIVE_REVERSE flag set and relative_end_offset %d", ndr->relative_end_offset); } - NDR_CHECK(ndr_token_store(ndr, &ndr->relative_begin_list, p, ndr->offset)); - return NDR_ERR_SUCCESS; + ret = ndr_token_store(ndr, + &ndr->relative_begin_list, + p, + ndr->offset); + if (ret == NDR_ERR_RANGE) { + return ndr_push_error(ndr, ret, + "More than %d NDR tokens stored for array_size", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* @@ -1786,8 +1869,15 @@ _PUBLIC_ void ndr_pull_restore_relative_base_offset(struct ndr_pull *ndr, uint32 */ _PUBLIC_ enum ndr_err_code ndr_pull_setup_relative_base_offset1(struct ndr_pull *ndr, const void *p, uint32_t offset) { + enum ndr_err_code ret; ndr->relative_base_offset = offset; - return ndr_token_store(ndr, &ndr->relative_base_list, p, offset); + ret = ndr_token_store(ndr, &ndr->relative_base_list, p, offset); + if (ret == NDR_ERR_RANGE) { + return ndr_pull_error(ndr, ret, + "More than %d NDR tokens stored for relative_base_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* @@ -1805,13 +1895,20 @@ _PUBLIC_ enum ndr_err_code ndr_pull_setup_relative_base_offset2(struct ndr_pull */ _PUBLIC_ enum ndr_err_code ndr_pull_relative_ptr1(struct ndr_pull *ndr, const void *p, uint32_t rel_offset) { + enum ndr_err_code ret; rel_offset += ndr->relative_base_offset; if (rel_offset > ndr->data_size) { return ndr_pull_error(ndr, NDR_ERR_BUFSIZE, "ndr_pull_relative_ptr1 rel_offset(%u) > ndr->data_size(%u)", rel_offset, ndr->data_size); } - return ndr_token_store(ndr, &ndr->relative_list, p, rel_offset); + ret = ndr_token_store(ndr, &ndr->relative_list, p, rel_offset); + if (ret == NDR_ERR_RANGE) { + return ndr_pull_error(ndr, ret, + "More than %d NDR tokens stored for relative_list", + NDR_TOKEN_MAX_LIST_SIZE); + } + return ret; } /* -- 2.17.1 From fe9e9f3f8896d937e32205481c8f8487ff57a163 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sat, 16 Nov 2019 09:37:30 +1300 Subject: [PATCH 3/7] s4-libcli/rap: Set the switch_value before NDR_BUFFERS to prepare for new libndr behaviour Signed-off-by: Andrew Bartlett --- source4/libcli/rap/rap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source4/libcli/rap/rap.c b/source4/libcli/rap/rap.c index 9f41e17d278..1b2cadb753d 100644 --- a/source4/libcli/rap/rap.c +++ b/source4/libcli/rap/rap.c @@ -476,6 +476,7 @@ static enum ndr_err_code ndr_pull_rap_NetPrintQEnum_data(struct ndr_pull *ndr, s NDR_CHECK(ndr_pull_rap_printq_info(ndr, NDR_SCALARS, &r->out.info[cntr_info_0])); } for (cntr_info_0 = 0; cntr_info_0 < r->out.count; cntr_info_0++) { + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->out.info[cntr_info_0], r->in.level)); NDR_CHECK(ndr_pull_rap_printq_info(ndr, NDR_BUFFERS, &r->out.info[cntr_info_0])); } NDR_PULL_SET_MEM_CTX(ndr, _mem_save_info_0, 0); @@ -864,6 +865,7 @@ static enum ndr_err_code ndr_pull_rap_NetPrintJobEnum_data(struct ndr_pull *ndr, NDR_CHECK(ndr_pull_rap_printj_info(ndr, NDR_SCALARS, &r->out.info[cntr_info_0])); } for (cntr_info_0 = 0; cntr_info_0 < r->out.count; cntr_info_0++) { + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->out.info[cntr_info_0], r->in.level)); NDR_CHECK(ndr_pull_rap_printj_info(ndr, NDR_BUFFERS, &r->out.info[cntr_info_0])); } NDR_PULL_SET_MEM_CTX(ndr, _mem_save_info_0, 0); @@ -1413,6 +1415,7 @@ static enum ndr_err_code ndr_pull_rap_NetSessionEnum_data(struct ndr_pull *ndr, NDR_CHECK(ndr_pull_rap_session_info(ndr, NDR_SCALARS, &r->out.info[cntr_info_0])); } for (cntr_info_0 = 0; cntr_info_0 < r->out.count; cntr_info_0++) { + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->out.info[cntr_info_0], r->in.level)); NDR_CHECK(ndr_pull_rap_session_info(ndr, NDR_BUFFERS, &r->out.info[cntr_info_0])); } NDR_PULL_SET_MEM_CTX(ndr, _mem_save_info_0, 0); -- 2.17.1 From 683799794fbaf5861fa9c29db346d9b9d244e741 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sun, 17 Nov 2019 19:32:50 +1300 Subject: [PATCH 4/7] negoex: Set the switch_value before NDR_BUFFERS to prepare for new libndr behaviour Signed-off-by: Andrew Bartlett --- librpc/ndr/ndr_negoex.c | 1 + 1 file changed, 1 insertion(+) diff --git a/librpc/ndr/ndr_negoex.c b/librpc/ndr/ndr_negoex.c index b5cb5bc8bcf..95adce5a7e3 100644 --- a/librpc/ndr/ndr_negoex.c +++ b/librpc/ndr/ndr_negoex.c @@ -439,6 +439,7 @@ enum ndr_err_code ndr_pull_negoex_MESSAGE(struct ndr_pull *ndr, int ndr_flags, s NDR_PULL_NEED_BYTES(ndr, r->message_length); ndr->data_size = ndr->offset + r->message_length; ndr->offset = saved_offset; + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->p, r->type)); NDR_CHECK(ndr_pull_negoex_PAYLOAD(ndr, NDR_BUFFERS, &r->p)); ndr->offset = ndr->data_size; ndr->data_size = start_data_size; -- 2.17.1 From 9d44443bc299a47c7afd82f5ec4273618264f5e4 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 15 Nov 2019 16:59:12 +1300 Subject: [PATCH 5/7] pidl: Generate and consume the switch level token for both NDR_SCALARS and NDR_BUFFERS This means what was previously a list becomes a single variable that could be passed as a function paraemter, but this is avoided for now because it would change the ABI and be more intrusive. Before this, a client could cause a NDR token containing the swith level to be allocated for each and every element in the array that they promised they were sending (without having to actually send them). Found by Michael Hanselmann using Honggfuzz and an fuzzer for Samba's NDR layer. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13876 Signed-off-by: Andrew Bartlett --- pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm | 37 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm index c1a2cc99cb7..636bfb05b32 100644 --- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm +++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm @@ -647,8 +647,6 @@ sub ParseElementPushLevel $self->pidl("NDR_CHECK(ndr_push_array_$nl->{DATA_TYPE}($ndr, $ndr_flags, $var_name, $length));"); return; } - } elsif ($l->{TYPE} eq "SWITCH") { - $self->ParseSwitchPush($e, $l, $ndr, $var_name, $env); } elsif ($l->{TYPE} eq "DATA") { $self->ParseDataPush($e, $l, $ndr, $var_name, $primitives, $deferred); } elsif ($l->{TYPE} eq "TYPEDEF") { @@ -709,6 +707,14 @@ sub ParseElementPushLevel $self->pidl("}"); } } elsif ($l->{TYPE} eq "SWITCH") { + my $nl = GetNextLevel($e,$l); + my $needs_deferred_switch = is_deferred_switch_non_empty($nl); + + # Avoid setting a switch value if it will not be + # consumed again in the NDR_BUFFERS pull + if ($needs_deferred_switch or !$deferred) { + $self->ParseSwitchPush($e, $l, $ndr, $var_name, $env); + } $self->ParseElementPushLevel($e, GetNextLevel($e, $l), $ndr, $var_name, $env, $primitives, $deferred); } } @@ -1174,8 +1180,6 @@ sub ParseElementPullLevel } } elsif ($l->{TYPE} eq "POINTER") { $self->ParsePtrPull($e, $l, $ndr, $var_name); - } elsif ($l->{TYPE} eq "SWITCH") { - $self->ParseSwitchPull($e, $l, $ndr, $var_name, $env); } elsif ($l->{TYPE} eq "DATA") { $self->ParseDataPull($e, $l, $ndr, $var_name, $primitives, $deferred); } elsif ($l->{TYPE} eq "TYPEDEF") { @@ -1260,7 +1264,15 @@ sub ParseElementPullLevel $self->ParseMemCtxPullEnd($e, $l, $ndr); } elsif ($l->{TYPE} eq "SWITCH") { - $self->ParseElementPullLevel($e, GetNextLevel($e,$l), $ndr, $var_name, $env, $primitives, $deferred); + my $nl = GetNextLevel($e,$l); + my $needs_deferred_switch = is_deferred_switch_non_empty($nl); + + # Avoid setting a switch value if it will not be + # consumed again in the NDR_BUFFERS pull + if ($needs_deferred_switch or !$deferred) { + $self->ParseSwitchPull($e, $l, $ndr, $var_name, $env); + } + $self->ParseElementPullLevel($e, $nl, $ndr, $var_name, $env, $primitives, $deferred); } } @@ -2156,20 +2168,21 @@ sub ParseUnionPull($$$$) $self->pidl("NDR_PULL_CHECK_FLAGS(ndr, ndr_flags);"); $self->pidl("if (ndr_flags & NDR_SCALARS) {"); $self->indent; - if (! $needs_deferred_switch) { - $self->pidl("/* This token is not used again */"); - $self->pidl("level = ndr_pull_steal_switch_value($ndr, $varname);"); - } else { - $self->pidl("level = ndr_pull_get_switch_value($ndr, $varname);"); - } + $self->pidl("/* This token is not used again (except perhaps below in the NDR_BUFFERS case) */"); + $self->pidl("level = ndr_pull_steal_switch_value($ndr, $varname);"); $self->ParseUnionPullPrimitives($e,$ndr,$varname,$switch_type); $self->deindent; $self->pidl("}"); if ($needs_deferred_switch) { $self->pidl("if (ndr_flags & NDR_BUFFERS) {"); $self->indent; - $self->pidl("/* The token is not needed after this. */"); + # In case we had ndr_flags of NDR_SCALERS|NDR_BUFFERS + $self->pidl("if (!(ndr_flags & NDR_SCALARS)) {"); + $self->indent; + $self->pidl("/* We didn't get it above, and the token is not needed after this. */"); $self->pidl("level = ndr_pull_steal_switch_value($ndr, $varname);"); + $self->deindent; + $self->pidl("}"); $self->ParseUnionPullDeferred($e,$ndr,$varname); $self->deindent; $self->pidl("}"); -- 2.17.1 From cfdbd4041623a7accd3f8f6e9894b6748551c498 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 18 Nov 2019 12:02:03 +1300 Subject: [PATCH 6/7] pidl: Add and use ndr_print_steal_switch_value(), removing ndr_print_get_switch_value() This avoids really long token lists for switch values that will not be needed past this point. The function name is changed to clarify what exactly is being done here, and the old function is removed to ensure it is not being used anywhere else. Merge the removal of ndr_print_get_switch_value into just-tagged librpc/ABI/ndr-1.0.0.sigs as this has not been put into any release yet. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13876 Signed-off-by: Andrew Bartlett --- librpc/ABI/ndr-1.0.0.sigs | 2 +- librpc/ndr/libndr.h | 2 +- librpc/ndr/ndr.c | 13 +++++++++++-- librpc/ndr/ndr_drsuapi.c | 2 +- librpc/ndr/ndr_ntlmssp.c | 2 +- librpc/ndr/ndr_schannel.c | 4 ++-- librpc/ndr/ndr_spoolss_buf.c | 2 +- pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm | 2 +- 8 files changed, 19 insertions(+), 10 deletions(-) diff --git a/librpc/ABI/ndr-1.0.0.sigs b/librpc/ABI/ndr-1.0.0.sigs index ffb3d0763fc..7546f63ed8e 100644 --- a/librpc/ABI/ndr-1.0.0.sigs +++ b/librpc/ABI/ndr-1.0.0.sigs @@ -49,7 +49,6 @@ ndr_print_double: void (struct ndr_print *, const char *, double) ndr_print_enum: void (struct ndr_print *, const char *, const char *, const char *, uint32_t) ndr_print_function_debug: void (ndr_print_function_t, const char *, int, void *) ndr_print_function_string: char *(TALLOC_CTX *, ndr_print_function_t, const char *, int, void *) -ndr_print_get_switch_value: uint32_t (struct ndr_print *, const void *) ndr_print_gid_t: void (struct ndr_print *, const char *, gid_t) ndr_print_hyper: void (struct ndr_print *, const char *, uint64_t) ndr_print_int16: void (struct ndr_print *, const char *, int16_t) @@ -68,6 +67,7 @@ ndr_print_printf_helper: void (struct ndr_print *, const char *, ...) ndr_print_ptr: void (struct ndr_print *, const char *, const void *) ndr_print_set_switch_value: enum ndr_err_code (struct ndr_print *, const void *, uint32_t) ndr_print_sockaddr_storage: void (struct ndr_print *, const char *, const struct sockaddr_storage *) +ndr_print_steal_switch_value: uint32_t (struct ndr_print *, const void *) ndr_print_string: void (struct ndr_print *, const char *, const char *) ndr_print_string_array: void (struct ndr_print *, const char *, const char **) ndr_print_string_helper: void (struct ndr_print *, const char *, ...) diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h index 392ab76cc94..128b363ddf3 100644 --- a/librpc/ndr/libndr.h +++ b/librpc/ndr/libndr.h @@ -618,7 +618,7 @@ enum ndr_err_code ndr_pull_set_switch_value(struct ndr_pull *ndr, const void *p, enum ndr_err_code ndr_print_set_switch_value(struct ndr_print *ndr, const void *p, uint32_t val); uint32_t ndr_push_get_switch_value(struct ndr_push *ndr, const void *p); uint32_t ndr_pull_get_switch_value(struct ndr_pull *ndr, const void *p); -uint32_t ndr_print_get_switch_value(struct ndr_print *ndr, const void *p); +uint32_t ndr_print_steal_switch_value(struct ndr_print *ndr, const void *p); uint32_t ndr_pull_steal_switch_value(struct ndr_pull *ndr, const void *p); enum ndr_err_code ndr_pull_struct_blob(const DATA_BLOB *blob, TALLOC_CTX *mem_ctx, void *p, ndr_pull_flags_fn_t fn); enum ndr_err_code ndr_pull_struct_blob_all(const DATA_BLOB *blob, TALLOC_CTX *mem_ctx, void *p, ndr_pull_flags_fn_t fn); diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c index 0272509a01f..1f5df4e2b27 100644 --- a/librpc/ndr/ndr.c +++ b/librpc/ndr/ndr.c @@ -1243,9 +1243,18 @@ _PUBLIC_ uint32_t ndr_pull_get_switch_value(struct ndr_pull *ndr, const void *p) return ndr_token_peek(&ndr->switch_list, p); } -_PUBLIC_ uint32_t ndr_print_get_switch_value(struct ndr_print *ndr, const void *p) +/* retrieve a switch value and remove it from the list */ +_PUBLIC_ uint32_t ndr_print_steal_switch_value(struct ndr_print *ndr, const void *p) { - return ndr_token_peek(&ndr->switch_list, p); + enum ndr_err_code status; + uint32_t v; + + status = ndr_token_retrieve(&ndr->switch_list, p, &v); + if (!NDR_ERR_CODE_IS_SUCCESS(status)) { + return 0; + } + + return v; } /* retrieve a switch value and remove it from the list */ diff --git a/librpc/ndr/ndr_drsuapi.c b/librpc/ndr/ndr_drsuapi.c index 45d3ac095c3..bcfd5145ba2 100644 --- a/librpc/ndr/ndr_drsuapi.c +++ b/librpc/ndr/ndr_drsuapi.c @@ -571,7 +571,7 @@ enum ndr_err_code ndr_pull_drsuapi_DsBindInfo(struct ndr_pull *ndr, int ndr_flag _PUBLIC_ void ndr_print_drsuapi_DsBindInfo(struct ndr_print *ndr, const char *name, const union drsuapi_DsBindInfo *r) { uint32_t level; - level = ndr_print_get_switch_value(ndr, r); + level = ndr_print_steal_switch_value(ndr, r); ndr_print_union(ndr, name, level, "drsuapi_DsBindInfo"); switch (level) { case 24: diff --git a/librpc/ndr/ndr_ntlmssp.c b/librpc/ndr/ndr_ntlmssp.c index 7027ac0b13d..021bc402a80 100644 --- a/librpc/ndr/ndr_ntlmssp.c +++ b/librpc/ndr/ndr_ntlmssp.c @@ -164,7 +164,7 @@ _PUBLIC_ void ndr_print_ntlmssp_lm_response(TALLOC_CTX *mem_ctx, _PUBLIC_ void ndr_print_ntlmssp_Version(struct ndr_print *ndr, const char *name, const union ntlmssp_Version *r) { int level; - level = ndr_print_get_switch_value(ndr, r); + level = ndr_print_steal_switch_value(ndr, r); switch (level) { case NTLMSSP_NEGOTIATE_VERSION: ndr_print_ntlmssp_VERSION(ndr, name, &r->version); diff --git a/librpc/ndr/ndr_schannel.c b/librpc/ndr/ndr_schannel.c index 9bbc628a294..6b08a79cab2 100644 --- a/librpc/ndr/ndr_schannel.c +++ b/librpc/ndr/ndr_schannel.c @@ -27,7 +27,7 @@ _PUBLIC_ void ndr_print_NL_AUTH_MESSAGE_BUFFER(struct ndr_print *ndr, const char *name, const union NL_AUTH_MESSAGE_BUFFER *r) { int level; - level = ndr_print_get_switch_value(ndr, r); + level = ndr_print_steal_switch_value(ndr, r); switch (level) { case NL_FLAG_OEM_NETBIOS_DOMAIN_NAME: ndr_print_string(ndr, name, r->a); @@ -58,7 +58,7 @@ _PUBLIC_ void ndr_print_NL_AUTH_MESSAGE_BUFFER(struct ndr_print *ndr, const char _PUBLIC_ void ndr_print_NL_AUTH_MESSAGE_BUFFER_REPLY(struct ndr_print *ndr, const char *name, const union NL_AUTH_MESSAGE_BUFFER_REPLY *r) { int level; - level = ndr_print_get_switch_value(ndr, r); + level = ndr_print_steal_switch_value(ndr, r); switch (level) { case NL_NEGOTIATE_RESPONSE: ndr_print_uint32(ndr, name, r->dummy); diff --git a/librpc/ndr/ndr_spoolss_buf.c b/librpc/ndr/ndr_spoolss_buf.c index 244d692dca8..a57f578fd45 100644 --- a/librpc/ndr/ndr_spoolss_buf.c +++ b/librpc/ndr/ndr_spoolss_buf.c @@ -1096,7 +1096,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_spoolss_DriverInfo101(struct ndr_pull *ndr, void ndr_print_spoolss_Field(struct ndr_print *ndr, const char *name, const union spoolss_Field *r) { int level; - level = ndr_print_get_switch_value(ndr, r); + level = ndr_print_steal_switch_value(ndr, r); ndr_print_union(ndr, name, level, "spoolss_Field"); switch (level) { case PRINTER_NOTIFY_TYPE: diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm index 636bfb05b32..fd872cf077f 100644 --- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm +++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm @@ -2017,7 +2017,7 @@ sub ParseUnionPrint($$$$$) $self->start_flags($e, $ndr); - $self->pidl("level = ndr_print_get_switch_value($ndr, $varname);"); + $self->pidl("level = ndr_print_steal_switch_value($ndr, $varname);"); $self->pidl("ndr_print_union($ndr, name, level, \"$name\");"); -- 2.17.1 From 0da2e06b90af269bef809e20d4956fc6b8c0075b Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 15 Nov 2019 20:04:41 +1300 Subject: [PATCH 7/7] pidl: Mismatch between set and get of relative base pointers The set was within the switch, the get was before the switch. The difference is shown when there is an empty default element. Signed-off-by: Andrew Bartlett --- pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm index fd872cf077f..d2d3d674903 100644 --- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm +++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm @@ -2107,11 +2107,6 @@ sub ParseUnionPullDeferred($$$$) my ($self,$e,$ndr,$varname) = @_; my $have_default = 0; - if (defined($e->{PROPERTIES}{relative_base})) { - # retrieve the current offset as base for relative pointers - # based on the toplevel struct/union - $self->pidl("NDR_CHECK(ndr_pull_setup_relative_base_offset2($ndr, $varname));"); - } $self->pidl("switch (level) {"); $self->indent; foreach my $el (@{$e->{ELEMENTS}}) { @@ -2122,6 +2117,11 @@ sub ParseUnionPullDeferred($$$$) $self->pidl("$el->{CASE}:"); if ($el->{TYPE} ne "EMPTY") { $self->indent; + if (defined($e->{PROPERTIES}{relative_base})) { + # retrieve the current offset as base for relative pointers + # based on the toplevel struct/union + $self->pidl("NDR_CHECK(ndr_pull_setup_relative_base_offset2($ndr, $varname));"); + } $self->ParseElementPull($el, $ndr, {$el->{NAME} => "$varname->$el->{NAME}"}, 0, 1); $self->deindent; } -- 2.17.1