Attachment 15167 Details for Bug 13941 – Proposed patch for V4.10 and V4.9

[patch] Proposed patch for V4.10 and V4.9

bug-13941-v4-9.patch.txt (text/plain), 5.45 KB, created by Gary Lockyer on 2019-05-19 20:49:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2019-05-19 20:49:14 UTC

Size: 5.45 KB

patch

obsolete

>From 3b5512091aecbbf6a896da9c4e96a1d139aa4f86 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 15 May 2019 14:47:53 +1200
>Subject: [PATCH 1/2] s4 dsdb/repl_meta_data: allocate new extended DNs during
> ADD on a better context
>
>Lower down in this function new_values is assigned over el->values and is
>filled in with the values of all the parsed DNs.  Therefore it is the natural
>talloc parent.
>
>This will allow el->values to be allocated on tmp_ctx in the next commit for
>a working area during the function call.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>(cherry picked from commit 4aa9924310287ff3b36618496fa6c707c615ad4c)
>---
> source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>index be4ea5592c4..053bdd1fc69 100644
>--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>@@ -1007,7 +1007,7 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
> 
> 	for (i = 0; i < el->num_values; i++) {
> 		struct parsed_dn *p = &pdn[i];
>-		ret = replmd_build_la_val(el->values, p->v, p->dsdb_dn,
>+		ret = replmd_build_la_val(new_values, p->v, p->dsdb_dn,
> 					  &ac->our_invocation_id,
> 					  ac->seq_num, now);
> 		if (ret != LDB_SUCCESS) {
>-- 
>2.17.1
>
>
>From e01fdb096450f2f54338c1554e38958f14642289 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Tue, 14 May 2019 15:53:22 +1200
>Subject: [PATCH 2/2] s4 dsdb/repl_meta_data: fix use after free in
> dsdb_audit_add_ldb_value
>
>Fix use after free detected by AddressSanitizer
>
>AddressSanitizer: heap-use-after-free on address 0x61400026a4a0
>                  at pc 0x7fd555c52f12 bp 0x7ffed7231180 sp 0x7ffed7231170
>                  READ of size 1 at 0x61400026a4a0 thread T0
>    #0 0x7fd555c52f11 in ldb_should_b64_encode
>       ../../lib/ldb/common/ldb_ldif.c:197
>    #1 0x7fd539dc9417 in dsdb_audit_add_ldb_value
>       ../../source4/dsdb/samdb/ldb_modules/audit_util.c:491
>    #2 0x7fd539dc9417 in dsdb_audit_attributes_json
>       ../../source4/dsdb/samdb/ldb_modules/audit_util.c:651
>    #3 0x7fd539dc6a7e in operation_json
>       ../../source4/dsdb/samdb/ldb_modules/audit_log.c:305
>
>The problem is that at the successful end of these functions
>el->values is overwritten with new_values.  However get_parsed_dns()
>points p->v at the supplied el and it effectively gets used
>as a working area by replmd_build_la_val().  So we must duplicate it
>because our caller only called ldb_msg_copy_shallow().
>
>The reason this matters is that the audit_log module is
>above repl_meta_data in the stack, and tries to log the
>ldb_message it saw after the reply (to include the error code).
>If that ldb_message is changed it is not only misleading,
>it can point to memory that has since gone away.
>
>In this case the memory for the full extended DN in the
>member attribute ended up on 'ac', a context lost by
>the time repl_meta_data has finished processing.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13941
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>
>Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
>Autobuild-Date(master): Wed May 15 05:35:47 UTC 2019 on sn-devel-184
>
>(cherry picked from commit 0daa0ff921b270df9b794f02acbaa391c95cd89b)
>---
> .../dsdb/samdb/ldb_modules/repl_meta_data.c   | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>index 053bdd1fc69..04a51ecab51 100644
>--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>@@ -984,6 +984,24 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
> 		talloc_free(tmp_ctx);
> 		return LDB_ERR_CONSTRAINT_VIOLATION;
> 	}
>+
>+	/*
>+	 * At the successful end of these functions el->values is
>+	 * overwritten with new_values.  However get_parsed_dns()
>+	 * points p->v at the supplied el and it effectively gets used
>+	 * as a working area by replmd_build_la_val().  So we must
>+	 * duplicate it because our caller only called
>+	 * ldb_msg_copy_shallow().
>+	 */
>+
>+	el->values = talloc_memdup(tmp_ctx,
>+				   el->values,
>+				   sizeof(el->values[0]) * el->num_values);
>+	if (el->values == NULL) {
>+		ldb_module_oom(module);
>+		talloc_free(tmp_ctx);
>+		return LDB_ERR_OPERATIONS_ERROR;
>+	}
> 	
> 	ret = get_parsed_dns(module, tmp_ctx, el, &pdn,
> 			     sa->syntax->ldap_oid, parent);
>@@ -3034,6 +3052,24 @@ static int replmd_modify_la_replace(struct ldb_module *module,
> 		return LDB_SUCCESS;
> 	}
> 
>+	/*
>+	 * At the successful end of these functions el->values is
>+	 * overwritten with new_values.  However get_parsed_dns()
>+	 * points p->v at the supplied el and it effectively gets used
>+	 * as a working area by replmd_build_la_val().  So we must
>+	 * duplicate it because our caller only called
>+	 * ldb_msg_copy_shallow().
>+	 */
>+
>+	el->values = talloc_memdup(tmp_ctx,
>+				   el->values,
>+				   sizeof(el->values[0]) * el->num_values);
>+	if (el->values == NULL) {
>+		ldb_module_oom(module);
>+		talloc_free(tmp_ctx);
>+		return LDB_ERR_OPERATIONS_ERROR;
>+	}
>+
> 	ret = get_parsed_dns(module, tmp_ctx, el, &dns, ldap_oid, parent);
> 	if (ret != LDB_SUCCESS) {
> 		talloc_free(tmp_ctx);
>-- 
>2.17.1
>

Flags:

abartlet: review+
gary: ci-passed+

Actions: View

Attachments on bug 13941: 15143 | 15167