From 5d4ba6d196fa273c9edc94603a3256be2c14e30e 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 d7ccf8309e86..838e5090f205 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 cd975484a496406a6377b8c6edd1d51f7f470492 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 838e5090f205..f0583561cb3f 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 f39e7ea60002523cc24d62c132ed795e1873850c 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 f0583561cb3f..eec2c318b612 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 1688d58a3806eaa16d954b47f86b9156fc0e724e 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 eec2c318b612..cfb63e4107fa 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 e8317520e9f3006f4183de32f0ac2a337b4546b2 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 cfb63e4107fa..6c0e0a19145f 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 d0a47ef60965486e362ba6ed8062599d7bff8fc0 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 6c0e0a19145f..3a975058a07f 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 2c8994f78026b27b020630469285906584c028da 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 3a975058a07f..25805de87850 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 54d00fe644cbd3cd6b53fc584241ac84b67c28d1 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 25805de87850..63f0f72605d0 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 d6a59daf85ae276f3f5c3ddaf983c9a0e8f431af 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 2f0f00d58f38fb84fc7f9b62ea3b9324674bea62 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 0942b2fe259d..4190cd59432b 100644 --- a/libcli/security/util_sid.c +++ b/libcli/security/util_sid.c @@ -1072,7 +1072,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; @@ -1094,8 +1093,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]; @@ -1113,7 +1110,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 1deb2ab3fb3afd81f58c68c7435d1d23d848f1f3 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