From 189d12940caffa66e6a11e226591bb2a20d4fa34 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra@samba.org>
Date: Tue, 2 Jan 2018 15:56:03 -0800
Subject: [PATCH 01/14] CVE-2018-1050: s3: RPC: spoolss server. Protect against
 null pointer derefs.

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

Signed-off-by: Jeremy Allison <jra@samba.org>
---
 source3/rpc_server/spoolss/srv_spoolss_nt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
index a9e778ee023..ef147e970e6 100644
--- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
+++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
@@ -182,6 +182,11 @@ static void prune_printername_cache(void);
 static const char *canon_servername(const char *servername)
 {
 	const char *pservername = servername;
+
+	if (servername == NULL) {
+		return "";
+	}
+
 	while (*pservername == '\\') {
 		pservername++;
 	}
@@ -2082,6 +2087,10 @@ WERROR _spoolss_DeletePrinterDriver(struct pipes_struct *p,
 		return WERR_ACCESS_DENIED;
 	}
 
+	if (r->in.architecture == NULL || r->in.driver == NULL) {
+		return WERR_INVALID_ENVIRONMENT;
+	}
+
 	/* check that we have a valid driver name first */
 
 	if ((version = get_version_id(r->in.architecture)) == -1) {
@@ -2221,6 +2230,10 @@ WERROR _spoolss_DeletePrinterDriverEx(struct pipes_struct *p,
 		return WERR_ACCESS_DENIED;
 	}
 
+	if (r->in.architecture == NULL || r->in.driver == NULL) {
+		return WERR_INVALID_ENVIRONMENT;
+	}
+
 	/* check that we have a valid driver name first */
 	if (get_version_id(r->in.architecture) == -1) {
 		/* this is what NT returns */
-- 
2.11.0


From 429a17f9b034fcdb4711d2405079c62b9af4b987 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 12:43:09 +0100
Subject: [PATCH 02/14] CVE-2018-1057: s4:dsdb/tests: add a test for password
 change with empty delete

Note that the request using the clearTextPassword attribute for the
password change is already correctly rejected by the server.

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>
---
 selftest/knownfail.d/samba4.ldap.passwords.python |  2 +
 source4/dsdb/tests/python/passwords.py            | 49 +++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 selftest/knownfail.d/samba4.ldap.passwords.python

diff --git a/selftest/knownfail.d/samba4.ldap.passwords.python b/selftest/knownfail.d/samba4.ldap.passwords.python
new file mode 100644
index 00000000000..343c5a7867d
--- /dev/null
+++ b/selftest/knownfail.d/samba4.ldap.passwords.python
@@ -0,0 +1,2 @@
+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/tests/python/passwords.py b/source4/dsdb/tests/python/passwords.py
index db013ea46ad..be1f34d2312 100755
--- a/source4/dsdb/tests/python/passwords.py
+++ b/source4/dsdb/tests/python/passwords.py
@@ -1020,6 +1020,55 @@ userPassword: thatsAcomplPASS4
         # Reset the "minPwdLength" as it was before
         self.ldb.set_minPwdLength(minPwdLength)
 
+    def test_pw_change_delete_no_value_userPassword(self):
+        """Test password change with userPassword where the delete attribute doesn't have a value"""
+
+        try:
+            self.ldb2.modify_ldif("""
+dn: cn=testuser,cn=users,""" + self.base_dn + """
+changetype: modify
+delete: userPassword
+add: userPassword
+userPassword: thatsAcomplPASS1
+""")
+        except LdbError, (num, msg):
+            self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+        else:
+            self.fail()
+
+    def test_pw_change_delete_no_value_clearTextPassword(self):
+        """Test password change with clearTextPassword where the delete attribute doesn't have a value"""
+
+        try:
+            self.ldb2.modify_ldif("""
+dn: cn=testuser,cn=users,""" + self.base_dn + """
+changetype: modify
+delete: clearTextPassword
+add: clearTextPassword
+clearTextPassword: thatsAcomplPASS2
+""")
+        except LdbError, (num, msg):
+            self.assertTrue(num == ERR_CONSTRAINT_VIOLATION or
+                            num == ERR_NO_SUCH_ATTRIBUTE) # for Windows
+        else:
+            self.fail()
+
+    def test_pw_change_delete_no_value_unicodePwd(self):
+        """Test password change with unicodePwd where the delete attribute doesn't have a value"""
+
+        try:
+            self.ldb2.modify_ldif("""
+dn: cn=testuser,cn=users,""" + self.base_dn + """
+changetype: modify
+delete: unicodePwd
+add: unicodePwd
+unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS3\"".encode('utf-16-le')) + """
+""")
+        except LdbError, (num, msg):
+            self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+        else:
+            self.fail()
+
     def tearDown(self):
         super(PasswordTests, self).tearDown()
         delete_force(self.ldb, "cn=testuser,cn=users," + self.base_dn)
-- 
2.11.0


From 116c4e316048afab09aed2470043cc6129a75b15 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 10:56:06 +0100
Subject: [PATCH 03/14] CVE-2018-1057: s4:dsdb/password_hash: add a helper
 variable for LDB_FLAG_MOD_TYPE

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>
---
 source4/dsdb/samdb/ldb_modules/password_hash.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c
index bd29377b1b6..bc1ef03db9d 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -3921,17 +3921,20 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r
 		}
 
 		while ((passwordAttr = ldb_msg_find_element(msg, *l)) != NULL) {
-			if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_DELETE) {
+			unsigned int mtype = LDB_FLAG_MOD_TYPE(passwordAttr->flags);
+
+			if (mtype == LDB_FLAG_MOD_DELETE) {
 				++del_attr_cnt;
 			}
-			if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_ADD) {
+			if (mtype == LDB_FLAG_MOD_ADD) {
 				++add_attr_cnt;
 			}
-			if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_REPLACE) {
+			if (mtype == LDB_FLAG_MOD_REPLACE) {
 				++rep_attr_cnt;
 			}
 			if ((passwordAttr->num_values != 1) &&
-			    (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_ADD)) {
+			    (mtype == LDB_FLAG_MOD_ADD))
+			{
 				talloc_free(ac);
 				ldb_asprintf_errstring(ldb,
 						       "'%s' attribute must have exactly one value on add operations!",
@@ -3939,7 +3942,8 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r
 				return LDB_ERR_CONSTRAINT_VIOLATION;
 			}
 			if ((passwordAttr->num_values > 1) &&
-			    (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_DELETE)) {
+			    (mtype == LDB_FLAG_MOD_DELETE))
+			{
 				talloc_free(ac);
 				ldb_asprintf_errstring(ldb,
 						       "'%s' attribute must have zero or one value(s) on delete operations!",
-- 
2.11.0


From 09eed842f79b122109501c570f4c5ca865756eba Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 14:40:59 +0100
Subject: [PATCH 04/14] CVE-2018-1057: s4:dsdb/password_hash: add a helper
 variable for passwordAttr->num_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>
---
 source4/dsdb/samdb/ldb_modules/password_hash.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c
index bc1ef03db9d..1e7081b997a 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -3922,6 +3922,7 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r
 
 		while ((passwordAttr = ldb_msg_find_element(msg, *l)) != NULL) {
 			unsigned int mtype = LDB_FLAG_MOD_TYPE(passwordAttr->flags);
+			unsigned int nvalues = passwordAttr->num_values;
 
 			if (mtype == LDB_FLAG_MOD_DELETE) {
 				++del_attr_cnt;
@@ -3932,18 +3933,14 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r
 			if (mtype == LDB_FLAG_MOD_REPLACE) {
 				++rep_attr_cnt;
 			}
-			if ((passwordAttr->num_values != 1) &&
-			    (mtype == LDB_FLAG_MOD_ADD))
-			{
+			if ((nvalues != 1) && (mtype == LDB_FLAG_MOD_ADD)) {
 				talloc_free(ac);
 				ldb_asprintf_errstring(ldb,
 						       "'%s' attribute must have exactly one value on add operations!",
 						       *l);
 				return LDB_ERR_CONSTRAINT_VIOLATION;
 			}
-			if ((passwordAttr->num_values > 1) &&
-			    (mtype == LDB_FLAG_MOD_DELETE))
-			{
+			if ((nvalues > 1) && (mtype == LDB_FLAG_MOD_DELETE)) {
 				talloc_free(ac);
 				ldb_asprintf_errstring(ldb,
 						       "'%s' attribute must have zero or one value(s) on delete operations!",
-- 
2.11.0


From a529401dfda5f9f255e42186a127d1dc52a8109b Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 17:38:31 +0100
Subject: [PATCH 05/14] CVE-2018-1057: s4:dsdb/acl: only call dsdb_acl_debug()
 if we checked the acl in acl_check_password_rights()

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index 27d4e7659cc..edc6733c7ab 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -1014,12 +1014,14 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 					       GUID_DRS_USER_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
 					       sid);
+		goto checked;
 	}
 	else if (rep_attr_cnt > 0 || (add_attr_cnt != del_attr_cnt)) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_FORCE_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
 					       sid);
+		goto checked;
 	}
 	else if (add_attr_cnt == 1 && del_attr_cnt == 1) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
@@ -1030,7 +1032,13 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) {
 			ret = LDB_ERR_CONSTRAINT_VIOLATION;
 		}
+		goto checked;
 	}
+
+	talloc_free(tmp_ctx);
+	return LDB_SUCCESS;
+
+checked:
 	if (ret != LDB_SUCCESS) {
 		dsdb_acl_debug(sd, acl_user_token(module),
 			       req->op.mod.message->dn,
-- 
2.11.0


From 67ad3bfb68e63ddb077f3ae024364c061d7f32b0 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 17:38:31 +0100
Subject: [PATCH 06/14] CVE-2018-1057: s4:dsdb/acl: remove unused else branches
 in acl_check_password_rights()

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index edc6733c7ab..c1655f9eeba 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -1016,14 +1016,24 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 					       sid);
 		goto checked;
 	}
-	else if (rep_attr_cnt > 0 || (add_attr_cnt != del_attr_cnt)) {
+
+	if (rep_attr_cnt > 0) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_FORCE_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
 					       sid);
 		goto checked;
 	}
-	else if (add_attr_cnt == 1 && del_attr_cnt == 1) {
+
+	if (add_attr_cnt != del_attr_cnt) {
+		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
+					       GUID_DRS_FORCE_CHANGE_PASSWORD,
+					       SEC_ADS_CONTROL_ACCESS,
+					       sid);
+		goto checked;
+	}
+
+	if (add_attr_cnt == 1 && del_attr_cnt == 1) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_USER_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
-- 
2.11.0


From ddf812257bf192ff89309f9197ab06d4660cdd4b Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 22:59:24 +0100
Subject: [PATCH 07/14] CVE-2018-1057: s4:dsdb/acl: check for internal controls
 before other checks

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 37 ++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index c1655f9eeba..b2aa20f4157 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -968,10 +968,33 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	unsigned int del_attr_cnt = 0, add_attr_cnt = 0, rep_attr_cnt = 0;
 	struct ldb_message_element *el;
 	struct ldb_message *msg;
+	struct ldb_control *c = NULL;
 	const char *passwordAttrs[] = { "userPassword", "clearTextPassword",
 					"unicodePwd", "dBCSPwd", NULL }, **l;
 	TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
 
+	c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID);
+	if (c != NULL) {
+		/*
+		 * The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we
+		 * have a user password change and not a set as the message
+		 * looks like. In it's value blob it contains the NT and/or LM
+		 * hash of the old password specified by the user.  This control
+		 * is used by the SAMR and "kpasswd" password change mechanisms.
+		 *
+		 * This control can't be used by real LDAP clients,
+		 * the only caller is samdb_set_password_internal(),
+		 * so we don't have to strict verification of the input.
+		 */
+		ret = acl_check_extended_right(tmp_ctx,
+					       sd,
+					       acl_user_token(module),
+					       GUID_DRS_USER_CHANGE_PASSWORD,
+					       SEC_ADS_CONTROL_ACCESS,
+					       sid);
+		goto checked;
+	}
+
 	msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message);
 	if (msg == NULL) {
 		return ldb_module_oom(module);
@@ -1002,20 +1025,6 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		return LDB_SUCCESS;
 	}
 
-	if (ldb_request_get_control(req,
-				    DSDB_CONTROL_PASSWORD_CHANGE_OID) != NULL) {
-		/* The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we
-		 * have a user password change and not a set as the message
-		 * looks like. In it's value blob it contains the NT and/or LM
-		 * hash of the old password specified by the user.
-		 * This control is used by the SAMR and "kpasswd" password
-		 * change mechanisms. */
-		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
-					       GUID_DRS_USER_CHANGE_PASSWORD,
-					       SEC_ADS_CONTROL_ACCESS,
-					       sid);
-		goto checked;
-	}
 
 	if (rep_attr_cnt > 0) {
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
-- 
2.11.0


From 39aa58a5c515c9e148f661efa1adbc9dedc51542 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 17:43:43 +0100
Subject: [PATCH 08/14] CVE-2018-1057: s4:dsdb/acl: add check for
 DSDB_CONTROL_PASSWORD_HASH_VALUES_OID control

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index b2aa20f4157..4bf9779d507 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -995,6 +995,26 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		goto checked;
 	}
 
+	c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_HASH_VALUES_OID);
+	if (c != NULL) {
+		/*
+		 * The "DSDB_CONTROL_PASSWORD_HASH_VALUES_OID" control, without
+		 * "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we
+		 * have a force password set.
+		 * This control is used by the SAMR/NETLOGON/LSA password
+		 * reset mechanisms.
+		 *
+		 * This control can't be used by real LDAP clients,
+		 * the only caller is samdb_set_password_internal(),
+		 * so we don't have to strict verification of the input.
+		 */
+		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
+					       GUID_DRS_FORCE_CHANGE_PASSWORD,
+					       SEC_ADS_CONTROL_ACCESS,
+					       sid);
+		goto checked;
+	}
+
 	msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message);
 	if (msg == NULL) {
 		return ldb_module_oom(module);
-- 
2.11.0


From 7f4fef05c45dfa797a2ab699af32d166df7451ce Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:17:26 +0100
Subject: [PATCH 09/14] CVE-2018-1057: s4:dsdb/acl: add a NULL check for
 talloc_new() in acl_check_password_rights()

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index 4bf9779d507..2c0aee41edf 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -973,6 +973,10 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 					"unicodePwd", "dBCSPwd", NULL }, **l;
 	TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
 
+	if (tmp_ctx == NULL) {
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
 	c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID);
 	if (c != NULL) {
 		/*
-- 
2.11.0


From c8aa8ffa40cf2cfb3ed2f295e55778b96418eebd Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 22 Feb 2018 10:54:37 +0100
Subject: [PATCH 10/14] 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>
---
 selftest/knownfail.d/samba4.ldap.passwords.python |  2 --
 source4/dsdb/samdb/ldb_modules/acl.c              | 18 +++++++++++++++++-
 2 files changed, 17 insertions(+), 3 deletions(-)
 delete mode 100644 selftest/knownfail.d/samba4.ldap.passwords.python

diff --git a/selftest/knownfail.d/samba4.ldap.passwords.python b/selftest/knownfail.d/samba4.ldap.passwords.python
deleted file mode 100644
index 343c5a7867d..00000000000
--- 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 2c0aee41edf..d27ec80461e 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;
 
-- 
2.11.0


From aee383268a4252d23e12518c973ca0048de56b35 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Wed, 14 Feb 2018 19:15:49 +0100
Subject: [PATCH 11/14] CVE-2018-1057: s4:dsdb/acl: run password checking only
 once

This is needed, because a later commit will let the acl module add a
control to the change request msg and we must ensure that this is only
done once.

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index d27ec80461e..f2259261588 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -1122,6 +1122,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 	struct ldb_control *as_system;
 	struct ldb_control *is_undelete;
 	bool userPassword;
+	bool password_rights_checked = false;
 	TALLOC_CTX *tmp_ctx;
 	const struct ldb_message *msg = req->op.mod.message;
 	static const char *acl_attrs[] = {
@@ -1267,6 +1268,9 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 		} else if (ldb_attr_cmp("unicodePwd", el->name) == 0 ||
 			   (userPassword && ldb_attr_cmp("userPassword", el->name) == 0) ||
 			   ldb_attr_cmp("clearTextPassword", el->name) == 0) {
+			if (password_rights_checked) {
+				continue;
+			}
 			ret = acl_check_password_rights(tmp_ctx,
 							module,
 							req,
@@ -1277,6 +1281,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 			if (ret != LDB_SUCCESS) {
 				goto fail;
 			}
+			password_rights_checked = true;
 		} else if (ldb_attr_cmp("servicePrincipalName", el->name) == 0) {
 			ret = acl_check_spn(tmp_ctx,
 					    module,
-- 
2.11.0


From 06032bffca2352e3e7757214563f6e97d4f162df Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:30:13 +0100
Subject: [PATCH 12/14] CVE-2018-1057: s4:dsdb/samdb: define
 DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID control

Will be used to pass "user password change" vs "password reset" from the
ACL to the password_hash module, ensuring both modules treat the request
identical.

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>
---
 source4/dsdb/samdb/samdb.h          | 9 +++++++++
 source4/libcli/ldap/ldap_controls.c | 1 +
 source4/setup/schema_samba4.ldif    | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h
index 586a3bfaf0e..6390258141e 100644
--- a/source4/dsdb/samdb/samdb.h
+++ b/source4/dsdb/samdb/samdb.h
@@ -182,6 +182,15 @@ struct dsdb_control_password_user_account_control {
 /* passed when we want to thoroughly delete linked attributes */
 #define DSDB_CONTROL_REPLMD_VANISH_LINKS "1.3.6.1.4.1.7165.4.3.29"
 
+/*
+ * Used to pass "user password change" vs "password reset" from the ACL to the
+ * password_hash module, ensuring both modules treat the request identical.
+ */
+#define DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID "1.3.6.1.4.1.7165.4.3.33"
+struct dsdb_control_password_acl_validation {
+	bool pwd_reset;
+};
+
 #define DSDB_EXTENDED_REPLICATED_OBJECTS_OID "1.3.6.1.4.1.7165.4.4.1"
 struct dsdb_extended_replicated_object {
 	struct ldb_message *msg;
diff --git a/source4/libcli/ldap/ldap_controls.c b/source4/libcli/ldap/ldap_controls.c
index 9df95c32a9d..7ecc9080cd1 100644
--- a/source4/libcli/ldap/ldap_controls.c
+++ b/source4/libcli/ldap/ldap_controls.c
@@ -1262,6 +1262,7 @@ static const struct ldap_control_handler ldap_known_controls[] = {
 	{ DSDB_CONTROL_PASSWORD_CHANGE_STATUS_OID, NULL, NULL },
 	{ DSDB_CONTROL_PASSWORD_HASH_VALUES_OID, NULL, NULL },
 	{ DSDB_CONTROL_PASSWORD_CHANGE_OID, NULL, NULL },
+	{ DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID, NULL, NULL },
 	{ DSDB_CONTROL_APPLY_LINKS, NULL, NULL },
 	{ LDB_CONTROL_BYPASS_OPERATIONAL_OID, NULL, NULL },
 	{ DSDB_CONTROL_CHANGEREPLMETADATA_OID, NULL, NULL },
diff --git a/source4/setup/schema_samba4.ldif b/source4/setup/schema_samba4.ldif
index 0189fb5934a..0e8847afbb0 100644
--- a/source4/setup/schema_samba4.ldif
+++ b/source4/setup/schema_samba4.ldif
@@ -222,6 +222,8 @@
 #Allocated: DSDB_CONTROL_SKIP_DUPLICATES_CHECK_OID 1.3.6.1.4.1.7165.4.3.28
 #Allocated: DSDB_CONTROL_REPLMD_VANISH_LINKS 1.3.6.1.4.1.7165.4.3.29
 #Allocated: LDB_CONTROL_RECALCULATE_RDN_OID 1.3.6.1.4.1.7165.4.3.30
+#Allocated: DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID 1.3.6.1.4.1.7165.4.3.33
+
 
 # Extended 1.3.6.1.4.1.7165.4.4.x
 #Allocated: DSDB_EXTENDED_REPLICATED_OBJECTS_OID 1.3.6.1.4.1.7165.4.4.1
-- 
2.11.0


From c1de637a37121d0e28d502d8b2ef507e7e8dd57f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:38:19 +0100
Subject: [PATCH 13/14] CVE-2018-1057: s4:dsdb: use
 DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID

This is used to pass information about which password change operation (change
or reset) the acl module validated, down to the password_hash module.

It's very important that both modules treat the request identical.

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c           | 41 ++++++++++++++++++++++++--
 source4/dsdb/samdb/ldb_modules/password_hash.c | 30 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index f2259261588..9b4be7b6909 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -973,13 +973,22 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	const char *passwordAttrs[] = { "userPassword", "clearTextPassword",
 					"unicodePwd", "dBCSPwd", NULL }, **l;
 	TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
+	struct dsdb_control_password_acl_validation *pav = NULL;
 
 	if (tmp_ctx == NULL) {
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
 
+	pav = talloc_zero(req, struct dsdb_control_password_acl_validation);
+	if (pav == NULL) {
+		talloc_free(tmp_ctx);
+		return LDB_ERR_OPERATIONS_ERROR;
+	}
+
 	c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID);
 	if (c != NULL) {
+		pav->pwd_reset = false;
+
 		/*
 		 * The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we
 		 * have a user password change and not a set as the message
@@ -1002,6 +1011,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 
 	c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_HASH_VALUES_OID);
 	if (c != NULL) {
+		pav->pwd_reset = true;
+
 		/*
 		 * The "DSDB_CONTROL_PASSWORD_HASH_VALUES_OID" control, without
 		 * "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we
@@ -1055,6 +1066,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 
 
 	if (rep_attr_cnt > 0) {
+		pav->pwd_reset = true;
+
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_FORCE_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
@@ -1063,6 +1076,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	}
 
 	if (add_attr_cnt != del_attr_cnt) {
+		pav->pwd_reset = true;
+
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_FORCE_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
@@ -1071,6 +1086,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	}
 
 	if (add_val_cnt == 1 && del_val_cnt == 1) {
+		pav->pwd_reset = false;
+
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_USER_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
@@ -1083,6 +1100,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	}
 
 	if (add_val_cnt == 1 && del_val_cnt == 0) {
+		pav->pwd_reset = true;
+
 		ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module),
 					       GUID_DRS_FORCE_CHANGE_PASSWORD,
 					       SEC_ADS_CONTROL_ACCESS,
@@ -1094,6 +1113,14 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		goto checked;
 	}
 
+	/*
+	 * Everything else is handled by the password_hash module where it will
+	 * fail, but with the correct error code when the module is again
+	 * checking the attributes. As the change request will lack the
+	 * DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID control, we can be sure that
+	 * any modification attempt that went this way will be rejected.
+	 */
+
 	talloc_free(tmp_ctx);
 	return LDB_SUCCESS;
 
@@ -1103,11 +1130,19 @@ checked:
 			       req->op.mod.message->dn,
 			       true,
 			       10);
+		talloc_free(tmp_ctx);
+		return ret;
 	}
-	talloc_free(tmp_ctx);
-	return ret;
-}
 
+	ret = ldb_request_add_control(req,
+		DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID, false, pav);
+	if (ret != LDB_SUCCESS) {
+		ldb_debug(ldb_module_get_ctx(module), LDB_DEBUG_ERROR,
+			  "Unable to register ACL validation control!\n");
+		return ret;
+	}
+	return LDB_SUCCESS;
+}
 
 static int acl_modify(struct ldb_module *module, struct ldb_request *req)
 {
diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c
index 1e7081b997a..3063d80f63b 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -3188,7 +3188,35 @@ static int setup_io(struct ph_context *ac,
 		/* On "add" we have only "password reset" */
 		ac->pwd_reset = true;
 	} else if (ac->req->operation == LDB_MODIFY) {
-		if (io->og.cleartext_utf8 || io->og.cleartext_utf16
+		struct ldb_control *pav_ctrl = NULL;
+		struct dsdb_control_password_acl_validation *pav = NULL;
+
+		pav_ctrl = ldb_request_get_control(ac->req,
+				DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID);
+		if (pav_ctrl != NULL) {
+			pav = talloc_get_type_abort(pav_ctrl->data,
+				struct dsdb_control_password_acl_validation);
+		}
+
+		if (pav == NULL && ac->update_password) {
+			bool ok;
+
+			/*
+			 * If the DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID
+			 * control is missing, we require system access!
+			 */
+			ok = dsdb_module_am_system(ac->module);
+			if (!ok) {
+				return ldb_module_operr(ac->module);
+			}
+		}
+
+		if (pav != NULL) {
+			/*
+			 * We assume what the acl module has validated.
+			 */
+			ac->pwd_reset = pav->pwd_reset;
+		} else if (io->og.cleartext_utf8 || io->og.cleartext_utf16
 		    || io->og.nt_hash || io->og.lm_hash) {
 			/* If we have an old password specified then for sure it
 			 * is a user "password change" */
-- 
2.11.0


From 8300e8e8c5f94fd1873cd856bdd83f89cb771de1 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 23:11:38 +0100
Subject: [PATCH 14/14] CVE-2018-1057: s4:dsdb/acl: changing dBCSPwd is only
 allowed with a control

This is not strictly needed to fig bug 13272, but it makes sense to also
fix this while fixing the overall ACL checking logic.

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>
---
 source4/dsdb/samdb/ldb_modules/acl.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c
index 9b4be7b6909..d750362c47f 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -971,7 +971,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 	struct ldb_message *msg;
 	struct ldb_control *c = NULL;
 	const char *passwordAttrs[] = { "userPassword", "clearTextPassword",
-					"unicodePwd", "dBCSPwd", NULL }, **l;
+					"unicodePwd", NULL }, **l;
 	TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
 	struct dsdb_control_password_acl_validation *pav = NULL;
 
@@ -1031,6 +1031,15 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx,
 		goto checked;
 	}
 
+	el = ldb_msg_find_element(req->op.mod.message, "dBCSPwd");
+	if (el != NULL) {
+		/*
+		 * dBCSPwd is only allowed with a control.
+		 */
+		talloc_free(tmp_ctx);
+		return LDB_ERR_UNWILLING_TO_PERFORM;
+	}
+
 	msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message);
 	if (msg == NULL) {
 		return ldb_module_oom(module);
-- 
2.11.0