From d2b36d3bdd4f318f8958974201e5f460cd118723 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Stefan Metzmacher --- 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