samba/backport-CVE-2022-32746.patch

From a258b3c0636b208de699b1e693d86f5ee9985cfd Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 14 Jun 2022 21:09:53 +1200
Subject: [PATCH 01/99] CVE-2022-32746 s4/dsdb/objectclass_attrs: Fix typo

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/objectclass_attrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c b/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
index 6ab46a729a2..2a77353cdfc 100644
--- a/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
+++ b/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
@@ -263,7 +263,7 @@ static int attr_handler(struct oc_context *ac)
 												LDB_CONTROL_AS_SYSTEM_OID);
 					if (!dsdb_module_am_system(ac->module) && !as_system) {
 						ldb_asprintf_errstring(ldb,
-								       "objectclass_attrs: attribute '%s' on entry '%s' must can only be modified as system",
+								       "objectclass_attrs: attribute '%s' on entry '%s' can only be modified as system",
 								       msg->elements[i].name,
 								       ldb_dn_get_linearized(msg->dn));
 						return LDB_ERR_CONSTRAINT_VIOLATION;
-- 
2.25.1


From e2ef0f299aed8c0f9660f1d7912472d23e81fee8 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 21 Jun 2022 15:37:15 +1200
Subject: [PATCH 02/99] CVE-2022-32746 s4:dsdb:tests: Add test for deleting a
 disallowed SPN

If an account has an SPN that requires Write Property to set, we should
still be able to delete it with just Validated Write.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 selftest/knownfail.d/acl-spn-delete |  1 +
 source4/dsdb/tests/python/acl.py    | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 selftest/knownfail.d/acl-spn-delete

diff --git a/selftest/knownfail.d/acl-spn-delete b/selftest/knownfail.d/acl-spn-delete
new file mode 100644
index 00000000000..32018413c49
--- /dev/null
+++ b/selftest/knownfail.d/acl-spn-delete
@@ -0,0 +1 @@
+^samba4.ldap.acl.python.*__main__.AclSPNTests.test_delete_disallowed_spn\(
diff --git a/source4/dsdb/tests/python/acl.py b/source4/dsdb/tests/python/acl.py
index 70dca9b7678..3e5f7a44a79 100755
--- a/source4/dsdb/tests/python/acl.py
+++ b/source4/dsdb/tests/python/acl.py
@@ -2285,6 +2285,32 @@ class AclSPNTests(AclTests):
         else:
             self.fail(f'able to add disallowed SPN {not_allowed_spn}')
 
+    def test_delete_disallowed_spn(self):
+        # Grant Validated-SPN property.
+        mod = f'(OA;;SW;{security.GUID_DRS_VALIDATE_SPN};;{self.user_sid1})'
+        self.sd_utils.dacl_add_ace(self.computerdn, mod)
+
+        spn_base = f'HOST/{self.computername}'
+
+        not_allowed_spn = f'{spn_base}/{self.dcctx.get_domain_name()}'
+
+        # Add a disallowed SPN as admin.
+        msg = Message(Dn(self.ldb_admin, self.computerdn))
+        msg['servicePrincipalName'] = MessageElement(not_allowed_spn,
+                                                     FLAG_MOD_ADD,
+                                                     'servicePrincipalName')
+        self.ldb_admin.modify(msg)
+
+        # Ensure we are able to delete a disallowed SPN.
+        msg = Message(Dn(self.ldb_user1, self.computerdn))
+        msg['servicePrincipalName'] = MessageElement(not_allowed_spn,
+                                                     FLAG_MOD_DELETE,
+                                                     'servicePrincipalName')
+        try:
+            self.ldb_user1.modify(msg)
+        except LdbError:
+            self.fail(f'unable to delete disallowed SPN {not_allowed_spn}')
+
 
 # tests SEC_ADS_LIST vs. SEC_ADS_LIST_OBJECT
 @DynamicTestCase
-- 
2.25.1


From 6bc5e73000a639bab3c3d6789bdf879d5395bf9c Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 21 Jun 2022 14:41:02 +1200
Subject: [PATCH 03/99] CVE-2022-32746 s4/dsdb/partition: Fix LDB flags
 comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(req_msg->elements[el_idx].flags & LDB_FLAG_MOD_MASK) != 0

which is true whenever any of the LDB_FLAG_MOD_* values are set. Correct
the expression to what it was probably intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/partition.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/partition.c b/source4/dsdb/samdb/ldb_modules/partition.c
index 2544a106d13..2d90ca5d1b3 100644
--- a/source4/dsdb/samdb/ldb_modules/partition.c
+++ b/source4/dsdb/samdb/ldb_modules/partition.c
@@ -493,8 +493,8 @@ static int partition_copy_all_callback_action(
 			 * them here too
 			 */
 			for (el_idx=0; el_idx < req_msg->num_elements; el_idx++) {
-				if (req_msg->elements[el_idx].flags & LDB_FLAG_MOD_DELETE
-				    || ((req_msg->elements[el_idx].flags & LDB_FLAG_MOD_REPLACE) &&
+				if (LDB_FLAG_MOD_TYPE(req_msg->elements[el_idx].flags) == LDB_FLAG_MOD_DELETE
+				    || ((LDB_FLAG_MOD_TYPE(req_msg->elements[el_idx].flags) == LDB_FLAG_MOD_REPLACE) &&
 					req_msg->elements[el_idx].num_values == 0)) {
 					if (ldb_msg_find_element(modify_msg,
 								 req_msg->elements[el_idx].name) != NULL) {
-- 
2.25.1


From 39371352d8fc1d3ab0dd2baeacebd9ce48b4ef02 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 21 Jun 2022 14:49:51 +1200
Subject: [PATCH 04/99] CVE-2022-32746 s4:torture: Fix LDB flags comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(el->flags & LDB_FLAG_MOD_MASK) == 0

which is only true if none of the LDB_FLAG_MOD_* values are set. Correct
the expression to what it was probably intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/torture/drs/rpc/dssync.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/source4/torture/drs/rpc/dssync.c b/source4/torture/drs/rpc/dssync.c
index f63e8a4ae7d..047bee116e9 100644
--- a/source4/torture/drs/rpc/dssync.c
+++ b/source4/torture/drs/rpc/dssync.c
@@ -527,7 +527,9 @@ static bool test_analyse_objects(struct torture_context *tctx,
 				el = &new_msg->elements[idx];
 				a = dsdb_attribute_by_lDAPDisplayName(ldap_schema,
 				                                      el->name);
-				if (!(el->flags & (LDB_FLAG_MOD_ADD|LDB_FLAG_MOD_REPLACE))) {
+				if (LDB_FLAG_MOD_TYPE(el->flags) != LDB_FLAG_MOD_ADD &&
+				    LDB_FLAG_MOD_TYPE(el->flags) != LDB_FLAG_MOD_REPLACE)
+				{
 					/* DRS only value */
 					is_warning = false;
 				} else if (a->linkID & 1) {
-- 
2.25.1


From 27efd19085d01e1e3702afb5dfd82eaf72c13bf9 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 21 Jun 2022 15:22:47 +1200
Subject: [PATCH 05/99] CVE-2022-32746 s4/dsdb/acl: Fix LDB flags comparison

LDB_FLAG_MOD_* values are not actually flags, and the previous
comparison was equivalent to

(el->flags & LDB_FLAG_MOD_MASK) == 0

which is only true if none of the LDB_FLAG_MOD_* values are set, so we
would not successfully return if the element was a DELETE. Correct the
expression to what it was intended to be.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 selftest/knownfail.d/acl-spn-delete  | 1 -
 source4/dsdb/samdb/ldb_modules/acl.c | 5 +++--
 2 files changed, 3 insertions(+), 3 deletions(-)
 delete mode 100644 selftest/knownfail.d/acl-spn-delete

diff --git a/selftest/knownfail.d/acl-spn-delete b/selftest/knownfail.d/acl-spn-delete
deleted file mode 100644
index 32018413c49..00000000000
--- a/selftest/knownfail.d/acl-spn-delete
+++ /dev/null
@@ -1 +0,0 @@
-^samba4.ldap.acl.python.*__main__.AclSPNTests.test_delete_disallowed_spn\(
diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index 21e83276bfd..8016a2d4bd0 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -734,8 +734,9 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx,
 		 * If not add or replace (eg delete),
 		 * return success
 		 */
-		if ((el->flags
-		     & (LDB_FLAG_MOD_ADD|LDB_FLAG_MOD_REPLACE)) == 0) {
+		if (LDB_FLAG_MOD_TYPE(el->flags) != LDB_FLAG_MOD_ADD &&
+		    LDB_FLAG_MOD_TYPE(el->flags) != LDB_FLAG_MOD_REPLACE)
+		{
 			talloc_free(tmp_ctx);
 			return LDB_SUCCESS;
 		}
-- 
2.25.1


From 7c4439c7b7ff4caa7152f810ec9e83732fa70c3c Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Wed, 16 Feb 2022 12:43:52 +1300
Subject: [PATCH 06/99] CVE-2022-32746 ldb:rdn_name: Use LDB_FLAG_MOD_TYPE()
 for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 lib/ldb/modules/rdn_name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ldb/modules/rdn_name.c b/lib/ldb/modules/rdn_name.c
index e69ad9315ae..25cffe07591 100644
--- a/lib/ldb/modules/rdn_name.c
+++ b/lib/ldb/modules/rdn_name.c
@@ -545,7 +545,7 @@ static int rdn_name_modify(struct ldb_module *module, struct ldb_request *req)
 	if (e != NULL) {
 		ldb_asprintf_errstring(ldb, "Modify of 'distinguishedName' on %s not permitted, must use 'rename' operation instead",
 				       ldb_dn_get_linearized(req->op.mod.message->dn));
-		if (e->flags == LDB_FLAG_MOD_REPLACE) {
+		if (LDB_FLAG_MOD_TYPE(e->flags) == LDB_FLAG_MOD_REPLACE) {
 			return LDB_ERR_CONSTRAINT_VIOLATION;
 		} else {
 			return LDB_ERR_UNWILLING_TO_PERFORM;
-- 
2.25.1


From 47e2b1080e603b36b5d54a3e00f005983e6911e2 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 14 Jun 2022 19:49:19 +1200
Subject: [PATCH 07/99] CVE-2022-32746 s4/dsdb/repl_meta_data: Use
 LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index ab506cec488..29ffda75c87 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -3525,7 +3525,7 @@ static int replmd_modify(struct ldb_module *module, struct ldb_request *req)
 			return ldb_module_operr(module);
 		}
 
-		if (req->op.mod.message->elements[0].flags != LDB_FLAG_MOD_REPLACE) {
+		if (LDB_FLAG_MOD_TYPE(req->op.mod.message->elements[0].flags) != LDB_FLAG_MOD_REPLACE) {
 			return ldb_module_operr(module);
 		}
 
@@ -3558,11 +3558,11 @@ static int replmd_modify(struct ldb_module *module, struct ldb_request *req)
 			return ldb_module_operr(module);
 		}
 
-		if (req->op.mod.message->elements[0].flags != LDB_FLAG_MOD_DELETE) {
+		if (LDB_FLAG_MOD_TYPE(req->op.mod.message->elements[0].flags) != LDB_FLAG_MOD_DELETE) {
 			return ldb_module_operr(module);
 		}
 
-		if (req->op.mod.message->elements[1].flags != LDB_FLAG_MOD_ADD) {
+		if (LDB_FLAG_MOD_TYPE(req->op.mod.message->elements[1].flags) != LDB_FLAG_MOD_ADD) {
 			return ldb_module_operr(module);
 		}
 
@@ -3645,7 +3645,7 @@ static int replmd_modify(struct ldb_module *module, struct ldb_request *req)
 			return ldb_module_operr(module);
 		}
 
-		if (msg->elements[0].flags != LDB_FLAG_MOD_ADD) {
+		if (LDB_FLAG_MOD_TYPE(msg->elements[0].flags) != LDB_FLAG_MOD_ADD) {
 			talloc_free(ac);
 			return ldb_module_operr(module);
 		}
-- 
2.25.1


From f2b821f24e9b144c2cb1a9ec85f3bf1fdd2c2a8e Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 14 Jun 2022 21:11:33 +1200
Subject: [PATCH 08/99] CVE-2022-32746 s4/dsdb/tombstone_reanimate: Use
 LDB_FLAG_MOD_TYPE() for flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c b/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
index 64e05195798..5f8911c66be 100644
--- a/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
+++ b/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
@@ -104,7 +104,7 @@ static bool is_tombstone_reanimate_request(struct ldb_request *req,
 	if (el_dn == NULL) {
 		return false;
 	}
-	if (el_dn->flags != LDB_FLAG_MOD_REPLACE) {
+	if (LDB_FLAG_MOD_TYPE(el_dn->flags) != LDB_FLAG_MOD_REPLACE) {
 		return false;
 	}
 	if (el_dn->num_values != 1) {
@@ -117,7 +117,7 @@ static bool is_tombstone_reanimate_request(struct ldb_request *req,
 		return false;
 	}
 
-	if (el_deleted->flags != LDB_FLAG_MOD_DELETE) {
+	if (LDB_FLAG_MOD_TYPE(el_deleted->flags) != LDB_FLAG_MOD_DELETE) {
 		return false;
 	}
 
-- 
2.25.1


From ba27d18c2e8e1d0cf1828bb6d072489e5c6c9159 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Tue, 14 Jun 2022 21:12:39 +1200
Subject: [PATCH 09/99] CVE-2022-32746 s4/registry: Use LDB_FLAG_MOD_TYPE() for
 flags equality check

Now unrelated flags will no longer affect the result.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/lib/registry/ldb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/lib/registry/ldb.c b/source4/lib/registry/ldb.c
index e089355975b..db383a560da 100644
--- a/source4/lib/registry/ldb.c
+++ b/source4/lib/registry/ldb.c
@@ -859,7 +859,7 @@ static WERROR ldb_set_value(struct hive_key *parent,
 
 	/* Try first a "modify" and if this doesn't work do try an "add" */
 	for (i = 0; i < msg->num_elements; i++) {
-		if (msg->elements[i].flags != LDB_FLAG_MOD_DELETE) {
+		if (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) != LDB_FLAG_MOD_DELETE) {
 			msg->elements[i].flags = LDB_FLAG_MOD_REPLACE;
 		}
 	}
-- 
2.25.1


From 1294192b821d2d3af444b750baa75924042f1162 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Mon, 21 Feb 2022 16:10:32 +1300
Subject: [PATCH 10/99] CVE-2022-32746 ldb: Add flag to mark message element
 values as shared

When making a shallow copy of an ldb message, mark the message elements
of the copy as sharing their values with the message elements in the
original message.

This flag value will be heeded in the next commit.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 lib/ldb/common/ldb_msg.c     | 43 +++++++++++++++++++++++++++++++-----
 lib/ldb/include/ldb_module.h |  6 +++++
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c
index 57dfc5a04c2..2a9ce384bb9 100644
--- a/lib/ldb/common/ldb_msg.c
+++ b/lib/ldb/common/ldb_msg.c
@@ -833,11 +833,7 @@ void ldb_msg_sort_elements(struct ldb_message *msg)
 		       ldb_msg_element_compare_name);
 }
 
-/*
-  shallow copy a message - copying only the elements array so that the caller
-  can safely add new elements without changing the message
-*/
-struct ldb_message *ldb_msg_copy_shallow(TALLOC_CTX *mem_ctx,
+static struct ldb_message *ldb_msg_copy_shallow_impl(TALLOC_CTX *mem_ctx,
 					 const struct ldb_message *msg)
 {
 	struct ldb_message *msg2;
@@ -863,6 +859,35 @@ failed:
 	return NULL;
 }
 
+/*
+  shallow copy a message - copying only the elements array so that the caller
+  can safely add new elements without changing the message
+*/
+struct ldb_message *ldb_msg_copy_shallow(TALLOC_CTX *mem_ctx,
+					 const struct ldb_message *msg)
+{
+	struct ldb_message *msg2;
+	unsigned int i;
+
+	msg2 = ldb_msg_copy_shallow_impl(mem_ctx, msg);
+	if (msg2 == NULL) {
+		return NULL;
+	}
+
+	for (i = 0; i < msg2->num_elements; ++i) {
+		/*
+		 * Mark this message's elements as sharing their values with the
+		 * original message, so that we don't inadvertently modify or
+		 * free them. We don't mark the original message element as
+		 * shared, so the original message element should not be
+		 * modified or freed while the shallow copy lives.
+		 */
+		struct ldb_message_element *el = &msg2->elements[i];
+		el->flags |= LDB_FLAG_INTERNAL_SHARED_VALUES;
+	}
+
+        return msg2;
+}
 
 /*
   copy a message, allocating new memory for all parts
@@ -873,7 +898,7 @@ struct ldb_message *ldb_msg_copy(TALLOC_CTX *mem_ctx,
 	struct ldb_message *msg2;
 	unsigned int i, j;
 
-	msg2 = ldb_msg_copy_shallow(mem_ctx, msg);
+	msg2 = ldb_msg_copy_shallow_impl(mem_ctx, msg);
 	if (msg2 == NULL) return NULL;
 
 	if (msg2->dn != NULL) {
@@ -894,6 +919,12 @@ struct ldb_message *ldb_msg_copy(TALLOC_CTX *mem_ctx,
 				goto failed;
 			}
 		}
+
+                /*
+                 * Since we copied this element's values, we can mark them as
+                 * not shared.
+		 */
+		el->flags &= ~LDB_FLAG_INTERNAL_SHARED_VALUES;
 	}
 
 	return msg2;
diff --git a/lib/ldb/include/ldb_module.h b/lib/ldb/include/ldb_module.h
index 8c1e5ee7936..4c7c85a17f0 100644
--- a/lib/ldb/include/ldb_module.h
+++ b/lib/ldb/include/ldb_module.h
@@ -96,6 +96,12 @@ struct ldb_module;
  */
 #define LDB_FLAG_INTERNAL_FORCE_UNIQUE_INDEX 0x100
 
+/*
+ * indicates that this element's values are shared with another element (for
+ * example, in a shallow copy of an ldb_message) and should not be freed
+ */
+#define LDB_FLAG_INTERNAL_SHARED_VALUES 0x200
+
 /* an extended match rule that always fails to match */
 #define SAMBA_LDAP_MATCH_ALWAYS_FALSE "1.3.6.1.4.1.7165.4.5.1"
 
-- 
2.25.1


From 3a68efe1bbba4923f02b89a7f675398fbd73265e Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Wed, 16 Feb 2022 12:35:13 +1300
Subject: [PATCH 11/99] CVE-2022-32746 ldb: Ensure shallow copy modifications
 do not affect original message

Using the newly added ldb flag, we can now detect when a message has
been shallow-copied so that its elements share their values with the
original message elements. Then when adding values to the copied
message, we now make a copy of the shared values array first.

This should prevent a use-after-free that occurred in LDB modules when
new values were added to a shallow copy of a message by calling
talloc_realloc() on the original values array, invalidating the 'values'
pointer in the original message element. The original values pointer can
later be used in the database audit logging module which logs database
requests, and potentially cause a crash.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 lib/ldb/common/ldb_msg.c   | 52 ++++++++++++++++++++++++++++++++------
 lib/ldb/include/ldb.h      |  6 +++++
 source4/dsdb/common/util.c | 20 +++++----------
 3 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c
index 2a9ce384bb9..44d3b29e9a7 100644
--- a/lib/ldb/common/ldb_msg.c
+++ b/lib/ldb/common/ldb_msg.c
@@ -417,6 +417,47 @@ int ldb_msg_add(struct ldb_message *msg,
 	return LDB_SUCCESS;
 }
 
+/*
+ * add a value to a message element
+ */
+int ldb_msg_element_add_value(TALLOC_CTX *mem_ctx,
+			      struct ldb_message_element *el,
+			      const struct ldb_val *val)
+{
+	struct ldb_val *vals;
+
+	if (el->flags & LDB_FLAG_INTERNAL_SHARED_VALUES) {
+		/*
+		 * Another message is using this message element's values array,
+		 * so we don't want to make any modifications to the original
+		 * message, or potentially invalidate its own values by calling
+		 * talloc_realloc(). Make a copy instead.
+		 */
+		el->flags &= ~LDB_FLAG_INTERNAL_SHARED_VALUES;
+
+		vals = talloc_array(mem_ctx, struct ldb_val,
+				    el->num_values + 1);
+		if (vals == NULL) {
+			return LDB_ERR_OPERATIONS_ERROR;
+		}
+
+		if (el->values != NULL) {
+			memcpy(vals, el->values, el->num_values * sizeof(struct ldb_val));
+		}
+	} else {
+		vals = talloc_realloc(mem_ctx, el->values, struct ldb_val,
+				      el->num_values + 1);
+		if (vals == NULL) {
+			return LDB_ERR_OPERATIONS_ERROR;
+		}
+	}
+	el->values = vals;
+	el->values[el->num_values] = *val;
+	el->num_values++;
+
+	return LDB_SUCCESS;
+}
+
 /*
   add a value to a message
 */
@@ -426,7 +467,6 @@ int ldb_msg_add_value(struct ldb_message *msg,
 		      struct ldb_message_element **return_el)
 {
 	struct ldb_message_element *el;
-	struct ldb_val *vals;
 	int ret;
 
 	el = ldb_msg_find_element(msg, attr_name);
@@ -437,14 +477,10 @@ int ldb_msg_add_value(struct ldb_message *msg,
 		}
 	}
 
-	vals = talloc_realloc(msg->elements, el->values, struct ldb_val,
-			      el->num_values+1);
-	if (!vals) {
-		return LDB_ERR_OPERATIONS_ERROR;
+	ret = ldb_msg_element_add_value(msg->elements, el, val);
+	if (ret != LDB_SUCCESS) {
+		return ret;
 	}
-	el->values = vals;
-	el->values[el->num_values] = *val;
-	el->num_values++;
 
 	if (return_el) {
 		*return_el = el;
diff --git a/lib/ldb/include/ldb.h b/lib/ldb/include/ldb.h
index bc44157eaf4..129beefeaf5 100644
--- a/lib/ldb/include/ldb.h
+++ b/lib/ldb/include/ldb.h
@@ -1981,6 +1981,12 @@ int ldb_msg_add_empty(struct ldb_message *msg,
 		int flags,
 		struct ldb_message_element **return_el);
 
+/**
+   add a value to a message element
+*/
+int ldb_msg_element_add_value(TALLOC_CTX *mem_ctx,
+			      struct ldb_message_element *el,
+			      const struct ldb_val *val);
 /**
    add a element to a ldb_message
 */
diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index 9b4afa45215..721e1e9d41d 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -815,7 +815,7 @@ int samdb_msg_add_addval(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx,
 			 const char *value)
 {
 	struct ldb_message_element *el;
-	struct ldb_val val, *vals;
+	struct ldb_val val;
 	char *v;
 	unsigned int i;
 	bool found = false;
@@ -850,14 +850,10 @@ int samdb_msg_add_addval(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	vals = talloc_realloc(msg->elements, el->values, struct ldb_val,
-			      el->num_values + 1);
-	if (vals == NULL) {
+	ret = ldb_msg_element_add_value(msg->elements, el, &val);
+	if (ret != LDB_SUCCESS) {
 		return ldb_oom(sam_ldb);
 	}
-	el->values = vals;
-	el->values[el->num_values] = val;
-	++(el->num_values);
 
 	return LDB_SUCCESS;
 }
@@ -871,7 +867,7 @@ int samdb_msg_add_delval(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx,
 			 const char *value)
 {
 	struct ldb_message_element *el;
-	struct ldb_val val, *vals;
+	struct ldb_val val;
 	char *v;
 	unsigned int i;
 	bool found = false;
@@ -906,14 +902,10 @@ int samdb_msg_add_delval(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	vals = talloc_realloc(msg->elements, el->values, struct ldb_val,
-			      el->num_values + 1);
-	if (vals == NULL) {
+	ret = ldb_msg_element_add_value(msg->elements, el, &val);
+	if (ret != LDB_SUCCESS) {
 		return ldb_oom(sam_ldb);
 	}
-	el->values = vals;
-	el->values[el->num_values] = val;
-	++(el->num_values);
 
 	return LDB_SUCCESS;
 }
-- 
2.25.1


From a25b97d0540fdb5a4a75fd85807d8963f14b607d Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Wed, 16 Feb 2022 16:30:03 +1300
Subject: [PATCH 12/99] CVE-2022-32746 ldb: Add functions for appending to an
 ldb_message

Currently, there are many places where we use ldb_msg_add_empty() to add
an empty element to a message, and then call ldb_msg_add_value() or
similar to add values to that element. However, this performs an
unnecessary search of the message's elements to locate the new element.
Moreover, if an element with the same attribute name already exists
earlier in the message, the values will be added to that element,
instead of to the intended newly added element.

A similar pattern exists where we add values to a message, and then call
ldb_msg_find_element() to locate that message element and sets its flags
to (e.g.) LDB_FLAG_MOD_REPLACE. This also performs an unnecessary
search, and may locate the wrong message element for setting the flags.

To avoid these problems, add functions for appending a value to a
message, so that a particular value can be added to the end of a message
in a single operation.

For ADD requests, it is important that no two message elements share the
same attribute name, otherwise things will break. (Normally,
ldb_msg_normalize() is called before processing the request to help
ensure this.) Thus, we must be careful not to append an attribute to an
ADD message, unless we are sure (e.g. through ldb_msg_find_element())
that an existing element for that attribute is not present.

These functions will be used in the next commit.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 lib/ldb/common/ldb_msg.c | 165 ++++++++++++++++++++++++++++++++++++++-
 lib/ldb/include/ldb.h    |  24 ++++++
 2 files changed, 185 insertions(+), 4 deletions(-)

diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c
index 44d3b29e9a7..9cd7998e21c 100644
--- a/lib/ldb/common/ldb_msg.c
+++ b/lib/ldb/common/ldb_msg.c
@@ -509,12 +509,15 @@ int ldb_msg_add_steal_value(struct ldb_message *msg,
 
 
 /*
-  add a string element to a message
+  add a string element to a message, specifying flags
 */
-int ldb_msg_add_string(struct ldb_message *msg,
-		       const char *attr_name, const char *str)
+int ldb_msg_add_string_flags(struct ldb_message *msg,
+			     const char *attr_name, const char *str,
+			     int flags)
 {
 	struct ldb_val val;
+	int ret;
+	struct ldb_message_element *el = NULL;
 
 	val.data = discard_const_p(uint8_t, str);
 	val.length = strlen(str);
@@ -524,7 +527,25 @@ int ldb_msg_add_string(struct ldb_message *msg,
 		return LDB_SUCCESS;
 	}
 
-	return ldb_msg_add_value(msg, attr_name, &val, NULL);
+	ret = ldb_msg_add_value(msg, attr_name, &val, &el);
+	if (ret != LDB_SUCCESS) {
+		return ret;
+	}
+
+	if (flags != 0) {
+		el->flags = flags;
+	}
+
+	return LDB_SUCCESS;
+}
+
+/*
+  add a string element to a message
+*/
+int ldb_msg_add_string(struct ldb_message *msg,
+		       const char *attr_name, const char *str)
+{
+	return ldb_msg_add_string_flags(msg, attr_name, str, 0);
 }
 
 /*
@@ -586,6 +607,142 @@ int ldb_msg_add_fmt(struct ldb_message *msg,
 	return ldb_msg_add_steal_value(msg, attr_name, &val);
 }
 
+static int ldb_msg_append_value_impl(struct ldb_message *msg,
+				     const char *attr_name,
+				     const struct ldb_val *val,
+				     int flags,
+				     struct ldb_message_element **return_el)
+{
+	struct ldb_message_element *el = NULL;
+	int ret;
+
+	ret = ldb_msg_add_empty(msg, attr_name, flags, &el);
+	if (ret != LDB_SUCCESS) {
+		return ret;
+	}
+
+	ret = ldb_msg_element_add_value(msg->elements, el, val);
+	if (ret != LDB_SUCCESS) {
+		return ret;
+	}
+
+	if (return_el != NULL) {
+		*return_el = el;
+	}
+
+	return LDB_SUCCESS;
+}
+
+/*
+  append a value to a message
+*/
+int ldb_msg_append_value(struct ldb_message *msg,
+			 const char *attr_name,
+			 const struct ldb_val *val,
+			 int flags)
+{
+	return ldb_msg_append_value_impl(msg, attr_name, val, flags, NULL);
+}
+
+/*
+  append a value to a message, stealing it into the 'right' place
+*/
+int ldb_msg_append_steal_value(struct ldb_message *msg,
+			       const char *attr_name,
+			       struct ldb_val *val,
+			       int flags)
+{
+	int ret;
+	struct ldb_message_element *el = NULL;
+
+	ret = ldb_msg_append_value_impl(msg, attr_name, val, flags, &el);
+	if (ret == LDB_SUCCESS) {
+		talloc_steal(el->values, val->data);
+	}
+	return ret;
+}
+
+/*
+  append a string element to a message, stealing it into the 'right' place
+*/
+int ldb_msg_append_steal_string(struct ldb_message *msg,
+				const char *attr_name, char *str,
+				int flags)
+{
+	struct ldb_val val;
+
+	val.data = (uint8_t *)str;
+	val.length = strlen(str);
+
+	if (val.length == 0) {
+		/* allow empty strings as non-existent attributes */
+		return LDB_SUCCESS;
+	}
+
+	return ldb_msg_append_steal_value(msg, attr_name, &val, flags);
+}
+
+/*
+  append a string element to a message
+*/
+int ldb_msg_append_string(struct ldb_message *msg,
+			  const char *attr_name, const char *str, int flags)
+{
+	struct ldb_val val;
+
+	val.data = discard_const_p(uint8_t, str);
+	val.length = strlen(str);
+
+	if (val.length == 0) {
+		/* allow empty strings as non-existent attributes */
+		return LDB_SUCCESS;
+	}
+
+	return ldb_msg_append_value(msg, attr_name, &val, flags);
+}
+
+/*
+  append a DN element to a message
+  WARNING: this uses the linearized string from the dn, and does not
+  copy the string.
+*/
+int ldb_msg_append_linearized_dn(struct ldb_message *msg, const char *attr_name,
+				 struct ldb_dn *dn, int flags)
+{
+	char *str = ldb_dn_alloc_linearized(msg, dn);
+
+	if (str == NULL) {
+		/* we don't want to have unknown DNs added */
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
+	return ldb_msg_append_steal_string(msg, attr_name, str, flags);
+}
+
+/*
+  append a printf formatted element to a message
+*/
+int ldb_msg_append_fmt(struct ldb_message *msg, int flags,
+		       const char *attr_name, const char *fmt, ...)
+{
+	struct ldb_val val;
+	va_list ap;
+	char *str = NULL;
+
+	va_start(ap, fmt);
+	str = talloc_vasprintf(msg, fmt, ap);
+	va_end(ap);
+
+	if (str == NULL) {
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
+	val.data   = (uint8_t *)str;
+	val.length = strlen(str);
+
+	return ldb_msg_append_steal_value(msg, attr_name, &val, flags);
+}
+
 /*
   compare two ldb_message_element structures
   assumes case sensitive comparison
diff --git a/lib/ldb/include/ldb.h b/lib/ldb/include/ldb.h
index 129beefeaf5..63d8aedd672 100644
--- a/lib/ldb/include/ldb.h
+++ b/lib/ldb/include/ldb.h
@@ -2002,12 +2002,36 @@ int ldb_msg_add_steal_value(struct ldb_message *msg,
 		      struct ldb_val *val);
 int ldb_msg_add_steal_string(struct ldb_message *msg,
 			     const char *attr_name, char *str);
+int ldb_msg_add_string_flags(struct ldb_message *msg,
+			     const char *attr_name, const char *str,
+			     int flags);
 int ldb_msg_add_string(struct ldb_message *msg,
 		       const char *attr_name, const char *str);
 int ldb_msg_add_linearized_dn(struct ldb_message *msg, const char *attr_name,
 			      struct ldb_dn *dn);
 int ldb_msg_add_fmt(struct ldb_message *msg,
 		    const char *attr_name, const char *fmt, ...) PRINTF_ATTRIBUTE(3,4);
+/**
+   append a element to a ldb_message
+*/
+int ldb_msg_append_value(struct ldb_message *msg,
+			 const char *attr_name,
+			 const struct ldb_val *val,
+			 int flags);
+int ldb_msg_append_steal_value(struct ldb_message *msg,
+			       const char *attr_name,
+			       struct ldb_val *val,
+			       int flags);
+int ldb_msg_append_steal_string(struct ldb_message *msg,
+				const char *attr_name, char *str,
+				int flags);
+int ldb_msg_append_string(struct ldb_message *msg,
+			  const char *attr_name, const char *str,
+			  int flags);
+int ldb_msg_append_linearized_dn(struct ldb_message *msg, const char *attr_name,
+				 struct ldb_dn *dn, int flags);
+int ldb_msg_append_fmt(struct ldb_message *msg, int flags,
+		       const char *attr_name, const char *fmt, ...) PRINTF_ATTRIBUTE(4,5);
 
 /**
    compare two message elements - return 0 on match
-- 
2.25.1


From 0446581bcce7c2d7f5ec22d8510a6e2069463d39 Mon Sep 17 00:00:00 2001
From: Joseph Sutton <josephsutton@catalyst.net.nz>
Date: Mon, 21 Feb 2022 16:27:37 +1300
Subject: [PATCH 13/99] CVE-2022-32746 ldb: Make use of functions for appending
 to an ldb_message

This aims to minimise usage of the error-prone pattern of searching for
a just-added message element in order to make modifications to it (and
potentially finding the wrong element).

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15009

Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 lib/ldb/ldb_map/ldb_map.c                     |   5 +-
 lib/ldb/ldb_map/ldb_map_inbound.c             |   9 +-
 lib/ldb/modules/rdn_name.c                    |  22 +---
 source3/passdb/pdb_samba_dsdb.c               |  14 +--
 source4/dns_server/dnsserver_common.c         |  12 +-
 source4/dsdb/common/util.c                    | 114 ++++++++++++++----
 source4/dsdb/samdb/ldb_modules/descriptor.c   |  10 +-
 source4/dsdb/samdb/ldb_modules/objectguid.c   |  20 +--
 .../dsdb/samdb/ldb_modules/partition_init.c   |  14 +--
 .../dsdb/samdb/ldb_modules/repl_meta_data.c   |  24 +---
 source4/dsdb/samdb/ldb_modules/samldb.c       |  78 +++++-------
 .../samdb/ldb_modules/tombstone_reanimate.c   |  12 +-
 source4/nbt_server/wins/winsdb.c              |  13 +-
 source4/rpc_server/lsa/dcesrv_lsa.c           |  55 +++------
 source4/winbind/idmap.c                       |  10 +-
 15 files changed, 183 insertions(+), 229 deletions(-)

diff --git a/lib/ldb/ldb_map/ldb_map.c b/lib/ldb/ldb_map/ldb_map.c
index b453dff80d2..c7b0c228631 100644
--- a/lib/ldb/ldb_map/ldb_map.c
+++ b/lib/ldb/ldb_map/ldb_map.c
@@ -946,10 +946,7 @@ struct ldb_request *map_build_fixup_req(struct map_context *ac,
 	if ( ! dn || ! ldb_dn_validate(msg->dn)) {
 		goto failed;
 	}
-	if (ldb_msg_add_empty(msg, IS_MAPPED, LDB_FLAG_MOD_REPLACE, NULL) != 0) {
-		goto failed;
-	}
-	if (ldb_msg_add_string(msg, IS_MAPPED, dn) != 0) {
+	if (ldb_msg_append_string(msg, IS_MAPPED, dn, LDB_FLAG_MOD_REPLACE) != 0) {
 		goto failed;
 	}
 
diff --git a/lib/ldb/ldb_map/ldb_map_inbound.c b/lib/ldb/ldb_map/ldb_map_inbound.c
index 324295737da..50b9427c26c 100644
--- a/lib/ldb/ldb_map/ldb_map_inbound.c
+++ b/lib/ldb/ldb_map/ldb_map_inbound.c
@@ -569,12 +569,9 @@ static int map_modify_do_local(struct map_context *ac)
 		/* No local record present, add it instead */
 		/* Add local 'IS_MAPPED' */
 		/* TODO: use GUIDs here instead */
-		if (ldb_msg_add_empty(ac->local_msg, IS_MAPPED,
-					LDB_FLAG_MOD_ADD, NULL) != 0) {
-			return LDB_ERR_OPERATIONS_ERROR;
-		}
-		ret = ldb_msg_add_linearized_dn(ac->local_msg, IS_MAPPED,
-						ac->remote_req->op.mod.message->dn);
+		ret = ldb_msg_append_linearized_dn(ac->local_msg, IS_MAPPED,
+						   ac->remote_req->op.mod.message->dn,
+						   LDB_FLAG_MOD_ADD);
 		if (ret != 0) {
 			return LDB_ERR_OPERATIONS_ERROR;
 		}
diff --git a/lib/ldb/modules/rdn_name.c b/lib/ldb/modules/rdn_name.c
index 25cffe07591..3cb62bf567b 100644
--- a/lib/ldb/modules/rdn_name.c
+++ b/lib/ldb/modules/rdn_name.c
@@ -308,16 +308,10 @@ static int rdn_rename_callback(struct ldb_request *req, struct ldb_reply *ares)
 	}
 	rdn_val = ldb_val_dup(msg, rdn_val_p);
 
-	if (ldb_msg_add_empty(msg, rdn_name, LDB_FLAG_MOD_REPLACE, NULL) != 0) {
+	if (ldb_msg_append_value(msg, rdn_name, &rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		goto error;
 	}
-	if (ldb_msg_add_value(msg, rdn_name, &rdn_val, NULL) != 0) {
-		goto error;
-	}
-	if (ldb_msg_add_empty(msg, "name", LDB_FLAG_MOD_REPLACE, NULL) != 0) {
-		goto error;
-	}
-	if (ldb_msg_add_value(msg, "name", &rdn_val, NULL) != 0) {
+	if (ldb_msg_append_value(msg, "name", &rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		goto error;
 	}
 
@@ -466,11 +460,7 @@ static int rdn_name_modify(struct ldb_module *module, struct ldb_request *req)
 		if (ret != 0) {
 			return ldb_module_oom(module);
 		}
-		ret = ldb_msg_add_empty(msg, rdn_name, LDB_FLAG_MOD_ADD, NULL);
-		if (ret != 0) {
-			return ldb_module_oom(module);
-		}
-		ret = ldb_msg_add_value(msg, rdn_name, &rdn_val, NULL);
+		ret = ldb_msg_append_value(msg, rdn_name, &rdn_val, LDB_FLAG_MOD_ADD);
 		if (ret != 0) {
 			return ldb_module_oom(module);
 		}
@@ -479,11 +469,7 @@ static int rdn_name_modify(struct ldb_module *module, struct ldb_request *req)
 		if (ret != 0) {
 			return ldb_module_oom(module);
 		}
-		ret = ldb_msg_add_empty(msg, "name", LDB_FLAG_MOD_ADD, NULL);
-		if (ret != 0) {
-			return ldb_module_oom(module);
-		}
-		ret = ldb_msg_add_value(msg, "name", &rdn_val, NULL);
+		ret = ldb_msg_append_value(msg, "name", &rdn_val, LDB_FLAG_MOD_ADD);
 		if (ret != 0) {
 			return ldb_module_oom(module);
 		}
diff --git a/source3/passdb/pdb_samba_dsdb.c b/source3/passdb/pdb_samba_dsdb.c
index 4f1d2f697f0..d9c31e57186 100644
--- a/source3/passdb/pdb_samba_dsdb.c
+++ b/source3/passdb/pdb_samba_dsdb.c
@@ -2776,18 +2776,10 @@ static bool pdb_samba_dsdb_set_trusteddom_pw(struct pdb_methods *m,
 	}
 
 	msg->num_elements = 0;
-	ret = ldb_msg_add_empty(msg, "trustAuthOutgoing",
-				LDB_FLAG_MOD_REPLACE, NULL);
+	ret = ldb_msg_append_value(msg, "trustAuthOutgoing",
+				   &new_val, LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
-		DEBUG(0, ("ldb_msg_add_empty() failed\n"));
-		TALLOC_FREE(tmp_ctx);
-		ldb_transaction_cancel(state->ldb);
-		return false;
-	}
-	ret = ldb_msg_add_value(msg, "trustAuthOutgoing",
-				&new_val, NULL);
-	if (ret != LDB_SUCCESS) {
-		DEBUG(0, ("ldb_msg_add_value() failed\n"));
+		DEBUG(0, ("ldb_msg_append_value() failed\n"));
 		TALLOC_FREE(tmp_ctx);
 		ldb_transaction_cancel(state->ldb);
 		return false;
diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c
index d1f896d6755..2b4d719b342 100644
--- a/source4/dns_server/dnsserver_common.c
+++ b/source4/dns_server/dnsserver_common.c
@@ -1122,15 +1122,9 @@ WERROR dns_common_replace(struct ldb_context *samdb,
 	}
 
 	if (was_tombstoned || become_tombstoned) {
-		ret = ldb_msg_add_empty(msg, "dNSTombstoned",
-					LDB_FLAG_MOD_REPLACE, NULL);
-		if (ret != LDB_SUCCESS) {
-			werr = DNS_ERR(SERVER_FAILURE);
-			goto exit;
-		}
-
-		ret = ldb_msg_add_fmt(msg, "dNSTombstoned", "%s",
-				      become_tombstoned ? "TRUE" : "FALSE");
+		ret = ldb_msg_append_fmt(msg, LDB_FLAG_MOD_REPLACE,
+					 "dNSTombstoned", "%s",
+					 become_tombstoned ? "TRUE" : "FALSE");
 		if (ret != LDB_SUCCESS) {
 			werr = DNS_ERR(SERVER_FAILURE);
 			goto exit;
diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index 721e1e9d41d..beb2883cad2 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -923,6 +923,16 @@ int samdb_msg_add_int(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct l
 	return ldb_msg_add_string(msg, attr_name, s);
 }
 
+int samdb_msg_add_int_flags(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+			    const char *attr_name, int v, int flags)
+{
+	const char *s = talloc_asprintf(mem_ctx, "%d", v);
+	if (s == NULL) {
+		return ldb_oom(sam_ldb);
+	}
+	return ldb_msg_add_string_flags(msg, attr_name, s, flags);
+}
+
 /*
  * Add an unsigned int element to a message
  *
@@ -941,6 +951,12 @@ int samdb_msg_add_uint(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct
 	return samdb_msg_add_int(sam_ldb, mem_ctx, msg, attr_name, (int)v);
 }
 
+int samdb_msg_add_uint_flags(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+			     const char *attr_name, unsigned int v, int flags)
+{
+	return samdb_msg_add_int_flags(sam_ldb, mem_ctx, msg, attr_name, (int)v, flags);
+}
+
 /*
   add a (signed) int64_t element to a message
 */
@@ -972,6 +988,68 @@ int samdb_msg_add_uint64(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struc
 	return samdb_msg_add_int64(sam_ldb, mem_ctx, msg, attr_name, (int64_t)v);
 }
 
+/*
+  append a int element to a message
+*/
+int samdb_msg_append_int(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+		      const char *attr_name, int v, int flags)
+{
+	const char *s = talloc_asprintf(mem_ctx, "%d", v);
+	if (s == NULL) {
+		return ldb_oom(sam_ldb);
+	}
+	return ldb_msg_append_string(msg, attr_name, s, flags);
+}
+
+/*
+ * Append an unsigned int element to a message
+ *
+ * The issue here is that we have not yet first cast to int32_t explicitly,
+ * before we cast to an signed int to printf() into the %d or cast to a
+ * int64_t before we then cast to a long long to printf into a %lld.
+ *
+ * There are *no* unsigned integers in Active Directory LDAP, even the RID
+ * allocations and ms-DS-Secondary-KrbTgt-Number are *signed* quantities.
+ * (See the schema, and the syntax definitions in schema_syntax.c).
+ *
+ */
+int samdb_msg_append_uint(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+			  const char *attr_name, unsigned int v, int flags)
+{
+	return samdb_msg_append_int(sam_ldb, mem_ctx, msg, attr_name, (int)v, flags);
+}
+
+/*
+  append a (signed) int64_t element to a message
+*/
+int samdb_msg_append_int64(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+			   const char *attr_name, int64_t v, int flags)
+{
+	const char *s = talloc_asprintf(mem_ctx, "%lld", (long long)v);
+	if (s == NULL) {
+		return ldb_oom(sam_ldb);
+	}
+	return ldb_msg_append_string(msg, attr_name, s, flags);
+}
+
+/*
+ * Append an unsigned int64_t (uint64_t) element to a message
+ *
+ * The issue here is that we have not yet first cast to int32_t explicitly,
+ * before we cast to an signed int to printf() into the %d or cast to a
+ * int64_t before we then cast to a long long to printf into a %lld.
+ *
+ * There are *no* unsigned integers in Active Directory LDAP, even the RID
+ * allocations and ms-DS-Secondary-KrbTgt-Number are *signed* quantities.
+ * (See the schema, and the syntax definitions in schema_syntax.c).
+ *
+ */
+int samdb_msg_append_uint64(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, struct ldb_message *msg,
+			    const char *attr_name, uint64_t v, int flags)
+{
+	return samdb_msg_append_int64(sam_ldb, mem_ctx, msg, attr_name, (int64_t)v, flags);
+}
+
 /*
   add a samr_Password element to a message
 */
@@ -2813,15 +2891,8 @@ NTSTATUS samdb_set_password_sid(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
 		tdo_msg->num_elements = 0;
 		TALLOC_FREE(tdo_msg->elements);
 
-		ret = ldb_msg_add_empty(tdo_msg, "trustAuthIncoming",
-					LDB_FLAG_MOD_REPLACE, NULL);
-		if (ret != LDB_SUCCESS) {
-			ldb_transaction_cancel(ldb);
-			TALLOC_FREE(frame);
-			return NT_STATUS_NO_MEMORY;
-		}
-		ret = ldb_msg_add_value(tdo_msg, "trustAuthIncoming",
-					&new_val, NULL);
+		ret = ldb_msg_append_value(tdo_msg, "trustAuthIncoming",
+					   &new_val, LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			ldb_transaction_cancel(ldb);
 			TALLOC_FREE(frame);
@@ -3186,6 +3257,7 @@ int dsdb_find_guid_by_dn(struct ldb_context *ldb,
 /*
  adds the given GUID to the given ldb_message. This value is added
  for the given attr_name (may be either "objectGUID" or "parentGUID").
+ This function is used in processing 'add' requests.
  */
 int dsdb_msg_add_guid(struct ldb_message *msg,
 		struct GUID *guid,
@@ -5655,7 +5727,8 @@ int dsdb_user_obj_set_defaults(struct ldb_context *ldb,
 }
 
 /**
- * Sets 'sAMAccountType on user object based on userAccountControl
+ * Sets 'sAMAccountType on user object based on userAccountControl.
+ * This function is used in processing both 'add' and 'modify' requests.
  * @param ldb Current ldb_context
  * @param usr_obj ldb_message representing User object
  * @param user_account_control Value for userAccountControl flags
@@ -5667,21 +5740,19 @@ int dsdb_user_obj_set_account_type(struct ldb_context *ldb, struct ldb_message *
 {
 	int ret;
 	uint32_t account_type;
-	struct ldb_message_element *el;
 
 	account_type = ds_uf2atype(user_account_control);
 	if (account_type == 0) {
 		ldb_set_errstring(ldb, "dsdb: Unrecognized account type!");
 		return LDB_ERR_UNWILLING_TO_PERFORM;
 	}
-	ret = samdb_msg_add_uint(ldb, usr_obj, usr_obj,
-				 "sAMAccountType",
-				 account_type);
+	ret = samdb_msg_add_uint_flags(ldb, usr_obj, usr_obj,
+				       "sAMAccountType",
+				       account_type,
+				       LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
-	el = ldb_msg_find_element(usr_obj, "sAMAccountType");
-	el->flags = LDB_FLAG_MOD_REPLACE;
 
 	if (account_type_p) {
 		*account_type_p = account_type;
@@ -5691,7 +5762,8 @@ int dsdb_user_obj_set_account_type(struct ldb_context *ldb, struct ldb_message *
 }
 
 /**
- * Determine and set primaryGroupID based on userAccountControl value
+ * Determine and set primaryGroupID based on userAccountControl value.
+ * This function is used in processing both 'add' and 'modify' requests.
  * @param ldb Current ldb_context
  * @param usr_obj ldb_message representing User object
  * @param user_account_control Value for userAccountControl flags
@@ -5703,17 +5775,15 @@ int dsdb_user_obj_set_primary_group_id(struct ldb_context *ldb, struct ldb_messa
 {
 	int ret;
 	uint32_t rid;
-	struct ldb_message_element *el;
 
 	rid = ds_uf2prim_group_rid(user_account_control);
 
-	ret = samdb_msg_add_uint(ldb, usr_obj, usr_obj,
-				 "primaryGroupID", rid);
+	ret = samdb_msg_add_uint_flags(ldb, usr_obj, usr_obj,
+				       "primaryGroupID", rid,
+				       LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
-	el = ldb_msg_find_element(usr_obj, "primaryGroupID");
-	el->flags = LDB_FLAG_MOD_REPLACE;
 
 	if (group_rid_p) {
 		*group_rid_p = rid;
diff --git a/source4/dsdb/samdb/ldb_modules/descriptor.c b/source4/dsdb/samdb/ldb_modules/descriptor.c
index 8a4c2c3591f..10ed577328d 100644
--- a/source4/dsdb/samdb/ldb_modules/descriptor.c
+++ b/source4/dsdb/samdb/ldb_modules/descriptor.c
@@ -861,14 +861,8 @@ static int descriptor_modify(struct ldb_module *module, struct ldb_request *req)
 			return ldb_module_done(req, NULL, NULL, LDB_SUCCESS);
 		}
 
-		ret = ldb_msg_add_empty(msg, "nTSecurityDescriptor",
-					LDB_FLAG_MOD_REPLACE,
-					&sd_element);
-		if (ret != LDB_SUCCESS) {
-			return ldb_oom(ldb);
-		}
-		ret = ldb_msg_add_value(msg, "nTSecurityDescriptor",
-					sd, NULL);
+		ret = ldb_msg_append_value(msg, "nTSecurityDescriptor",
+					   sd, LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ldb_oom(ldb);
 		}
diff --git a/source4/dsdb/samdb/ldb_modules/objectguid.c b/source4/dsdb/samdb/ldb_modules/objectguid.c
index bc3260cf0d8..0fe995a5763 100644
--- a/source4/dsdb/samdb/ldb_modules/objectguid.c
+++ b/source4/dsdb/samdb/ldb_modules/objectguid.c
@@ -41,7 +41,6 @@
 */
 static int add_time_element(struct ldb_message *msg, const char *attr, time_t t)
 {
-	struct ldb_message_element *el;
 	char *s;
 	int ret;
 
@@ -54,16 +53,13 @@ static int add_time_element(struct ldb_message *msg, const char *attr, time_t t)
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
 
-	ret = ldb_msg_add_string(msg, attr, s);
+	/* always set as replace. This works because on add ops, the flag
+	   is ignored */
+	ret = ldb_msg_append_string(msg, attr, s, LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
 
-	el = ldb_msg_find_element(msg, attr);
-	/* always set as replace. This works because on add ops, the flag
-	   is ignored */
-	el->flags = LDB_FLAG_MOD_REPLACE;
-
 	return LDB_SUCCESS;
 }
 
@@ -73,23 +69,19 @@ static int add_time_element(struct ldb_message *msg, const char *attr, time_t t)
 static int add_uint64_element(struct ldb_context *ldb, struct ldb_message *msg,
 			      const char *attr, uint64_t v)
 {
-	struct ldb_message_element *el;
 	int ret;
 
 	if (ldb_msg_find_element(msg, attr) != NULL) {
 		return LDB_SUCCESS;
 	}
 
-	ret = samdb_msg_add_uint64(ldb, msg, msg, attr, v);
+	/* always set as replace. This works because on add ops, the flag
+	   is ignored */
+	ret = samdb_msg_append_uint64(ldb, msg, msg, attr, v, LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
 
-	el = ldb_msg_find_element(msg, attr);
-	/* always set as replace. This works because on add ops, the flag
-	   is ignored */
-	el->flags = LDB_FLAG_MOD_REPLACE;
-
 	return LDB_SUCCESS;
 }
 
diff --git a/source4/dsdb/samdb/ldb_modules/partition_init.c b/source4/dsdb/samdb/ldb_modules/partition_init.c
index 58c65ccedd0..484b5bffb27 100644
--- a/source4/dsdb/samdb/ldb_modules/partition_init.c
+++ b/source4/dsdb/samdb/ldb_modules/partition_init.c
@@ -742,10 +742,6 @@ int partition_create(struct ldb_module *module, struct ldb_request *req)
 		}
 		
 		mod_msg->dn = ldb_dn_new(mod_msg, ldb, DSDB_PARTITION_DN);
-		ret = ldb_msg_add_empty(mod_msg, DSDB_PARTITION_ATTR, LDB_FLAG_MOD_ADD, NULL);
-		if (ret != LDB_SUCCESS) {
-			return ret;
-		}
 		
 		casefold_dn = ldb_dn_get_casefold(dn);
 		
@@ -785,18 +781,16 @@ int partition_create(struct ldb_module *module, struct ldb_request *req)
 		}
 		partition_record = talloc_asprintf(mod_msg, "%s:%s", casefold_dn, filename);
 
-		ret = ldb_msg_add_steal_string(mod_msg, DSDB_PARTITION_ATTR, partition_record);
+		ret = ldb_msg_append_steal_string(mod_msg, DSDB_PARTITION_ATTR, partition_record,
+						  LDB_FLAG_MOD_ADD);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
 
 		if (ldb_request_get_control(req, DSDB_CONTROL_PARTIAL_REPLICA)) {
 			/* this new partition is a partial replica */
-			ret = ldb_msg_add_empty(mod_msg, "partialReplica", LDB_FLAG_MOD_ADD, NULL);
-			if (ret != LDB_SUCCESS) {
-				return ret;
-			}
-			ret = ldb_msg_add_fmt(mod_msg, "partialReplica", "%s", ldb_dn_get_linearized(dn));
+			ret = ldb_msg_append_fmt(mod_msg, LDB_FLAG_MOD_ADD,
+						 "partialReplica", "%s", ldb_dn_get_linearized(dn));
 			if (ret != LDB_SUCCESS) {
 				return ret;
 			}
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 29ffda75c87..eec1e639856 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -3888,22 +3888,12 @@ static int replmd_rename_callback(struct ldb_request *req, struct ldb_reply *are
 				       ldb_operr(ldb));
 	}
 
-	if (ldb_msg_add_empty(msg, rdn_name, LDB_FLAG_MOD_REPLACE, NULL) != 0) {
+	if (ldb_msg_append_value(msg, rdn_name, rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		talloc_free(ares);
 		return ldb_module_done(ac->req, NULL, NULL,
 				       ldb_oom(ldb));
 	}
-	if (ldb_msg_add_value(msg, rdn_name, rdn_val, NULL) != 0) {
-		talloc_free(ares);
-		return ldb_module_done(ac->req, NULL, NULL,
-				       ldb_oom(ldb));
-	}
-	if (ldb_msg_add_empty(msg, "name", LDB_FLAG_MOD_REPLACE, NULL) != 0) {
-		talloc_free(ares);
-		return ldb_module_done(ac->req, NULL, NULL,
-				       ldb_oom(ldb));
-	}
-	if (ldb_msg_add_value(msg, "name", rdn_val, NULL) != 0) {
+	if (ldb_msg_append_value(msg, "name", rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		talloc_free(ares);
 		return ldb_module_done(ac->req, NULL, NULL,
 				       ldb_oom(ldb));
@@ -5161,16 +5151,10 @@ static int replmd_name_modify(struct replmd_replicated_request *ar,
 		goto failed;
 	}
 
-	if (ldb_msg_add_empty(msg, rdn_name, LDB_FLAG_MOD_REPLACE, NULL) != 0) {
-		goto failed;
-	}
-	if (ldb_msg_add_value(msg, rdn_name, rdn_val, NULL) != 0) {
-		goto failed;
-	}
-	if (ldb_msg_add_empty(msg, "name", LDB_FLAG_MOD_REPLACE, NULL) != 0) {
+	if (ldb_msg_append_value(msg, rdn_name, rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		goto failed;
 	}
-	if (ldb_msg_add_value(msg, "name", rdn_val, NULL) != 0) {
+	if (ldb_msg_append_value(msg, "name", rdn_val, LDB_FLAG_MOD_REPLACE) != 0) {
 		goto failed;
 	}
 
diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
index 24971d521aa..b89d93910fd 100644
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
@@ -1103,14 +1103,11 @@ static int samldb_rodc_add(struct samldb_ctx *ac)
 	return LDB_ERR_OTHER;
 
 found:
-	ret = ldb_msg_add_empty(ac->msg, "msDS-SecondaryKrbTgtNumber",
-				LDB_FLAG_INTERNAL_DISABLE_VALIDATION, NULL);
-	if (ret != LDB_SUCCESS) {
-		return ldb_operr(ldb);
-	}
 
-	ret = samdb_msg_add_uint(ldb, ac->msg, ac->msg,
-				 "msDS-SecondaryKrbTgtNumber", krbtgt_number);
+	ldb_msg_remove_attr(ac->msg, "msDS-SecondaryKrbTgtNumber");
+	ret = samdb_msg_append_uint(ldb, ac->msg, ac->msg,
+				    "msDS-SecondaryKrbTgtNumber", krbtgt_number,
+				    LDB_FLAG_INTERNAL_DISABLE_VALIDATION);
 	if (ret != LDB_SUCCESS) {
 		return ldb_operr(ldb);
 	}
@@ -1792,7 +1789,7 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac)
 	struct ldb_context *ldb = ldb_module_get_ctx(ac->module);
 	void *skip_allocate_sids = ldb_get_opaque(ldb,
 						  "skip_allocate_sids");
-	struct ldb_message_element *el, *el2;
+	struct ldb_message_element *el;
 	struct dom_sid *sid;
 	int ret;
 
@@ -1926,23 +1923,17 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac)
 		/* "isCriticalSystemObject" might be set */
 		if (user_account_control &
 		    (UF_SERVER_TRUST_ACCOUNT | UF_PARTIAL_SECRETS_ACCOUNT)) {
-			ret = ldb_msg_add_string(ac->msg, "isCriticalSystemObject",
-						 "TRUE");
+			ret = ldb_msg_add_string_flags(ac->msg, "isCriticalSystemObject",
+						       "TRUE", LDB_FLAG_MOD_REPLACE);
 			if (ret != LDB_SUCCESS) {
 				return ret;
 			}
-			el2 = ldb_msg_find_element(ac->msg,
-						   "isCriticalSystemObject");
-			el2->flags = LDB_FLAG_MOD_REPLACE;
 		} else if (user_account_control & UF_WORKSTATION_TRUST_ACCOUNT) {
-			ret = ldb_msg_add_string(ac->msg, "isCriticalSystemObject",
-						 "FALSE");
+			ret = ldb_msg_add_string_flags(ac->msg, "isCriticalSystemObject",
+						       "FALSE", LDB_FLAG_MOD_REPLACE);
 			if (ret != LDB_SUCCESS) {
 				return ret;
 			}
-			el2 = ldb_msg_find_element(ac->msg,
-						   "isCriticalSystemObject");
-			el2->flags = LDB_FLAG_MOD_REPLACE;
 		}
 
 		/* Step 1.4: "userAccountControl" -> "primaryGroupID" mapping */
@@ -2018,14 +2009,13 @@ static int samldb_objectclass_trigger(struct samldb_ctx *ac)
 				ldb_set_errstring(ldb, "samldb: Unrecognized account type!");
 				return LDB_ERR_UNWILLING_TO_PERFORM;
 			}
-			ret = samdb_msg_add_uint(ldb, ac->msg, ac->msg,
-						 "sAMAccountType",
-						 account_type);
+			ret = samdb_msg_add_uint_flags(ldb, ac->msg, ac->msg,
+						       "sAMAccountType",
+						       account_type,
+						       LDB_FLAG_MOD_REPLACE);
 			if (ret != LDB_SUCCESS) {
 				return ret;
 			}
-			el2 = ldb_msg_find_element(ac->msg, "sAMAccountType");
-			el2->flags = LDB_FLAG_MOD_REPLACE;
 		}
 		break;
 	}
@@ -2945,26 +2935,23 @@ static int samldb_user_account_control_change(struct samldb_ctx *ac)
 	}
 
 	if (old_atype != new_atype) {
-		ret = samdb_msg_add_uint(ldb, ac->msg, ac->msg,
-					 "sAMAccountType", new_atype);
+		ret = samdb_msg_append_uint(ldb, ac->msg, ac->msg,
+					    "sAMAccountType", new_atype,
+					    LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
-		el = ldb_msg_find_element(ac->msg, "sAMAccountType");
-		el->flags = LDB_FLAG_MOD_REPLACE;
 	}
 
 	/* As per MS-SAMR 3.1.1.8.10 these flags have not to be set */
 	if ((clear_uac & UF_LOCKOUT) && (old_lockoutTime != 0)) {
 		/* "lockoutTime" reset as per MS-SAMR 3.1.1.8.10 */
 		ldb_msg_remove_attr(ac->msg, "lockoutTime");
-		ret = samdb_msg_add_uint64(ldb, ac->msg, ac->msg, "lockoutTime",
-					   (NTTIME)0);
+		ret = samdb_msg_append_uint64(ldb, ac->msg, ac->msg, "lockoutTime",
+					      (NTTIME)0, LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
-		el = ldb_msg_find_element(ac->msg, "lockoutTime");
-		el->flags = LDB_FLAG_MOD_REPLACE;
 	}
 
 	/*
@@ -2975,14 +2962,12 @@ static int samldb_user_account_control_change(struct samldb_ctx *ac)
 	 * creating the attribute.
 	 */
 	if (old_is_critical != new_is_critical || old_atype != new_atype) {
-		ret = ldb_msg_add_string(ac->msg, "isCriticalSystemObject",
-					 new_is_critical ? "TRUE": "FALSE");
+		ret = ldb_msg_append_string(ac->msg, "isCriticalSystemObject",
+					    new_is_critical ? "TRUE": "FALSE",
+					    LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
-		el = ldb_msg_find_element(ac->msg,
-					   "isCriticalSystemObject");
-		el->flags = LDB_FLAG_MOD_REPLACE;
 	}
 
 	if (!ldb_msg_find_element(ac->msg, "primaryGroupID") &&
@@ -2995,14 +2980,12 @@ static int samldb_user_account_control_change(struct samldb_ctx *ac)
 			}
 		}
 
-		ret = samdb_msg_add_uint(ldb, ac->msg, ac->msg,
-					 "primaryGroupID", new_pgrid);
+		ret = samdb_msg_append_uint(ldb, ac->msg, ac->msg,
+					    "primaryGroupID", new_pgrid,
+					    LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
-		el = ldb_msg_find_element(ac->msg,
-					   "primaryGroupID");
-		el->flags = LDB_FLAG_MOD_REPLACE;
 	}
 
 	/* Propagate eventual "userAccountControl" attribute changes */
@@ -3205,13 +3188,12 @@ static int samldb_lockout_time(struct samldb_ctx *ac)
 
 	/* lockoutTime == 0 resets badPwdCount */
 	ldb_msg_remove_attr(ac->msg, "badPwdCount");
-	ret = samdb_msg_add_int(ldb, ac->msg, ac->msg,
-				"badPwdCount", 0);
+	ret = samdb_msg_append_int(ldb, ac->msg, ac->msg,
+				   "badPwdCount", 0,
+				   LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
-	el = ldb_msg_find_element(ac->msg, "badPwdCount");
-	el->flags = LDB_FLAG_MOD_REPLACE;
 
 	return LDB_SUCCESS;
 }
@@ -3309,13 +3291,11 @@ static int samldb_group_type_change(struct samldb_ctx *ac)
 		ldb_set_errstring(ldb, "samldb: Unrecognized account type!");
 		return LDB_ERR_UNWILLING_TO_PERFORM;
 	}
-	ret = samdb_msg_add_uint(ldb, ac->msg, ac->msg, "sAMAccountType",
-				 account_type);
+	ret = samdb_msg_append_uint(ldb, ac->msg, ac->msg, "sAMAccountType",
+				    account_type, LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		return ret;
 	}
-	el = ldb_msg_find_element(ac->msg, "sAMAccountType");
-	el->flags = LDB_FLAG_MOD_REPLACE;
 
 	return LDB_SUCCESS;
 }
diff --git a/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c b/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
index 5f8911c66be..99c5955e9e7 100644
--- a/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
+++ b/source4/dsdb/samdb/ldb_modules/tombstone_reanimate.c
@@ -294,14 +294,13 @@ static int tr_prepare_attributes(struct tr_context *ac)
 			return ldb_error(ldb, LDB_ERR_UNWILLING_TO_PERFORM,
 					 "reanimate: Unrecognized account type!");
 		}
-		ret = samdb_msg_add_uint(ldb, ac->mod_msg, ac->mod_msg,
-					 "sAMAccountType", account_type);
+		ret = samdb_msg_append_uint(ldb, ac->mod_msg, ac->mod_msg,
+					    "sAMAccountType", account_type,
+					    LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return ldb_error(ldb, LDB_ERR_OPERATIONS_ERROR,
 					 "reanimate: Failed to add sAMAccountType to restored object.");
 		}
-		el = ldb_msg_find_element(ac->mod_msg, "sAMAccountType");
-		el->flags = LDB_FLAG_MOD_REPLACE;
 
 		/* Default values set by Windows */
 		ret = samdb_find_or_add_attribute(ldb, ac->mod_msg,
@@ -324,12 +323,11 @@ static int tr_prepare_attributes(struct tr_context *ac)
 			return ret;
 		}
 
-		ret = ldb_msg_add_string(ac->mod_msg, "objectCategory", value);
+		ret = ldb_msg_append_string(ac->mod_msg, "objectCategory", value,
+					    LDB_FLAG_MOD_ADD);
 		if (ret != LDB_SUCCESS) {
 			return ret;
 		}
-		el = ldb_msg_find_element(ac->mod_msg, "objectCategory");
-		el->flags = LDB_FLAG_MOD_ADD;
 	}
 
 	return LDB_SUCCESS;
diff --git a/source4/nbt_server/wins/winsdb.c b/source4/nbt_server/wins/winsdb.c
index e4a7c2042ed..2a05e96bca4 100644
--- a/source4/nbt_server/wins/winsdb.c
+++ b/source4/nbt_server/wins/winsdb.c
@@ -102,13 +102,11 @@ uint64_t winsdb_set_maxVersion(struct winsdb_handle *h, uint64_t newMaxVersion)
 	msg->dn = dn;
 
 
-	ret = ldb_msg_add_empty(msg, "objectClass", LDB_FLAG_MOD_REPLACE, NULL);
+	ret = ldb_msg_append_string(msg, "objectClass", "winsMaxVersion",
+				    LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) goto failed;
-	ret = ldb_msg_add_string(msg, "objectClass", "winsMaxVersion");
-	if (ret != LDB_SUCCESS) goto failed;
-	ret = ldb_msg_add_empty(msg, "maxVersion", LDB_FLAG_MOD_REPLACE, NULL);
-	if (ret != LDB_SUCCESS) goto failed;
-	ret = ldb_msg_add_fmt(msg, "maxVersion", "%llu", (long long)newMaxVersion);
+	ret = ldb_msg_append_fmt(msg, LDB_FLAG_MOD_REPLACE,
+				 "maxVersion", "%llu", (long long)newMaxVersion);
 	if (ret != LDB_SUCCESS) goto failed;
 
 	ret = ldb_modify(wins_db, msg);
@@ -779,8 +777,7 @@ static struct ldb_message *winsdb_message(struct ldb_context *ldb,
 		ret |= ldb_msg_add_winsdb_addr(msg, rec, "address", rec->addresses[i]);
 	}
 	if (rec->registered_by) {
-		ret |= ldb_msg_add_empty(msg, "registeredBy", 0, NULL);
-		ret |= ldb_msg_add_string(msg, "registeredBy", rec->registered_by);
+		ret |= ldb_msg_append_string(msg, "registeredBy", rec->registered_by, 0);
 	}
 	if (ret != LDB_SUCCESS) goto failed;
 	return msg;
diff --git a/source4/rpc_server/lsa/dcesrv_lsa.c b/source4/rpc_server/lsa/dcesrv_lsa.c
index 15b068aec62..a165ab2b9d6 100644
--- a/source4/rpc_server/lsa/dcesrv_lsa.c
+++ b/source4/rpc_server/lsa/dcesrv_lsa.c
@@ -1778,12 +1778,7 @@ static NTSTATUS update_uint32_t_value(TALLOC_CTX *mem_ctx,
 		goto done;
 	}
 
-	ret = ldb_msg_add_empty(dest, attribute, flags, NULL);
-	if (ret != LDB_SUCCESS) {
-		return NT_STATUS_NO_MEMORY;
-	}
-
-	ret = samdb_msg_add_uint(sam_ldb, dest, dest, attribute, value);
+	ret = samdb_msg_append_uint(sam_ldb, dest, dest, attribute, value, flags);
 	if (ret != LDB_SUCCESS) {
 		return NT_STATUS_NO_MEMORY;
 	}
@@ -1874,13 +1869,7 @@ static NTSTATUS update_trust_user(TALLOC_CTX *mem_ctx,
 			continue;
 		}
 
-		ret = ldb_msg_add_empty(msg, attribute,
-					LDB_FLAG_MOD_REPLACE, NULL);
-		if (ret != LDB_SUCCESS) {
-			return NT_STATUS_NO_MEMORY;
-		}
-
-		ret = ldb_msg_add_value(msg, attribute, &v, NULL);
+		ret = ldb_msg_append_value(msg, attribute, &v, LDB_FLAG_MOD_REPLACE);
 		if (ret != LDB_SUCCESS) {
 			return NT_STATUS_NO_MEMORY;
 		}
@@ -2166,28 +2155,30 @@ static NTSTATUS setInfoTrustedDomain_base(struct dcesrv_call_state *dce_call,
 	}
 
 	if (add_incoming || del_incoming) {
-		ret = ldb_msg_add_empty(msg, "trustAuthIncoming",
-					LDB_FLAG_MOD_REPLACE, NULL);
-		if (ret != LDB_SUCCESS) {
-			return NT_STATUS_NO_MEMORY;
-		}
 		if (add_incoming) {
-			ret = ldb_msg_add_value(msg, "trustAuthIncoming",
-						&trustAuthIncoming, NULL);
+			ret = ldb_msg_append_value(msg, "trustAuthIncoming",
+						   &trustAuthIncoming, LDB_FLAG_MOD_REPLACE);
+			if (ret != LDB_SUCCESS) {
+				return NT_STATUS_NO_MEMORY;
+			}
+		} else {
+			ret = ldb_msg_add_empty(msg, "trustAuthIncoming",
+						LDB_FLAG_MOD_REPLACE, NULL);
 			if (ret != LDB_SUCCESS) {
 				return NT_STATUS_NO_MEMORY;
 			}
 		}
 	}
 	if (add_outgoing || del_outgoing) {
-		ret = ldb_msg_add_empty(msg, "trustAuthOutgoing",
-					LDB_FLAG_MOD_REPLACE, NULL);
-		if (ret != LDB_SUCCESS) {
-			return NT_STATUS_NO_MEMORY;
-		}
 		if (add_outgoing) {
-			ret = ldb_msg_add_value(msg, "trustAuthOutgoing",
-						&trustAuthOutgoing, NULL);
+			ret = ldb_msg_append_value(msg, "trustAuthOutgoing",
+						   &trustAuthOutgoing, LDB_FLAG_MOD_REPLACE);
+			if (ret != LDB_SUCCESS) {
+				return NT_STATUS_NO_MEMORY;
+			}
+		} else {
+			ret = ldb_msg_add_empty(msg, "trustAuthOutgoing",
+						LDB_FLAG_MOD_REPLACE, NULL);
 			if (ret != LDB_SUCCESS) {
 				return NT_STATUS_NO_MEMORY;
 			}
@@ -4635,14 +4626,8 @@ static NTSTATUS dcesrv_lsa_lsaRSetForestTrustInformation(struct dcesrv_call_stat
 		goto done;
 	}
 
-	ret = ldb_msg_add_empty(msg, "msDS-TrustForestTrustInfo",
-				LDB_FLAG_MOD_REPLACE, NULL);
-	if (ret != LDB_SUCCESS) {
-		status = NT_STATUS_NO_MEMORY;
-		goto done;
-	}
-	ret = ldb_msg_add_value(msg, "msDS-TrustForestTrustInfo",
-				&ft_blob, NULL);
+	ret = ldb_msg_append_value(msg, "msDS-TrustForestTrustInfo",
+				   &ft_blob, LDB_FLAG_MOD_REPLACE);
 	if (ret != LDB_SUCCESS) {
 		status = NT_STATUS_NO_MEMORY;
 		goto done;
diff --git a/source4/winbind/idmap.c b/source4/winbind/idmap.c
index c4039be473a..c6375f8357a 100644
--- a/source4/winbind/idmap.c
+++ b/source4/winbind/idmap.c
@@ -672,14 +672,8 @@ static NTSTATUS idmap_sid_to_xid(struct idmap_context *idmap_ctx,
 		vals[1].data = (uint8_t *)hwm_string;
 		vals[1].length = strlen(hwm_string);
 	} else {
-		ret = ldb_msg_add_empty(hwm_msg, "xidNumber", LDB_FLAG_MOD_ADD,
-					NULL);
-		if (ret != LDB_SUCCESS) {
-			status = NT_STATUS_NONE_MAPPED;
-			goto failed;
-		}
-
-		ret = ldb_msg_add_string(hwm_msg, "xidNumber", hwm_string);
+		ret = ldb_msg_append_string(hwm_msg, "xidNumber", hwm_string,
+					    LDB_FLAG_MOD_ADD);
 		if (ret != LDB_SUCCESS)
 		{
 			status = NT_STATUS_NONE_MAPPED;
-- 
2.25.1