Attachment 15164 Details for Bug 13932 – Proposed patch for V4.10

[patch] Proposed patch for V4.10

bug-13932-v4-10.patch.txt (text/plain), 11.65 KB, created by Gary Lockyer on 2019-05-16 21:03:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2019-05-16 21:03:39 UTC

Size: 11.65 KB

patch

obsolete

>From 0dfd2ede75ec1060cb424cbb32bb03b365f36478 Mon Sep 17 00:00:00 2001
>From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>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 <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Noel Power <npower@samba.org>
>(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 <gary@catalyst.net.nz>
>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 <gary@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(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
>

Flags:

abartlet: review-
gary: ci-passed+

Actions: View

Attachments on bug 13932: 15129 | 15164 | 15181