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

diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 6c65820e84ef8a5e651849dfc7b4803f59943bff..5202c41a7bfebfe48123359ec938ac18a98e60ff 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -1029,6 +1029,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);
@@ -3074,6 +3092,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);