Attachment 15691 Details for Bug 14050 – Proposed patch, applies to master

[patch] Proposed patch, applies to master

gary-bug-14050-master-02.patch (text/plain), 4.96 KB, created by Gary Lockyer on 2019-12-16 19:08:36 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2019-12-16 19:08:36 UTC

Size: 4.96 KB

patch

obsolete

>From 71f10c304d426cec538204a26a6a16e660463314 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Mon, 16 Dec 2019 13:57:47 +1300
>Subject: [PATCH] kcc dns scavenging: Fix use after free in
> dns_tombstone_records_zone
>
>ldb_msg_add_empty reallocates the underlying element array, leaving
>old_el pointing to freed memory.
>
>This patch takes two defensive copies of the ldb message, and performs
>the updates on them rather than the ldb messages in the result.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=14050
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/kcc/scavenge_dns_records.c | 51 ++++++++++++++++++++-----
> 1 file changed, 42 insertions(+), 9 deletions(-)
>
>diff --git a/source4/dsdb/kcc/scavenge_dns_records.c b/source4/dsdb/kcc/scavenge_dns_records.c
>index 6c0684b3153..8e916cf7b06 100644
>--- a/source4/dsdb/kcc/scavenge_dns_records.c
>+++ b/source4/dsdb/kcc/scavenge_dns_records.c
>@@ -128,6 +128,8 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 	struct ldb_message_element *el = NULL;
> 	struct ldb_message_element *tombstone_el = NULL;
> 	struct ldb_message_element *old_el = NULL;
>+	struct ldb_message *new_msg = NULL;
>+	struct ldb_message *old_msg = NULL;
> 	int ret;
> 	struct GUID guid;
> 	struct GUID_txt_buf buf_guid;
>@@ -184,12 +186,29 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 	 * change.  This prevents race conditions.
> 	 */
> 	for (i = 0; i < res->count; i++) {
>-		old_el = ldb_msg_find_element(res->msgs[i], "dnsRecord");
>+		old_msg = ldb_msg_copy(mem_ctx, res->msgs[i]);
>+		if (old_msg == NULL) {
>+			return NT_STATUS_INTERNAL_ERROR;
>+		}
>+
>+		old_el = ldb_msg_find_element(old_msg, "dnsRecord");
>+		if (old_el == NULL) {
>+			TALLOC_FREE(old_msg);
>+			return NT_STATUS_INTERNAL_ERROR;
>+		}
>+
> 		old_el->flags = LDB_FLAG_MOD_DELETE;
>+		new_msg = ldb_msg_copy(mem_ctx, old_msg);
>+		if (new_msg == NULL) {
>+			TALLOC_FREE(old_msg);
>+			return NT_STATUS_INTERNAL_ERROR;
>+		}
> 
> 		ret = ldb_msg_add_empty(
>-		    res->msgs[i], "dnsRecord", LDB_FLAG_MOD_ADD, &el);
>+		    new_msg, "dnsRecord", LDB_FLAG_MOD_ADD, &el);
> 		if (ret != LDB_SUCCESS) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			return NT_STATUS_INTERNAL_ERROR;
> 		}
> 
>@@ -197,12 +216,16 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 		status = copy_current_records(mem_ctx, old_el, el, t);
> 
> 		if (!NT_STATUS_IS_OK(status)) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			return NT_STATUS_INTERNAL_ERROR;
> 		}
> 
> 		/* If nothing was expired, do nothing. */
> 		if (el->num_values == old_el->num_values &&
> 		    el->num_values != 0) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			continue;
> 		}
> 
>@@ -213,14 +236,16 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 			el->values = tombstone_blob;
> 			el->num_values = 1;
> 
>-			tombstone_el = ldb_msg_find_element(res->msgs[i],
>+			tombstone_el = ldb_msg_find_element(new_msg,
> 						  "dnsTombstoned");
> 			if (tombstone_el == NULL) {
>-				ret = ldb_msg_add_value(res->msgs[i],
>+				ret = ldb_msg_add_value(new_msg,
> 							"dnsTombstoned",
> 							true_struct,
> 							&tombstone_el);
> 				if (ret != LDB_SUCCESS) {
>+					TALLOC_FREE(old_msg);
>+					TALLOC_FREE(new_msg);
> 					return NT_STATUS_INTERNAL_ERROR;
> 				}
> 				tombstone_el->flags = LDB_FLAG_MOD_ADD;
>@@ -234,13 +259,15 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 			 * Do not change the status of dnsTombstoned
> 			 * if we found any live records
> 			 */
>-			ldb_msg_remove_attr(res->msgs[i],
>+			ldb_msg_remove_attr(new_msg,
> 					    "dnsTombstoned");
> 		}
> 
> 		/* Set DN to the GUID in case the object was moved. */
>-		el = ldb_msg_find_element(res->msgs[i], "objectGUID");
>+		el = ldb_msg_find_element(new_msg, "objectGUID");
> 		if (el == NULL) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			*error_string =
> 			    talloc_asprintf(mem_ctx,
> 					    "record has no objectGUID "
>@@ -251,20 +278,24 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 
> 		status = GUID_from_ndr_blob(el->values, &guid);
> 		if (!NT_STATUS_IS_OK(status)) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			*error_string =
> 			    discard_const_p(char, "Error: Invalid GUID.\n");
> 			return NT_STATUS_INTERNAL_ERROR;
> 		}
> 
> 		GUID_buf_string(&guid, &buf_guid);
>-		res->msgs[i]->dn =
>+		new_msg->dn =
> 		    ldb_dn_new_fmt(mem_ctx, samdb, "<GUID=%s>", buf_guid.buf);
> 
> 		/* Remove the GUID so we're not trying to modify it. */
>-		ldb_msg_remove_attr(res->msgs[i], "objectGUID");
>+		ldb_msg_remove_attr(new_msg, "objectGUID");
> 
>-		ret = ldb_modify(samdb, res->msgs[i]);
>+		ret = ldb_modify(samdb, new_msg);
> 		if (ret != LDB_SUCCESS) {
>+			TALLOC_FREE(old_msg);
>+			TALLOC_FREE(new_msg);
> 			*error_string =
> 			    talloc_asprintf(mem_ctx,
> 					    "Failed to modify dns record "
>@@ -273,6 +304,8 @@ NTSTATUS dns_tombstone_records_zone(TALLOC_CTX *mem_ctx,
> 					    ldb_errstring(samdb));
> 			return NT_STATUS_INTERNAL_ERROR;
> 		}
>+		TALLOC_FREE(old_msg);
>+		TALLOC_FREE(new_msg);
> 	}
> 
> 	return NT_STATUS_OK;
>-- 
>2.17.1
>

Flags:

abartlet: review+
gary: ci-passed+

Actions: View

Attachments on bug 14050: 15325 | 15684 | 15691 | 15692 | 15693 | 15694 | 15696 | 15697 | 15698 | 15699 | 15700 | 15739 | 15740