s4:samdb_msg_find_old_and_new_ldb_val - rework

- don't crash when no values where specified
- return ERR_CONSTRAINT_VIOLATION on malformed messages
- only check for flags when we are involved in a LDB modify operation

2 files changed, 38 insertions, 10 deletions

diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index 3fa6774acb..79a4c143c7 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -709,12 +709,13 @@ struct ldb_message_element *samdb_find_attribute(struct ldb_context *ldb,
  * for entries). The latter (old value) has always specified
  * LDB_FLAG_MOD_DELETE.
  *
- * Returns LDB_ERR_NO_SUCH_ATTRIBUTE if the attribute which should be deleted
- * doesn't contain only one value (this is the Windows Server behaviour)
- * otherwise LDB_SUCCESS.
+ * Returns LDB_ERR_CONSTRAINT_VIOLATION and LDB_ERR_UNWILLING_TO_PERFORM if
+ * matching message elements are malformed in respect to the set/change rules.
+ * Otherwise it returns LDB_SUCCESS.
  */
 int samdb_msg_find_old_and_new_ldb_val(const struct ldb_message *msg,
 				       const char *name,
+				       enum ldb_request_type operation,
 				       const struct ldb_val **new_val,
 				       const struct ldb_val **old_val)
 {
@@ -728,11 +729,31 @@ int samdb_msg_find_old_and_new_ldb_val(const struct ldb_message *msg,
 	}
 
 	for (i = 0; i < msg->num_elements; i++) {
-		if (ldb_attr_cmp(msg->elements[i].name, name) == 0) {
-			if (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) == LDB_FLAG_MOD_DELETE) {
+		if (ldb_attr_cmp(msg->elements[i].name, name) != 0) {
+			continue;
+		}
+
+		if ((operation == LDB_MODIFY) &&
+		    (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) == LDB_FLAG_MOD_DELETE)) {
+			/* 0 values are allowed */
+			if (msg->elements[i].num_values == 1) {
 				*old_val = &msg->elements[i].values[0];
+			} else if (msg->elements[i].num_values > 1) {
+				return LDB_ERR_CONSTRAINT_VIOLATION;
+			}
+		} else if ((operation == LDB_MODIFY) &&
+			   (LDB_FLAG_MOD_TYPE(msg->elements[i].flags) == LDB_FLAG_MOD_REPLACE)) {
+			if (msg->elements[i].num_values > 0) {
+				*new_val = &msg->elements[i].values[msg->elements[i].num_values - 1];
+			} else {
+				return LDB_ERR_UNWILLING_TO_PERFORM;
+			}
+		} else {
+			/* Add operations and LDB_FLAG_MOD_ADD */
+			if (msg->elements[i].num_values > 0) {
+				*new_val = &msg->elements[i].values[msg->elements[i].num_values - 1];
 			} else {
-				*new_val = &msg->elements[i].values[0];
+				return LDB_ERR_CONSTRAINT_VIOLATION;
 			}
 		}
 	}
diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c
index b2ce9d4587..e5785f7fb7 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -1687,7 +1687,9 @@ static int setup_io(struct ph_context *ac,
 	}
 
 	ret = samdb_msg_find_old_and_new_ldb_val(orig_msg, "userPassword",
-		&io->n.cleartext_utf8, &io->og.cleartext_utf8);
+						 ac->req->operation,
+						 &io->n.cleartext_utf8,
+						 &io->og.cleartext_utf8);
 	if (ret != LDB_SUCCESS) {
 		ldb_asprintf_errstring(ldb,
 			"setup_io: "
@@ -1696,7 +1698,9 @@ static int setup_io(struct ph_context *ac,
 	}
 
 	ret = samdb_msg_find_old_and_new_ldb_val(orig_msg, "clearTextPassword",
-		&io->n.cleartext_utf16, &io->og.cleartext_utf16);
+						 ac->req->operation,
+						 &io->n.cleartext_utf16,
+						 &io->og.cleartext_utf16);
 	if (ret != LDB_SUCCESS) {
 		ldb_asprintf_errstring(ldb,
 			"setup_io: "
@@ -1718,7 +1722,9 @@ static int setup_io(struct ph_context *ac,
 	   a nthash */
 
 	ret = samdb_msg_find_old_and_new_ldb_val(orig_msg, "unicodePwd",
-		&quoted_utf16, &old_quoted_utf16);
+						 ac->req->operation,
+						 &quoted_utf16,
+						 &old_quoted_utf16);
 	if (ret != LDB_SUCCESS) {
 		ldb_asprintf_errstring(ldb,
 			"setup_io: "
@@ -1833,7 +1839,8 @@ static int setup_io(struct ph_context *ac,
 	/* Handles the "dBCSPwd" attribute (LM hash) */
 	io->n.lm_hash = NULL; io->og.lm_hash = NULL;
 	ret = samdb_msg_find_old_and_new_ldb_val(orig_msg, "dBCSPwd",
-		&lm_hash, &old_lm_hash);
+						 ac->req->operation,
+						 &lm_hash, &old_lm_hash);
 	if (ret != LDB_SUCCESS) {
 		ldb_asprintf_errstring(ldb,
 			"setup_io: "