The Samba-Bugzilla – Attachment 14043 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-3.metze01.patches.txt
CVE-2018-1057-v4-3.metze01.patches.txt (text/plain), 31.58 KB, created by
Stefan Metzmacher
on 2018-03-13 09:31:51 UTC
(
hide
)
Description:
CVE-2018-1057-v4-3.metze01.patches.txt
Filename:
MIME Type:
Creator:
Stefan Metzmacher
Created:
2018-03-13 09:31:51 UTC
Size:
31.58 KB
patch
obsolete
>From 568e86585be9d7b627aa7ed86f659de00327166a 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 c8012cabbb96f3bbef80e3976ff7335db7cc0176 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 3780f09d54d4d04719a7745946df4c111d98630c 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 74ff9c16ce20dcb719541a0091f047dbff4a00a8 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 78e6461..0a94075 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 0ab9ec313e4c3817b62ed9eb09948084a71198ed 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 0a94075..b0c01b9 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 b5cbd33792ea16e14e001b24712d6008a1841084 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 b0c01b9..f1dd39a 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 e6e1a232361ef162495be4a29e65fe8b1b09e07a 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 f1dd39a..c7656ac 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 ce08338aa9c4d1408fe98f4cde9f392eb7ff34a2 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 c7656ac..48fb0c0 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 281de59d13fbfa2fb418ea6bb3fb099c6844a097 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 48fb0c0..9a15de1 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 c03f7faa85770ea96cd81710e781272e6a7c111d 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 9a15de1..8ed6a5b 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 afe008f107a9e1290d15c6645d4f6929ed40f0f9 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 324045a..b3df29c 100644 >--- a/source4/dsdb/samdb/samdb.h >+++ b/source4/dsdb/samdb/samdb.h >@@ -157,6 +157,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 448a263..b73e1a8 100644 >--- a/source4/libcli/ldap/ldap_controls.c >+++ b/source4/libcli/ldap/ldap_controls.c >@@ -1280,6 +1280,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 aaa51051b811c66c0e519b0426189be27d17ee95 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 8ed6a5b..b968049 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 && 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 6dbdf12b4e4827be3894bee7a5c3e3104c0452f5 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 b968049..6e7e047 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 >
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
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