The Samba-Bugzilla – Attachment 14008 Details for
Bug 13272
[SECURITY] CVE-2018-1057: Unprivileged user can change any user (and admin) password
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
CVE-2018-1057-v4-8.metze01.patches.txt
CVE-2018-1057-v4-8.metze01.patches.txt (text/plain), 31.51 KB, created by
Stefan Metzmacher
on 2018-03-01 12:25:35 UTC
(
hide
)
Description:
CVE-2018-1057-v4-8.metze01.patches.txt
Filename:
MIME Type:
Creator:
Stefan Metzmacher
Created:
2018-03-01 12:25:35 UTC
Size:
31.51 KB
patch
obsolete
>From d2b36d3bdd4f318f8958974201e5f460cd118723 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 db013ea..be1f34d 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) >-- >1.9.1 > > >From a6f15e056cb2f54d0650d4cb53efa500a9538eab 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 c428ff7..5b9de95 100644 >--- a/source4/dsdb/samdb/ldb_modules/password_hash.c >+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c >@@ -4246,17 +4246,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!", >@@ -4264,7 +4267,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 a0def34662763120753d03165d54b708d9b247b9 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 5b9de95..ab17ce3 100644 >--- a/source4/dsdb/samdb/ldb_modules/password_hash.c >+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c >@@ -4247,6 +4247,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; >@@ -4257,18 +4258,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 2d761c4a79d4a33d139af6e22da3f156a48d88af 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 27d4e765..edc6733 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, >-- >1.9.1 > > >From f8522a9603d265b4b9f6cb423aee098d84e60a02 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 edc6733..c1655f9 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, >-- >1.9.1 > > >From 41170d8f78c2203d72a0975ca048d98d521668c8 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 c1655f9..b2aa20f 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), >-- >1.9.1 > > >From f2ba57055d51bb35ac954ab1c2a892a0cc558f38 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 b2aa20f..4bf9779 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); >-- >1.9.1 > > >From e10d35d9ab14d00784f4a25faffee93420cf90bc 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 4bf9779..2c0aee4 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) { > /* >-- >1.9.1 > > >From c5a87c6421eb4091fe744d794d643694a9b5d319 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 2c0aee4..d27ec80 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; > >-- >1.9.1 > > >From d98acd26f3d6e51f2c9243d73a8ecdb21e96806f 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 d27ec80..f225926 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, >-- >1.9.1 > > >From fd6bb80065a0e902156dd49107c8497ca22a6b36 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 | 1 + > 3 files changed, 11 insertions(+) > >diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h >index 6a4820c..eb52740 100644 >--- a/source4/dsdb/samdb/samdb.h >+++ b/source4/dsdb/samdb/samdb.h >@@ -195,6 +195,15 @@ struct dsdb_control_password_user_account_control { > > #define DSDB_CONTROL_INVALID_NOT_IMPLEMENTED "1.3.6.1.4.1.7165.4.3.32" > >+/* >+ * 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 9df95c3..7ecc908 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 ebc1ae6..6aafc9e 100644 >--- a/source4/setup/schema_samba4.ldif >+++ b/source4/setup/schema_samba4.ldif >@@ -226,6 +226,7 @@ > #Allocated: LDB_CONTROL_RECALCULATE_RDN_OID 1.3.6.1.4.1.7165.4.3.30 > #Allocated: DSDB_CONTROL_FORCE_RODC_LOCAL_CHANGE 1.3.6.1.4.1.7165.4.3.31 > #Allocated: DSDB_CONTROL_INVALID_NOT_IMPLEMENTED 1.3.6.1.4.1.7165.4.3.32 >+#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 >-- >1.9.1 > > >From 4e036c4cb5d062347cf3131c758c221431467c79 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 f225926..9b4be7b 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 ab17ce3..1ddafb3 100644 >--- a/source4/dsdb/samdb/ldb_modules/password_hash.c >+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c >@@ -3512,7 +3512,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" */ >-- >1.9.1 > > >From e9b7e6d3626aaed44d6b3e86b7a2662b719a2e94 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 9b4be7b..d750362 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); >-- >1.9.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
slow
:
review+
metze
:
review?
(
abartlet
)
Actions:
View
Attachments on
bug 13272
:
13977
|
13978
| 14008 |
14009
|
14010
|
14011
|
14016
|
14028
|
14029
|
14030
|
14031
|
14032
|
14033
|
14034
|
14035
|
14036
|
14038
|
14039
|
14040
|
14041
|
14042
|
14043
|
14044
|
14045
|
14047
|
14048