From 3b5512091aecbbf6a896da9c4e96a1d139aa4f86 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 Reviewed-by: Douglas Bagnall (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 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 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Autobuild-User(master): Andrew Bartlett 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