From 0dfd2ede75ec1060cb424cbb32bb03b365f36478 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 3 May 2018 09:53:56 +1200 Subject: [PATCH 1/2] s4/pyrpc_util: appropriately decrement refcounts on failure Signed-off-by: Douglas Bagnall Reviewed-by: Noel Power (cherry picked from commit e23b9f88cc1c8a8c8cda07fb25d639218c12d91a) --- source4/librpc/rpc/pyrpc_util.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/source4/librpc/rpc/pyrpc_util.c b/source4/librpc/rpc/pyrpc_util.c index 3a151e1591f..70274f0268e 100644 --- a/source4/librpc/rpc/pyrpc_util.c +++ b/source4/librpc/rpc/pyrpc_util.c @@ -132,6 +132,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (event_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -139,6 +140,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (lp_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -147,6 +149,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } } else if (py_basis != Py_None) { @@ -157,6 +160,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py py_base = PyImport_ImportModule("samba.dcerpc.base"); if (py_base == NULL) { TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -164,12 +168,17 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (ClientConnection_Type == NULL) { PyErr_SetNone(PyExc_TypeError); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); + Py_DECREF(py_base); return NULL; } if (!PyObject_TypeCheck(py_basis, ClientConnection_Type)) { PyErr_SetString(PyExc_TypeError, "basis_connection must be a DCE/RPC connection"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); + Py_DECREF(py_base); + Py_DECREF(ClientConnection_Type); return NULL; } @@ -178,6 +187,9 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (base_pipe == NULL) { PyErr_NoMemory(); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); + Py_DECREF(py_base); + Py_DECREF(ClientConnection_Type); return NULL; } @@ -185,10 +197,15 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); + Py_DECREF(py_base); + Py_DECREF(ClientConnection_Type); return NULL; } ret->pipe = talloc_steal(ret->mem_ctx, ret->pipe); + Py_XDECREF(ClientConnection_Type); + Py_XDECREF(py_base); } else { struct tevent_context *event_ctx; struct loadparm_context *lp_ctx; @@ -198,6 +215,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (event_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -205,6 +223,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (lp_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -212,6 +231,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (credentials == NULL) { PyErr_SetString(PyExc_TypeError, "Expected credentials"); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } status = dcerpc_pipe_connect(ret->mem_ctx, &ret->pipe, binding_string, @@ -219,6 +239,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); return NULL; } @@ -378,6 +399,7 @@ PyObject *py_return_ndr_struct(const char *module_name, const char *type_name, py_type = (PyTypeObject *)PyObject_GetAttrString(module, type_name); if (py_type == NULL) { + Py_DECREF(module); return NULL; } -- 2.17.1 From 00995d3f31dc670fbf03fe8e65ac6dc2e806d427 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 8 May 2019 11:30:20 +1200 Subject: [PATCH 2/2] s4 librpc rpc pyrpc: Ensure tevent_context deleted last Ensure that the tevent_context is deleted after the connection, to prevent a use after free. Note: Py_DECREF calls dcerpc_interface_dealloc so the TALLOC_FREE(ret->mem_ctx) calls in the error paths of py_dcerpc_interface_init_helper needed removal. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13932 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett (cherry picked from commit d65b7641c84976c543ded8f0de5ab2da3c19b407) --- source4/librpc/rpc/pyrpc.c | 18 +++++++++++++ source4/librpc/rpc/pyrpc.h | 1 + source4/librpc/rpc/pyrpc_util.c | 48 ++++++++++++++------------------- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/source4/librpc/rpc/pyrpc.c b/source4/librpc/rpc/pyrpc.c index cf2d4c24007..1aa8008c013 100644 --- a/source4/librpc/rpc/pyrpc.c +++ b/source4/librpc/rpc/pyrpc.c @@ -281,9 +281,27 @@ static PyMethodDef dcerpc_interface_methods[] = { static void dcerpc_interface_dealloc(PyObject* self) { dcerpc_InterfaceObject *interface = (dcerpc_InterfaceObject *)self; + + /* + * This can't fail, and if it did talloc_unlink(NULL, NULL) is + * harmless below + */ + struct tevent_context *ev_save = talloc_reparent( + NULL, interface->mem_ctx, interface->ev); + interface->binding_handle = NULL; interface->pipe = NULL; + + /* + * Free everything *except* the event context, which must go + * away last + */ TALLOC_FREE(interface->mem_ctx); + + /* + * Now wish a fond goodbye to the event context itself + */ + talloc_unlink(NULL, ev_save); self->ob_type->tp_free(self); } diff --git a/source4/librpc/rpc/pyrpc.h b/source4/librpc/rpc/pyrpc.h index 968bf863c4c..8852def7251 100644 --- a/source4/librpc/rpc/pyrpc.h +++ b/source4/librpc/rpc/pyrpc.h @@ -44,6 +44,7 @@ typedef struct { TALLOC_CTX *mem_ctx; struct dcerpc_pipe *pipe; struct dcerpc_binding_handle *binding_handle; + struct tevent_context *ev; } dcerpc_InterfaceObject; diff --git a/source4/librpc/rpc/pyrpc_util.c b/source4/librpc/rpc/pyrpc_util.c index 70274f0268e..c8931bf96f0 100644 --- a/source4/librpc/rpc/pyrpc_util.c +++ b/source4/librpc/rpc/pyrpc_util.c @@ -118,6 +118,7 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py ret = PyObject_New(dcerpc_InterfaceObject, type); ret->pipe = NULL; ret->binding_handle = NULL; + ret->ev = NULL; ret->mem_ctx = talloc_new(NULL); if (ret->mem_ctx == NULL) { PyErr_NoMemory(); @@ -125,30 +126,26 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py } if (strncmp(binding_string, "irpc:", 5) == 0) { - struct tevent_context *event_ctx; struct loadparm_context *lp_ctx; - event_ctx = s4_event_context_init(ret->mem_ctx); - if (event_ctx == NULL) { + ret->ev = s4_event_context_init(ret->mem_ctx); + if (ret->ev == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } - lp_ctx = lpcfg_from_py_object(event_ctx, py_lp_ctx); + lp_ctx = lpcfg_from_py_object(ret->ev, py_lp_ctx); if (lp_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } status = pyrpc_irpc_connect(ret->mem_ctx, binding_string+5, table, - event_ctx, lp_ctx, &ret->binding_handle); + ret->ev, lp_ctx, &ret->binding_handle); if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } @@ -159,7 +156,6 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py py_base = PyImport_ImportModule("samba.dcerpc.base"); if (py_base == NULL) { - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } @@ -167,7 +163,6 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py ClientConnection_Type = (PyTypeObject *)PyObject_GetAttrString(py_base, "ClientConnection"); if (ClientConnection_Type == NULL) { PyErr_SetNone(PyExc_TypeError); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); Py_DECREF(py_base); return NULL; @@ -175,7 +170,6 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py if (!PyObject_TypeCheck(py_basis, ClientConnection_Type)) { PyErr_SetString(PyExc_TypeError, "basis_connection must be a DCE/RPC connection"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); Py_DECREF(py_base); Py_DECREF(ClientConnection_Type); @@ -186,7 +180,17 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py ((dcerpc_InterfaceObject *)py_basis)->pipe); if (base_pipe == NULL) { PyErr_NoMemory(); - TALLOC_FREE(ret->mem_ctx); + Py_DECREF(ret); + Py_DECREF(py_base); + Py_DECREF(ClientConnection_Type); + return NULL; + } + + ret->ev = talloc_reference( + ret->mem_ctx, + ((dcerpc_InterfaceObject *)py_basis)->ev); + if (ret->ev == NULL) { + PyErr_NoMemory(); Py_DECREF(ret); Py_DECREF(py_base); Py_DECREF(ClientConnection_Type); @@ -196,7 +200,6 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py status = dcerpc_secondary_context(base_pipe, &ret->pipe, table); if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); Py_DECREF(py_base); Py_DECREF(ClientConnection_Type); @@ -207,22 +210,19 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py Py_XDECREF(ClientConnection_Type); Py_XDECREF(py_base); } else { - struct tevent_context *event_ctx; struct loadparm_context *lp_ctx; struct cli_credentials *credentials; - event_ctx = s4_event_context_init(ret->mem_ctx); - if (event_ctx == NULL) { + ret->ev = s4_event_context_init(ret->mem_ctx); + if (ret->ev == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } - lp_ctx = lpcfg_from_py_object(event_ctx, py_lp_ctx); + lp_ctx = lpcfg_from_py_object(ret->ev, py_lp_ctx); if (lp_ctx == NULL) { PyErr_SetString(PyExc_TypeError, "Expected loadparm context"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } @@ -230,24 +230,16 @@ PyObject *py_dcerpc_interface_init_helper(PyTypeObject *type, PyObject *args, Py credentials = cli_credentials_from_py_object(py_credentials); if (credentials == NULL) { PyErr_SetString(PyExc_TypeError, "Expected credentials"); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } status = dcerpc_pipe_connect(ret->mem_ctx, &ret->pipe, binding_string, - table, credentials, event_ctx, lp_ctx); + table, credentials, ret->ev, lp_ctx); if (!NT_STATUS_IS_OK(status)) { PyErr_SetNTSTATUS(status); - TALLOC_FREE(ret->mem_ctx); Py_DECREF(ret); return NULL; } - - /* - * the event context is cached under the connection, - * so let it be a child of it. - */ - talloc_steal(ret->pipe->conn, event_ctx); } if (ret->pipe) { -- 2.17.1