From 6ff2935f6a1bb2bdfb45beea07d4cb7c69c66a74 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 12:43:09 +0100
Subject: [PATCH 01/13] 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 0000000..343c5a7
--- /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 fb3eee5..c50f2b6 100755
--- a/source4/dsdb/tests/python/passwords.py
+++ b/source4/dsdb/tests/python/passwords.py
@@ -931,6 +931,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)
--
1.9.1
From 35f8367aa64955d9f34beac9a62f8336e5e6c510 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 10:56:06 +0100
Subject: [PATCH 02/13] 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 05b0854..aa3871d 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -3152,17 +3152,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!",
@@ -3170,7 +3173,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!",
--
1.9.1
From 63c91916d15f355e7179292fac998056c0bd6a44 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 14:40:59 +0100
Subject: [PATCH 03/13] 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 aa3871d..690bb98 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -3153,6 +3153,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;
@@ -3163,18 +3164,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!",
--
1.9.1
From 895b1d2c9cbbde96646146a3c7b93bd326aada55 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 17:38:31 +0100
Subject: [PATCH 04/13] 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 62e560f..aa1660c 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -989,12 +989,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),
@@ -1005,7 +1007,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,
--
1.9.1
From db056b588d40c4c6995ee882286042dbf383f502 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/13] 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 aa1660c..5ec5fd3 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -991,14 +991,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,
--
1.9.1
From ff82d4c547476751f4506092517952ac682ec38c Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 22:59:24 +0100
Subject: [PATCH 06/13] 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 5ec5fd3..56ba988 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -943,10 +943,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);
@@ -977,20 +1000,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),
--
1.9.1
From 5c92da9918e2ccbcb39db2b060406f05973c0a24 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 17:43:43 +0100
Subject: [PATCH 07/13] 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 56ba988..00d52fe 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -970,6 +970,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);
--
1.9.1
From 6417b18bc767d471e3c88935073acdc19448dc54 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:17:26 +0100
Subject: [PATCH 08/13] 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 00d52fe..4146cbc 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -948,6 +948,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) {
/*
--
1.9.1
From bf6c7e1b4510242750de64b0a7a112c2024b4372 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 22 Feb 2018 10:54:37 +0100
Subject: [PATCH 09/13] 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 343c5a7..0000000
--- 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 4146cbc..7a003df 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -941,6 +941,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;
@@ -1006,12 +1007,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);
}
@@ -1041,7 +1045,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,
@@ -1053,6 +1057,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;
--
1.9.1
From fba762e9d7599e4e2f5022a1486f3ab777d18e6d Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Wed, 14 Feb 2018 19:15:49 +0100
Subject: [PATCH 10/13] 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 7a003df..c239c01 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -1097,6 +1097,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[] = {
@@ -1242,6 +1243,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,
@@ -1252,6 +1256,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,
--
1.9.1
From bc733fce398658e2c280dae4ba5041113e7cd500 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:30:13 +0100
Subject: [PATCH 11/13] 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 0a1d90d..98faa4f 100644
--- a/source4/dsdb/samdb/samdb.h
+++ b/source4/dsdb/samdb/samdb.h
@@ -158,6 +158,15 @@ struct dsdb_control_password_change {
*/
#define DSDB_CONTROL_CHANGEREPLMETADATA_RESORT_OID "1.3.6.1.4.1.7165.4.3.25"
+/*
+ * 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 14a80af..7837e05 100644
--- a/source4/libcli/ldap/ldap_controls.c
+++ b/source4/libcli/ldap/ldap_controls.c
@@ -1281,6 +1281,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 69aa363..6e184bc 100644
--- a/source4/setup/schema_samba4.ldif
+++ b/source4/setup/schema_samba4.ldif
@@ -200,6 +200,8 @@
#Allocated: DSDB_CONTROL_PERMIT_INTERDOMAIN_TRUST_UAC_OID 1.3.6.1.4.1.7165.4.3.23
#Allocated: DSDB_CONTROL_RESTORE_TOMBSTONE_OID 1.3.6.1.4.1.7165.4.3.24
#Allocated: DSDB_CONTROL_CHANGEREPLMETADATA_RESORT_OID 1.3.6.1.4.1.7165.4.3.25
+#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
--
1.9.1
From 7fc6a5ef5b1bad171dd6d2c019a4fe4c0ec00eb6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Fri, 16 Feb 2018 15:38:19 +0100
Subject: [PATCH 12/13] 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 c239c01..17e1e67 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -948,13 +948,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
@@ -977,6 +986,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
@@ -1030,6 +1041,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,
@@ -1038,6 +1051,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,
@@ -1046,6 +1061,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,
@@ -1058,6 +1075,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,
@@ -1069,6 +1088,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;
@@ -1078,11 +1105,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 690bb98..de565bc 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -2572,7 +2572,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) {
+ 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" */
--
1.9.1
From 0815e8653277383918530f8dd8afaeadfe8085d5 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow@samba.org>
Date: Thu, 15 Feb 2018 23:11:38 +0100
Subject: [PATCH 13/13] 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 17e1e67..8d9b780 100644
--- a/source4/dsdb/samdb/ldb_modules/acl.c
+++ b/source4/dsdb/samdb/ldb_modules/acl.c
@@ -946,7 +946,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;
@@ -1006,6 +1006,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);
--
1.9.1