From: Ralph Boehme <slow@samba.org>
Date: Thu, 22 Feb 2018 09:54:37 +0000 (+0100)
Subject: CVE-2018-1057: s4/dsdb: correctly detect password resets
X-Git-Url: http://git.samba.org/?a=commitdiff_plain;h=3e6621fe58014f19477633b1c0b54288550f0e87;p=metze%2Fsamba%2Fwip.git

CVE-2018-1057: s4/dsdb: correctly detect password resets

This change ensures we correctly treat the following LDIF

  dn: cn=testuser,cn=users,...
  changetype: modify
  delete: userPassword
  add: userPassword
  userPassword: thatsAcomplPASS1

as a password reset. Because delete and add element counts are both
one, the ACL module wrongly treated this as a password change
request.

For a password change we need at least one value to delete and one value
to add. This patch ensures we correctly check attributes and their
values.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272

Signed-off-by: Ralph Boehme <slow@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
---

diff --git a/selftest/knownfail.d/samba4.ldap.passwords.python b/selftest/knownfail.d/samba4.ldap.passwords.python
deleted file mode 100644
index 343c5a7867d2..000000000000
--- a/selftest/knownfail.d/samba4.ldap.passwords.python
+++ /dev/null
@@ -1,2 +0,0 @@
-samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_userPassword
-samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_unicodePwd
diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index 2c0aee41edf9..d27ec80461eb 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -966,6 +966,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 {
 	int ret = LDB_SUCCESS;
 	unsigned int del_attr_cnt = 0, add_attr_cnt = 0, rep_attr_cnt = 0;
+	unsigned int del_val_cnt = 0, add_val_cnt = 0, rep_val_cnt = 0;
 	struct ldb_message_element *el;
 	struct ldb_message *msg;
 	struct ldb_control *c = NULL;
@@ -1031,12 +1032,15 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		while ((el = ldb_msg_find_element(msg, *l)) != NULL) {
 			if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_DELETE) {
 				++del_attr_cnt;
+				del_val_cnt += el->num_values;
 			}
 			if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_ADD) {
 				++add_attr_cnt;
+				add_val_cnt += el->num_values;
 			}
 			if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_REPLACE) {
 				++rep_attr_cnt;
+				rep_val_cnt += el->num_values;
 			}
 			ldb_msg_remove_element(msg, el);
 		}
@@ -1066,7 +1070,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		goto checked;
 	}
 
-	if (add_attr_cnt == 1 && del_attr_cnt == 1) {
+	if (add_val_cnt == 1 && del_val_cnt == 1) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_USER_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
@@ -1078,6 +1082,18 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		goto checked;
 	}
 
+	if (add_val_cnt == 1 && del_val_cnt == 0) {
+		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
+					       GUID_DRS_FORCE_CHANGE_PASSWORD,
+					       SEC_ADS_CONTROL_ACCESS,
+					       sid);
+		/* Very strange, but we get constraint violation in this case */
+		if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) {
+			ret = LDB_ERR_CONSTRAINT_VIOLATION;
+		}
+		goto checked;
+	}
+
 	talloc_free(tmp_ctx);
 	return LDB_SUCCESS;