From d49988193bbd452105e4456a0b4e6f9201faec90 Mon Sep 17 00:00:00 2001 From: Gary Lockyer 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 --- 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, "", 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