From 6081c357cdcf03b947511c7360a300deea3e0455 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 12:02:08 +0100 Subject: [PATCH 01/11] pidl:Python: generate nicer code for PyNdrRpcMethodDef arrays BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit fa6d0fd1b00e4836215b4d80f1a9f527db82e01a) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index c09ee4b784d4..53dcf142de6d 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -1417,9 +1417,23 @@ sub Interface($$$) $self->indent; foreach my $d (@fns) { my ($infn, $outfn, $callfn, $prettyname, $docstring, $opnum) = @$d; - $self->pidl("{ \"$prettyname\", $docstring, (py_dcerpc_call_fn)$callfn, (py_data_pack_fn)$infn, (py_data_unpack_fn)$outfn, $opnum, &ndr_table_$interface->{NAME} },"); + $self->pidl("{"); + $self->indent; + $self->pidl(".name = \"$prettyname\","); + $self->pidl(".doc = $docstring,"); + $self->pidl(".call = (py_dcerpc_call_fn)$callfn,"); + $self->pidl(".pack_in_data = (py_data_pack_fn)$infn,"); + $self->pidl(".unpack_out_data = (py_data_unpack_fn)$outfn,"); + $self->pidl(".opnum = $opnum,"); + $self->pidl(".table = &ndr_table_$interface->{NAME},"); + $self->deindent; + $self->pidl("},"); } - $self->pidl("{0}"); + $self->pidl("{"); + $self->indent; + $self->pidl(".name = NULL,"); + $self->deindent; + $self->pidl("},"); $self->deindent; $self->pidl("};"); $self->pidl(""); -- 2.34.1 From b6f3f57163c680113b5fe7cd20a17d94abbb6c29 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 10:07:18 +0100 Subject: [PATCH 02/11] pidl:Python: introduce $is_raisable_return helper variable No change in the generated code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 8cdf7af43a365b0545562033f6c51150f2fbb3a4) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index 53dcf142de6d..44c7e70507e1 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -979,8 +979,13 @@ sub PythonFunctionUnpackOut($$$) $result_size++; } - if ($fn->{RETURN_TYPE}) { - $result_size++ unless ($fn->{RETURN_TYPE} eq "WERROR" or $fn->{RETURN_TYPE} eq "NTSTATUS"); + my $is_raisable_return = 0; + if ($fn->{RETURN_TYPE} and ($fn->{RETURN_TYPE} eq "WERROR" or $fn->{RETURN_TYPE} eq "NTSTATUS")) { + $is_raisable_return = 1; + } + + if ($fn->{RETURN_TYPE} and not $is_raisable_return) { + $result_size++; } my $i = 0; -- 2.34.1 From 5482889500cc32370f8a6e8c9f440e5ec9dbdd93 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 10:19:09 +0100 Subject: [PATCH 03/11] pidl:Python: initialize pointers and add 'result' at the end BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 9dfb0ed8d29bd4a9146cf38bd63c4bb55b5faf73) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index 44c7e70507e1..e024b19a19ad 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -970,12 +970,11 @@ sub PythonFunctionUnpackOut($$$) $self->pidl("static PyObject *$outfnname(struct $fn->{NAME} *r)"); $self->pidl("{"); $self->indent; - $self->pidl("PyObject *result;"); foreach my $e (@{$fn->{ELEMENTS}}) { next unless (grep(/out/,@{$e->{DIRECTION}})); next if (($metadata_args->{in}->{$e->{NAME}} and grep(/in/, @{$e->{DIRECTION}})) or ($metadata_args->{out}->{$e->{NAME}}) and grep(/out/, @{$e->{DIRECTION}})); - $self->pidl("PyObject *py_$e->{NAME};"); + $self->pidl("PyObject *py_$e->{NAME} = NULL;"); $result_size++; } @@ -988,7 +987,8 @@ sub PythonFunctionUnpackOut($$$) $result_size++; } - my $i = 0; + $self->pidl("PyObject *result = NULL;"); + $self->pidl(""); if ($result_size > 1) { $self->pidl("result = PyTuple_New($result_size);"); @@ -999,6 +999,8 @@ sub PythonFunctionUnpackOut($$$) $signature .= "None"; } + my $i = 0; + foreach my $e (@{$fn->{ELEMENTS}}) { next if ($metadata_args->{out}->{$e->{NAME}}); my $py_name = "py_$e->{NAME}"; -- 2.34.1 From 23adcc41dc1836147af450d1541544af0b741173 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 10:20:08 +0100 Subject: [PATCH 04/11] pidl:Python: check PyTuple_New() return value BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit f7282c0dffbf30d72051a81d46d831344a9bbcf9) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index e024b19a19ad..efa5fa83e732 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -992,6 +992,12 @@ sub PythonFunctionUnpackOut($$$) if ($result_size > 1) { $self->pidl("result = PyTuple_New($result_size);"); + $self->pidl("if (result == NULL) {"); + $self->indent; + $self->pidl("return NULL;"); + $self->deindent; + $self->pidl("}"); + $self->pidl(""); $signature .= "("; } elsif ($result_size == 0) { $self->pidl("result = Py_None;"); -- 2.34.1 From 23603913ffff985527c6c4d9517d87f93646abd5 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 10:27:16 +0100 Subject: [PATCH 05/11] pidl:Python: separate logic to calculate the signature string BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 13d9231800ea969675f3207cd2c863e433104b4d) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index efa5fa83e732..fb9e73c1f4b0 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -990,6 +990,12 @@ sub PythonFunctionUnpackOut($$$) $self->pidl("PyObject *result = NULL;"); $self->pidl(""); + if ($result_size > 1) { + $signature .= "("; + } elsif ($result_size == 0) { + $signature .= "None"; + } + if ($result_size > 1) { $self->pidl("result = PyTuple_New($result_size);"); $self->pidl("if (result == NULL) {"); @@ -998,11 +1004,9 @@ sub PythonFunctionUnpackOut($$$) $self->deindent; $self->pidl("}"); $self->pidl(""); - $signature .= "("; } elsif ($result_size == 0) { $self->pidl("result = Py_None;"); $self->pidl("Py_INCREF(result);"); - $signature .= "None"; } my $i = 0; @@ -1012,13 +1016,17 @@ sub PythonFunctionUnpackOut($$$) my $py_name = "py_$e->{NAME}"; if (grep(/out/,@{$e->{DIRECTION}})) { $self->ConvertObjectToPython("r", $env, $e, "r->out.$e->{NAME}", $py_name, "return NULL;"); + if ($result_size > 1) { + $signature .= "$e->{NAME}, "; + } else { + $signature .= $e->{NAME}; + } + if ($result_size > 1) { $self->pidl("PyTuple_SetItem(result, $i, $py_name);"); $i++; - $signature .= "$e->{NAME}, "; } else { $self->pidl("result = $py_name;"); - $signature .= $e->{NAME}; } } } -- 2.34.1 From be8a33c059e55fcf0d1b79fc840a5b3fba6f5622 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 14:56:54 +0100 Subject: [PATCH 06/11] pidl:Python: handle NTSTATUS/WERROR exceptions first If we raise an exception we should not leak temporary python objects. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 67c35d6b4ef6b7dbe9db3c52547b25580cd4756c) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index fb9e73c1f4b0..6db26cc04dc7 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -996,6 +996,14 @@ sub PythonFunctionUnpackOut($$$) $signature .= "None"; } + if ($fn->{RETURN_TYPE} and $is_raisable_return) { + if (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "NTSTATUS") { + $self->handle_ntstatus("r->out.result", "NULL", undef); + } elsif (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "WERROR") { + $self->handle_werror("r->out.result", "NULL", undef); + } + } + if ($result_size > 1) { $self->pidl("result = PyTuple_New($result_size);"); $self->pidl("if (result == NULL) {"); @@ -1031,11 +1039,7 @@ sub PythonFunctionUnpackOut($$$) } } - if (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "NTSTATUS") { - $self->handle_ntstatus("r->out.result", "NULL", undef); - } elsif (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "WERROR") { - $self->handle_werror("r->out.result", "NULL", undef); - } elsif (defined($fn->{RETURN_TYPE})) { + if ($fn->{RETURN_TYPE} and not $is_raisable_return) { my $conv = $self->ConvertObjectToPythonData("r", $fn->{RETURN_TYPE}, "r->out.result", $fn); if ($result_size > 1) { $self->pidl("PyTuple_SetItem(result, $i, $conv);"); -- 2.34.1 From 79794375343ab5879bae8c6d4bf6aa0f367abd6c Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 14:56:54 +0100 Subject: [PATCH 07/11] pidl:Python: prepare code to avoid NTSTATUS/WERROR exceptions They are returned as additional result. It means callers can look at all out params, even if the status is an error. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 7ecaf1a779370ef3ecf189e51a5e668329fa24c7) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 99 ++++++++++++++++++++++++---- 1 file changed, 87 insertions(+), 12 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index 6db26cc04dc7..7a415c99c3ab 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -970,6 +970,7 @@ sub PythonFunctionUnpackOut($$$) $self->pidl("static PyObject *$outfnname(struct $fn->{NAME} *r)"); $self->pidl("{"); $self->indent; + $self->pidl("_UNUSED_ bool raise_result_exception = true;"); foreach my $e (@{$fn->{ELEMENTS}}) { next unless (grep(/out/,@{$e->{DIRECTION}})); next if (($metadata_args->{in}->{$e->{NAME}} and grep(/in/, @{$e->{DIRECTION}})) or @@ -987,7 +988,16 @@ sub PythonFunctionUnpackOut($$$) $result_size++; } + my $max_result_size = $result_size; + my $alloc_size = "$result_size"; + if ($fn->{RETURN_TYPE} and $is_raisable_return) { + $max_result_size++; + } $self->pidl("PyObject *result = NULL;"); + if ($max_result_size != $result_size) { + $self->pidl("size_t result_size = $result_size;"); + $alloc_size = "result_size"; + } $self->pidl(""); if ($result_size > 1) { @@ -997,24 +1007,39 @@ sub PythonFunctionUnpackOut($$$) } if ($fn->{RETURN_TYPE} and $is_raisable_return) { + $self->pidl("if (raise_result_exception) {"); + $self->indent; if (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "NTSTATUS") { $self->handle_ntstatus("r->out.result", "NULL", undef); } elsif (defined($fn->{RETURN_TYPE}) and $fn->{RETURN_TYPE} eq "WERROR") { $self->handle_werror("r->out.result", "NULL", undef); } + $self->deindent; + $self->pidl("} else {"); + $self->indent; + $self->pidl("/* $fn->{RETURN_TYPE} will be part of the results */"); + $self->pidl("result_size += 1;"); + $self->deindent; + $self->pidl("}"); + $self->pidl(""); } - if ($result_size > 1) { - $self->pidl("result = PyTuple_New($result_size);"); + if ($max_result_size > 1) { + if ($max_result_size != $result_size) { + $self->pidl("if (result_size > 1) {"); + $self->indent; + } + $self->pidl("result = PyTuple_New($alloc_size);"); $self->pidl("if (result == NULL) {"); $self->indent; $self->pidl("return NULL;"); $self->deindent; $self->pidl("}"); + if ($max_result_size != $result_size) { + $self->deindent; + $self->pidl("}"); + } $self->pidl(""); - } elsif ($result_size == 0) { - $self->pidl("result = Py_None;"); - $self->pidl("Py_INCREF(result);"); } my $i = 0; @@ -1030,23 +1055,59 @@ sub PythonFunctionUnpackOut($$$) $signature .= $e->{NAME}; } - if ($result_size > 1) { + if ($max_result_size > 1) { + if ($max_result_size != $result_size and $result_size == 1) { + $self->pidl("if (result_size > 1) {"); + $self->indent; + } $self->pidl("PyTuple_SetItem(result, $i, $py_name);"); - $i++; - } else { + if ($max_result_size != $result_size and $result_size == 1) { + $self->deindent; + $self->pidl("}"); + } + } + if ($result_size == 1) { + if ($max_result_size != $result_size) { + $self->pidl("if (result_size == 1) {"); + $self->indent; + } $self->pidl("result = $py_name;"); + if ($max_result_size != $result_size) { + $self->deindent; + $self->pidl("}"); + } } + $self->pidl(""); + $i++; } } - if ($fn->{RETURN_TYPE} and not $is_raisable_return) { + if ($fn->{RETURN_TYPE} and $is_raisable_return) { + $self->pidl("if (!raise_result_exception) {"); + $self->indent; + } + + if ($fn->{RETURN_TYPE}) { my $conv = $self->ConvertObjectToPythonData("r", $fn->{RETURN_TYPE}, "r->out.result", $fn); - if ($result_size > 1) { + if ($max_result_size > 1) { $self->pidl("PyTuple_SetItem(result, $i, $conv);"); - } else { + } elsif ($max_result_size == 1) { $self->pidl("result = $conv;"); + } else { + fatal($fn->{ORIGINAL}, "Internal error max_result_size=$max_result_size"); } - $signature .= "result"; + + if (not $is_raisable_return) { + $signature .= "result"; + } + } + + if ($fn->{RETURN_TYPE} and $is_raisable_return) { + $self->deindent; + $self->pidl("}"); + $self->pidl(""); + } elsif ($fn->{RETURN_TYPE}) { + $self->pidl(""); } if (substr($signature, -2) eq ", ") { @@ -1056,6 +1117,20 @@ sub PythonFunctionUnpackOut($$$) $signature .= ")"; } + if ($result_size == 0) { + if ($max_result_size != $result_size) { + $self->pidl("if (result_size == 0) {"); + $self->indent; + } + $self->pidl("result = Py_None;"); + $self->pidl("Py_INCREF(result);"); + if ($max_result_size != $result_size) { + $self->deindent; + $self->pidl("}"); + } + $self->pidl(""); + } + $self->pidl("return result;"); $self->deindent; $self->pidl("}"); -- 2.34.1 From 8e5406192973e9f45b1be34465d67853c54607b8 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 14:57:53 +0100 Subject: [PATCH 08/11] s4:pyrpc: allow connections with raise_result_exceptions=False This is needed in order to do useful tests with specific error codes and still checking all other out parameters. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 6b1ff9a38fcddbe72b00e28960414526a42bde14) --- pidl/lib/Parse/Pidl/Samba4/Python.pm | 3 +-- source4/librpc/rpc/pyrpc.h | 1 + source4/librpc/rpc/pyrpc_util.c | 27 +++++++++++++++++++++++---- source4/librpc/rpc/pyrpc_util.h | 2 +- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm index 7a415c99c3ab..285e4321b196 100644 --- a/pidl/lib/Parse/Pidl/Samba4/Python.pm +++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm @@ -967,10 +967,9 @@ sub PythonFunctionUnpackOut($$$) my $env = GenerateFunctionOutEnv($fn, "r->"); my $result_size = 0; - $self->pidl("static PyObject *$outfnname(struct $fn->{NAME} *r)"); + $self->pidl("static PyObject *$outfnname(struct $fn->{NAME} *r, bool raise_result_exception)"); $self->pidl("{"); $self->indent; - $self->pidl("_UNUSED_ bool raise_result_exception = true;"); foreach my $e (@{$fn->{ELEMENTS}}) { next unless (grep(/out/,@{$e->{DIRECTION}})); next if (($metadata_args->{in}->{$e->{NAME}} and grep(/in/, @{$e->{DIRECTION}})) or diff --git a/source4/librpc/rpc/pyrpc.h b/source4/librpc/rpc/pyrpc.h index 390e01a11967..09f4a1abd7ed 100644 --- a/source4/librpc/rpc/pyrpc.h +++ b/source4/librpc/rpc/pyrpc.h @@ -50,6 +50,7 @@ typedef struct { struct dcerpc_pipe *pipe; struct dcerpc_binding_handle *binding_handle; struct tevent_context *ev; + bool raise_result_exceptions; } dcerpc_InterfaceObject; diff --git a/source4/librpc/rpc/pyrpc_util.c b/source4/librpc/rpc/pyrpc_util.c index 43c29af0e76a..a0910b4f802d 100644 --- a/source4/librpc/rpc/pyrpc_util.c +++ b/source4/librpc/rpc/pyrpc_util.c @@ -102,11 +102,29 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py PyObject *py_lp_ctx = Py_None, *py_credentials = Py_None, *py_basis = Py_None; NTSTATUS status; unsigned int timeout = (unsigned int)-1; + int raise_result_exceptions = 1; const char *kwnames[] = { - "binding", "lp_ctx", "credentials", "timeout", "basis_connection", NULL + "binding", + "lp_ctx", + "credentials", + "timeout", + "basis_connection", + "raise_result_exceptions", + NULL }; - - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s|OOIO:samr", discard_const_p(char *, kwnames), &binding_string, &py_lp_ctx, &py_credentials, &timeout, &py_basis)) { + bool ok; + + ok = PyArg_ParseTupleAndKeywords(args, + kwargs, + "s|OOIOp", + discard_const_p(char *, kwnames), + &binding_string, + &py_lp_ctx, + &py_credentials, + &timeout, + &py_basis, + &raise_result_exceptions); + if (!ok) { return NULL; } @@ -122,6 +140,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py return NULL; } + ret->raise_result_exceptions = (raise_result_exceptions != 0); ret->pipe = NULL; ret->binding_handle = NULL; ret->ev = NULL; @@ -300,7 +319,7 @@ static PyObject *py_dcerpc_run_function(dcerpc_InterfaceObject *iface, return NULL; } - result = md->unpack_out_data(r); + result = md->unpack_out_data(r, iface->raise_result_exceptions); talloc_free(mem_ctx); return result; diff --git a/source4/librpc/rpc/pyrpc_util.h b/source4/librpc/rpc/pyrpc_util.h index 73157fcba1c2..8da68a3555a1 100644 --- a/source4/librpc/rpc/pyrpc_util.h +++ b/source4/librpc/rpc/pyrpc_util.h @@ -34,7 +34,7 @@ void PyErr_SetDCERPCStatus(struct dcerpc_pipe *p, NTSTATUS status); typedef NTSTATUS (*py_dcerpc_call_fn) (struct dcerpc_binding_handle *, TALLOC_CTX *, void *); typedef bool (*py_data_pack_fn) (PyObject *args, PyObject *kwargs, void *r); -typedef PyObject *(*py_data_unpack_fn) (void *r); +typedef PyObject *(*py_data_unpack_fn) (void *r, bool raise_result_exception); struct PyNdrRpcMethodDef { const char *name; -- 2.34.1 From 091bf6f670ada9542896fe3aa9313bbc12e23be6 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 28 Jan 2025 09:51:14 +0100 Subject: [PATCH 09/11] python:tests/dcerpc/lsa: add tests for invalid LookupSids2 combinations BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit abba3495d23524142a4cf55d208dce041adee96b) --- python/samba/tests/dcerpc/lsa.py | 226 +++++++++++++++++++- selftest/knownfail.d/samba.tests.dcerpc.lsa | 1 + 2 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/samba.tests.dcerpc.lsa diff --git a/python/samba/tests/dcerpc/lsa.py b/python/samba/tests/dcerpc/lsa.py index 355bb1f44409..685dd14bd185 100644 --- a/python/samba/tests/dcerpc/lsa.py +++ b/python/samba/tests/dcerpc/lsa.py @@ -20,12 +20,18 @@ """Tests for samba.dcerpc.lsa.""" -from samba.dcerpc import lsa +from samba.dcerpc import lsa, security from samba.credentials import Credentials from samba.tests import TestCase from samba.dcerpc.security import dom_sid from samba import NTSTATUSError -from samba.ntstatus import NT_STATUS_ACCESS_DENIED +from samba.ntstatus import ( + NT_STATUS_OK, + NT_STATUS_ACCESS_DENIED, + NT_STATUS_NONE_MAPPED, + NT_STATUS_SOME_NOT_MAPPED, + NT_STATUS_INVALID_SID, +) import samba.tests class LsaTests(TestCase): @@ -331,3 +337,219 @@ class LsaTests(TestCase): client_revision) if (e.exception.args[0] != NT_STATUS_ACCESS_DENIED): raise AssertionError("LookupSids3 without schannel must fail with ACCESS_DENIED") + + def test_lsa_LookupSids2_none_mapped(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_np:%s[print]" % self.server, + self.lp, + machine_creds, + raise_result_exceptions=False) + + objectAttr = lsa.ObjectAttribute() + objectAttr.sec_qos = lsa.QosInfo() + + (pol_handle, status) = c.OpenPolicy2('', + objectAttr, + security.SEC_FLAG_MAXIMUM_ALLOWED) + self.assertEqual(status[0], NT_STATUS_OK) + self.assertIsNotNone(pol_handle) + + x0 = dom_sid("S-1-3-66") + sid0 = lsa.SidPtr() + sid0.sid = x0 + x1 = dom_sid("S-1-3-77") + sid1 = lsa.SidPtr() + sid1.sid = x1 + x2 = dom_sid("S-1-3-88") + sid2 = lsa.SidPtr() + sid2.sid = x2 + x3 = dom_sid("S-1-3-99") + sid3 = lsa.SidPtr() + sid3.sid = x3 + sids = lsa.SidArray() + sids.sids = [sid0,sid1,sid2,sid3] + sids.num_sids = 4 + + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + (domains, names, count, status) = c.LookupSids2(pol_handle, + sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(status[0], NT_STATUS_NONE_MAPPED) + self.assertEqual(count, 0) + self.assertIsNotNone(domains) + self.assertEqual(domains.count, 0) + self.assertIsNotNone(names) + self.assertEqual(names.count, 4) + self.assertEqual(names.names[0].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[0].name.string, str(x0)) + self.assertEqual(names.names[0].sid_index, 0xffffffff) + self.assertEqual(names.names[1].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[1].name.string, str(x1)) + self.assertEqual(names.names[1].sid_index, 0xffffffff) + self.assertEqual(names.names[2].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[2].name.string, str(x2)) + self.assertEqual(names.names[2].sid_index, 0xffffffff) + self.assertEqual(names.names[3].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[3].name.string, str(x3)) + self.assertEqual(names.names[3].sid_index, 0xffffffff) + + return + + def test_lsa_LookupSids2_some_not_mapped(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_np:%s[print]" % self.server, + self.lp, + machine_creds, + raise_result_exceptions=False) + + objectAttr = lsa.ObjectAttribute() + objectAttr.sec_qos = lsa.QosInfo() + + (pol_handle, status) = c.OpenPolicy2('', + objectAttr, + security.SEC_FLAG_MAXIMUM_ALLOWED) + self.assertEqual(status[0], NT_STATUS_OK) + self.assertIsNotNone(pol_handle) + + dx0 = dom_sid("S-1-3") + + x0 = dom_sid("S-1-3-66") + sid0 = lsa.SidPtr() + sid0.sid = x0 + x1 = dom_sid("S-1-3-0") + sid1 = lsa.SidPtr() + sid1.sid = x1 + x2 = dom_sid("S-1-3") + sid2 = lsa.SidPtr() + sid2.sid = x2 + x3 = dom_sid("S-1-3-99") + sid3 = lsa.SidPtr() + sid3.sid = x3 + sids = lsa.SidArray() + sids.sids = [sid0,sid1,sid2,sid3] + sids.num_sids = 4 + + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + (domains, names, count, status) = c.LookupSids2(pol_handle, + sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(status[0], NT_STATUS_SOME_NOT_MAPPED) + self.assertEqual(count, 1) + self.assertIsNotNone(domains) + self.assertEqual(domains.count, 1) + self.assertEqual(domains.domains[0].name.string, "") + self.assertEqual(domains.domains[0].sid, dx0) + self.assertIsNotNone(names) + self.assertEqual(names.count, 4) + self.assertEqual(names.names[0].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[0].name.string, str(x0)) + self.assertEqual(names.names[0].sid_index, 0xffffffff) + self.assertEqual(names.names[1].sid_type, lsa.SID_NAME_WKN_GRP) + self.assertEqual(names.names[1].name.string, "CREATOR OWNER") + self.assertEqual(names.names[1].sid_index, 0) + self.assertEqual(names.names[2].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[2].name.string, str(x2)) + self.assertEqual(names.names[2].sid_index, 0xffffffff) + self.assertEqual(names.names[3].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertEqual(names.names[3].name.string, str(x3)) + self.assertEqual(names.names[3].sid_index, 0xffffffff) + + return + + def test_lsa_LookupSids2_invalid_sid(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_np:%s[print]" % self.server, + self.lp, + machine_creds, + raise_result_exceptions=False) + + objectAttr = lsa.ObjectAttribute() + objectAttr.sec_qos = lsa.QosInfo() + + (pol_handle, status) = c.OpenPolicy2('', + objectAttr, + security.SEC_FLAG_MAXIMUM_ALLOWED) + self.assertEqual(status[0], NT_STATUS_OK) + self.assertIsNotNone(pol_handle) + + dx0 = dom_sid("S-1-3") + + x0 = dom_sid("S-1-3-66") + sid0 = lsa.SidPtr() + sid0.sid = x0 + x1 = dom_sid("S-1-3-77") + sid1 = lsa.SidPtr() + sid1.sid = x1 + x2 = dom_sid("S-1-3") + sid2 = lsa.SidPtr() + sid2.sid = x2 + x3 = dom_sid("S-1-3-99") + sid3 = lsa.SidPtr() + sid3.sid = x3 + sids = lsa.SidArray() + sids.sids = [sid0,sid1,sid2,sid3] + sids.num_sids = 4 + + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + (domains, names, count, status) = c.LookupSids2(pol_handle, + sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(status[0], NT_STATUS_INVALID_SID) + self.assertEqual(count, 0) + self.assertIsNotNone(domains) + self.assertEqual(domains.count, 0) + self.assertIsNotNone(names) + self.assertEqual(names.count, 4) + self.assertEqual(names.names[0].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertIsNone(names.names[0].name.string) + self.assertEqual(names.names[0].sid_index, 0xffffffff) + self.assertEqual(names.names[1].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertIsNone(names.names[1].name.string) + self.assertEqual(names.names[1].sid_index, 0xffffffff) + self.assertEqual(names.names[2].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertIsNone(names.names[2].name.string) + self.assertEqual(names.names[2].sid_index, 0xffffffff) + self.assertEqual(names.names[3].sid_type, lsa.SID_NAME_UNKNOWN) + self.assertIsNone(names.names[3].name.string) + self.assertEqual(names.names[3].sid_index, 0xffffffff) + + return diff --git a/selftest/knownfail.d/samba.tests.dcerpc.lsa b/selftest/knownfail.d/samba.tests.dcerpc.lsa new file mode 100644 index 000000000000..1cd75e20d847 --- /dev/null +++ b/selftest/knownfail.d/samba.tests.dcerpc.lsa @@ -0,0 +1 @@ +^samba.tests.dcerpc.lsa.*.LsaTests.test_lsa_LookupSids2_invalid_sid -- 2.34.1 From a89a238ab2ae6c5f2acd02a6c0002f1e387c0321 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 12 Aug 2020 17:08:14 +0200 Subject: [PATCH 10/11] libcli/security: let dom_sid_lookup_predefined_sid() behave like Windows 2008R2 Windows 2008R2 (172.31.9.133) returns the following: #> rpcclient 172.31.9.133 -Uadministrator%A1b2C3d4 -c 'lookupsids S-1-22-1 S-1-22-1-0;lookupsids S-1-22;lookupsids S-1-3-0 S-1-3-99;lookupsids S-1-3' S-1-22-1 *unknown*\*unknown* (8) S-1-22-1-0 *unknown*\*unknown* (8) result was NT_STATUS_INVALID_SID S-1-3-0 \CREATOR OWNER (5) S-1-3-99 *unknown*\*unknown* (8) result was NT_STATUS_INVALID_SID While the current Samba (172.31.9.163) returns the following: #> rpcclient 172.31.9.163 -Uadministrator%A1b2C3d4 -c 'lookupsids S-1-22-1 S-1-22-1-0;lookupsids S-1-22;lookupsids S-1-3-0 S-1-3-99;lookupsids S-1-3' result was NT_STATUS_INVALID_SID result was NT_STATUS_INVALID_SID S-1-3-0 \CREATOR OWNER (5) S-1-3-99 *unknown*\*unknown* (8) S-1-3 *unknown*\*unknown* (8) With this change also return the same as Windows 2008R2: #> rpcclient 172.31.9.163 -Uadministrator%A1b2C3d4 -c 'lookupsids S-1-22-1 S-1-22-1-0;lookupsids S-1-22;lookupsids S-1-3-0 S-1-3-99;lookupsids S-1-3' S-1-22-1 *unknown*\*unknown* (8) S-1-22-1-0 *unknown*\*unknown* (8) result was NT_STATUS_INVALID_SID S-1-3-0 \CREATOR OWNER (5) S-1-3-99 *unknown*\*unknown* (8) result was NT_STATUS_INVALID_SID This is a minimal fix in order to avoid crashes in the Windows Explorer. The real fix needs more work and additional tests, as the behavior seems to be different in newer Windows releases. The following patch will let us behave like Windows 2022/2025... BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 9f369c62317d74615834f99a088caababef685fc) --- libcli/security/util_sid.c | 5 +---- selftest/knownfail.d/samba.tests.dcerpc.lsa | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/libcli/security/util_sid.c b/libcli/security/util_sid.c index 54a2fc35fda7..d7631374af11 100644 --- a/libcli/security/util_sid.c +++ b/libcli/security/util_sid.c @@ -1068,7 +1068,6 @@ NTSTATUS dom_sid_lookup_predefined_sid(const struct dom_sid *sid, const char **authority_name) { size_t di; - bool match_domain = false; *name = NULL; *type = SID_NAME_UNKNOWN; @@ -1090,8 +1089,6 @@ NTSTATUS dom_sid_lookup_predefined_sid(const struct dom_sid *sid, continue; } - match_domain = true; - for (ni = 0; ni < d->num_names; ni++) { const struct predefined_name_mapping *n = &d->names[ni]; @@ -1109,7 +1106,7 @@ NTSTATUS dom_sid_lookup_predefined_sid(const struct dom_sid *sid, } } - if (!match_domain) { + if (sid->num_auths == 0) { return NT_STATUS_INVALID_SID; } diff --git a/selftest/knownfail.d/samba.tests.dcerpc.lsa b/selftest/knownfail.d/samba.tests.dcerpc.lsa index 1cd75e20d847..a0cc4ec1b37a 100644 --- a/selftest/knownfail.d/samba.tests.dcerpc.lsa +++ b/selftest/knownfail.d/samba.tests.dcerpc.lsa @@ -1 +1,2 @@ ^samba.tests.dcerpc.lsa.*.LsaTests.test_lsa_LookupSids2_invalid_sid +^samba.tests.dcerpc.lsa.*.LsaTests.test_lsa_LookupSids2_some_not_mapped -- 2.34.1 From c41287137951581017e2be53ccf2ffd9300884d0 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 10 Mar 2023 15:05:15 +0100 Subject: [PATCH 11/11] s4:rpc_server/lsa: let LookupSids* behave like Windows 2022/2025 The important part is the INVALID_SID should not cause an early exit of the loop. We need to return the intact names array with the correct count. And only return INVALID_SID if we would otherwise return NONE_MAPPED. For SOME_NOT_MAPPED we need to ignore invalid sids and just pretend they are not mapped. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14213 Signed-off-by: Stefan Metzmacher Reviewed-by: Douglas Bagnall (cherry picked from commit 218a0f067c894cbf61cde6183a269c0474d64ddc) --- selftest/knownfail.d/samba.tests.dcerpc.lsa | 2 -- source4/rpc_server/lsa/lsa_lookup.c | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/samba.tests.dcerpc.lsa diff --git a/selftest/knownfail.d/samba.tests.dcerpc.lsa b/selftest/knownfail.d/samba.tests.dcerpc.lsa deleted file mode 100644 index a0cc4ec1b37a..000000000000 --- a/selftest/knownfail.d/samba.tests.dcerpc.lsa +++ /dev/null @@ -1,2 +0,0 @@ -^samba.tests.dcerpc.lsa.*.LsaTests.test_lsa_LookupSids2_invalid_sid -^samba.tests.dcerpc.lsa.*.LsaTests.test_lsa_LookupSids2_some_not_mapped diff --git a/source4/rpc_server/lsa/lsa_lookup.c b/source4/rpc_server/lsa/lsa_lookup.c index ca3ad4f961ed..6cfbbb3cb383 100644 --- a/source4/rpc_server/lsa/lsa_lookup.c +++ b/source4/rpc_server/lsa/lsa_lookup.c @@ -35,6 +35,7 @@ struct dcesrv_lsa_TranslatedItem { uint32_t flags; uint32_t wb_idx; bool done; + bool invalid_sid; struct { const char *domain; /* only $DOMAIN\ */ const char *namespace; /* $NAMESPACE\ or @$NAMESPACE */ @@ -380,6 +381,10 @@ static NTSTATUS dcesrv_lsa_LookupSids_base_call(struct dcesrv_lsa_LookupSids_bas status = view->lookup_sid(state, item); if (NT_STATUS_IS_OK(status)) { item->done = true; + } else if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_SID)) { + item->done = true; + item->invalid_sid = true; + status = NT_STATUS_OK; } else if (NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED)) { status = NT_STATUS_OK; } else if (NT_STATUS_EQUAL(status, NT_STATUS_SOME_NOT_MAPPED)) { @@ -438,6 +443,7 @@ static NTSTATUS dcesrv_lsa_LookupSids_base_finish( struct dcesrv_lsa_LookupSids_base_state *state) { struct lsa_LookupSids3 *r = &state->r; + uint32_t num_invalid_sid = 0; uint32_t i; for (i=0;iin.sids->num_sids;i++) { @@ -470,9 +476,18 @@ static NTSTATUS dcesrv_lsa_LookupSids_base_finish( if (item->type != SID_NAME_UNKNOWN) { (*r->out.count)++; } + if (item->invalid_sid) { + num_invalid_sid++; + } } if (*r->out.count == 0) { + if (num_invalid_sid != 0) { + for (i=0;iout.names->count;i++) { + r->out.names->names[i].name.string = NULL; + } + return NT_STATUS_INVALID_SID; + } return NT_STATUS_NONE_MAPPED; } if (*r->out.count != r->in.sids->num_sids) { -- 2.34.1