Issue Overview : A critical wild pointer issue has been identified in source4\dsdb\kcc\scavenge_dns_records.c involving memory management defects in the dns_tombstone_records_zone function. Problematic Code Segment old_el = ldb_msg_find_element(new_msg, "dnsRecord"); if (old_el == NULL) { TALLOC_FREE(new_msg); return NT_STATUS_INTERNAL_ERROR; } old_el->flags = LDB_FLAG_MOD_DELETE; ret = ldb_msg_add_empty( new_msg, "dnsRecord", LDB_FLAG_MOD_ADD, &el); if (ret != LDB_SUCCESS) { TALLOC_FREE(new_msg); return NT_STATUS_INTERNAL_ERROR; } Memory Reallocation Risk ldb_msg_add_empty() may trigger internal realloc() operations After reallocation, the memory address of new_msg may change Previously obtained old_el pointer may point to freed memory
thanks. I have restricted visibility for the moment while we work out what this means.
Created attachment 18652 [details] Patch Possible fix?
Created attachment 18653 [details] patch moving the realloc up I think we can do it like this, still only finding old_el once, but doing it after the ldb_msg_add_empty().
Created attachment 18654 [details] patch v2
Created attachment 18655 [details] patch v3 I'm thinking this is not a security release, because there is not really any gap (i.e. memory writes or context switches) between the free and the read in which the value could be replaced. There might be some OS that could reclaim the page, but that doesn't seem common as there have been no reports. Introduced in 4a2bfd249d06e229b080cb6f50a68086a8aa1e52.
Comment on attachment 18654 [details] patch v2 + /* + * This empty record will become the replacement for old_el. + * (we add it forst because it reallocs). + */ forst -> first? + if (old_el == NULL || + old_el == &(new_msg->elements[new_msg->num_elements - 1])) { if (old_el == NULL || old_el == el) { ? Otherwise looks good.
Comment on attachment 18655 [details] patch v3 Oh, you beat me to it.
This bug was referenced in samba master: 750f6847f04d5c18ee308ac8bc5bc0828c32deeb