Date: Wed, 14 Mar 2018 02:04:36 +0000 (UTC) From: "Timur I. Bakeyev" <timur@FreeBSD.org> To: ports-committers@freebsd.org, svn-ports-all@freebsd.org, svn-ports-head@freebsd.org Subject: svn commit: r464444 - in head/net/samba44: . files Message-ID: <201803140204.w2E24ahv099445@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: timur Date: Wed Mar 14 02:04:36 2018 New Revision: 464444 URL: https://svnweb.freebsd.org/changeset/ports/464444 Log: Security update for samba44 and marking it deprecated PR: 226585 Security: CVE-2018-1057 Sponsored by: iXsystems Inc. Added: head/net/samba44/files/samba-4.4.16-CVE-2018-1057.patch (contents, props changed) Modified: head/net/samba44/Makefile Modified: head/net/samba44/Makefile ============================================================================== --- head/net/samba44/Makefile Wed Mar 14 01:20:29 2018 (r464443) +++ head/net/samba44/Makefile Wed Mar 14 02:04:36 2018 (r464444) @@ -3,7 +3,7 @@ PORTNAME?= ${SAMBA4_BASENAME}44 PORTVERSION?= ${SAMBA4_VERSION} -PORTREVISION?= 1 +PORTREVISION?= 2 CATEGORIES?= net MASTER_SITES= SAMBA/samba/stable SAMBA/samba/rc DISTNAME= ${SAMBA4_DISTNAME} @@ -13,9 +13,12 @@ COMMENT?= Free SMB/CIFS and AD/DC server and client f LICENSE= GPLv3 +DEPRECATED= yes +EXPIRATION_DATE= 2017-09-21 + CONFLICTS?= *samba3[2-6]-3.* samba4-4.0.* samba4[1-35-9]-4.* p5-Parse-Pidl-4.* -#EXTRA_PATCHES= ${PATCHDIR}/extra-patch-security:-p1 +EXTRA_PATCHES= ${PATCHDIR}/samba-4.4.16-CVE-2018-1057.patch:-p1 SAMBA4_BASENAME= samba SAMBA4_PORTNAME= ${SAMBA4_BASENAME}4 Added: head/net/samba44/files/samba-4.4.16-CVE-2018-1057.patch ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/net/samba44/files/samba-4.4.16-CVE-2018-1057.patch Wed Mar 14 02:04:36 2018 (r464444) @@ -0,0 +1,903 @@ +From 6ff2935f6a1bb2bdfb45beea07d4cb7c69c66a74 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 12:43:09 +0100 +Subject: [PATCH 01/13] CVE-2018-1057: s4:dsdb/tests: add a test for password + change with empty delete + +Note that the request using the clearTextPassword attribute for the +password change is already correctly rejected by the server. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + selftest/knownfail.d/samba4.ldap.passwords.python | 2 + + source4/dsdb/tests/python/passwords.py | 49 +++++++++++++++++++++++ + 2 files changed, 51 insertions(+) + create mode 100644 selftest/knownfail.d/samba4.ldap.passwords.python + +diff --git a/selftest/knownfail.d/samba4.ldap.passwords.python b/selftest/knownfail.d/samba4.ldap.passwords.python +new file mode 100644 +index 0000000..343c5a7 +--- /dev/null ++++ b/selftest/knownfail.d/samba4.ldap.passwords.python +@@ -0,0 +1,2 @@ ++samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_userPassword ++samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_unicodePwd +diff --git a/source4/dsdb/tests/python/passwords.py b/source4/dsdb/tests/python/passwords.py +index fb3eee5..c50f2b6 100755 +--- a/source4/dsdb/tests/python/passwords.py ++++ b/source4/dsdb/tests/python/passwords.py +@@ -931,6 +931,55 @@ userPassword: thatsAcomplPASS4 + # Reset the "minPwdLength" as it was before + self.ldb.set_minPwdLength(minPwdLength) + ++ def test_pw_change_delete_no_value_userPassword(self): ++ """Test password change with userPassword where the delete attribute doesn't have a value""" ++ ++ try: ++ self.ldb2.modify_ldif(""" ++dn: cn=testuser,cn=users,""" + self.base_dn + """ ++changetype: modify ++delete: userPassword ++add: userPassword ++userPassword: thatsAcomplPASS1 ++""") ++ except LdbError, (num, msg): ++ self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) ++ else: ++ self.fail() ++ ++ def test_pw_change_delete_no_value_clearTextPassword(self): ++ """Test password change with clearTextPassword where the delete attribute doesn't have a value""" ++ ++ try: ++ self.ldb2.modify_ldif(""" ++dn: cn=testuser,cn=users,""" + self.base_dn + """ ++changetype: modify ++delete: clearTextPassword ++add: clearTextPassword ++clearTextPassword: thatsAcomplPASS2 ++""") ++ except LdbError, (num, msg): ++ self.assertTrue(num == ERR_CONSTRAINT_VIOLATION or ++ num == ERR_NO_SUCH_ATTRIBUTE) # for Windows ++ else: ++ self.fail() ++ ++ def test_pw_change_delete_no_value_unicodePwd(self): ++ """Test password change with unicodePwd where the delete attribute doesn't have a value""" ++ ++ try: ++ self.ldb2.modify_ldif(""" ++dn: cn=testuser,cn=users,""" + self.base_dn + """ ++changetype: modify ++delete: unicodePwd ++add: unicodePwd ++unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS3\"".encode('utf-16-le')) + """ ++""") ++ except LdbError, (num, msg): ++ self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) ++ else: ++ self.fail() ++ + def tearDown(self): + super(PasswordTests, self).tearDown() + delete_force(self.ldb, "cn=testuser,cn=users," + self.base_dn) +-- +1.9.1 + + +From 35f8367aa64955d9f34beac9a62f8336e5e6c510 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 10:56:06 +0100 +Subject: [PATCH 02/13] CVE-2018-1057: s4:dsdb/password_hash: add a helper + variable for LDB_FLAG_MOD_TYPE + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/password_hash.c | 14 +++++++++----- + 1 file changed, 9 insertions(+), 5 deletions(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c +index 05b0854..aa3871d 100644 +--- a/source4/dsdb/samdb/ldb_modules/password_hash.c ++++ b/source4/dsdb/samdb/ldb_modules/password_hash.c +@@ -3152,17 +3152,20 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r + } + + while ((passwordAttr = ldb_msg_find_element(msg, *l)) != NULL) { +- if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_DELETE) { ++ unsigned int mtype = LDB_FLAG_MOD_TYPE(passwordAttr->flags); ++ ++ if (mtype == LDB_FLAG_MOD_DELETE) { + ++del_attr_cnt; + } +- if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_ADD) { ++ if (mtype == LDB_FLAG_MOD_ADD) { + ++add_attr_cnt; + } +- if (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_REPLACE) { ++ if (mtype == LDB_FLAG_MOD_REPLACE) { + ++rep_attr_cnt; + } + if ((passwordAttr->num_values != 1) && +- (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_ADD)) { ++ (mtype == LDB_FLAG_MOD_ADD)) ++ { + talloc_free(ac); + ldb_asprintf_errstring(ldb, + "'%s' attribute must have exactly one value on add operations!", +@@ -3170,7 +3173,8 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r + return LDB_ERR_CONSTRAINT_VIOLATION; + } + if ((passwordAttr->num_values > 1) && +- (LDB_FLAG_MOD_TYPE(passwordAttr->flags) == LDB_FLAG_MOD_DELETE)) { ++ (mtype == LDB_FLAG_MOD_DELETE)) ++ { + talloc_free(ac); + ldb_asprintf_errstring(ldb, + "'%s' attribute must have zero or one value(s) on delete operations!", +-- +1.9.1 + + +From 63c91916d15f355e7179292fac998056c0bd6a44 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 14:40:59 +0100 +Subject: [PATCH 03/13] CVE-2018-1057: s4:dsdb/password_hash: add a helper + variable for passwordAttr->num_values + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/password_hash.c | 9 +++------ + 1 file changed, 3 insertions(+), 6 deletions(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c +index aa3871d..690bb98 100644 +--- a/source4/dsdb/samdb/ldb_modules/password_hash.c ++++ b/source4/dsdb/samdb/ldb_modules/password_hash.c +@@ -3153,6 +3153,7 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r + + while ((passwordAttr = ldb_msg_find_element(msg, *l)) != NULL) { + unsigned int mtype = LDB_FLAG_MOD_TYPE(passwordAttr->flags); ++ unsigned int nvalues = passwordAttr->num_values; + + if (mtype == LDB_FLAG_MOD_DELETE) { + ++del_attr_cnt; +@@ -3163,18 +3164,14 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r + if (mtype == LDB_FLAG_MOD_REPLACE) { + ++rep_attr_cnt; + } +- if ((passwordAttr->num_values != 1) && +- (mtype == LDB_FLAG_MOD_ADD)) +- { ++ if ((nvalues != 1) && (mtype == LDB_FLAG_MOD_ADD)) { + talloc_free(ac); + ldb_asprintf_errstring(ldb, + "'%s' attribute must have exactly one value on add operations!", + *l); + return LDB_ERR_CONSTRAINT_VIOLATION; + } +- if ((passwordAttr->num_values > 1) && +- (mtype == LDB_FLAG_MOD_DELETE)) +- { ++ if ((nvalues > 1) && (mtype == LDB_FLAG_MOD_DELETE)) { + talloc_free(ac); + ldb_asprintf_errstring(ldb, + "'%s' attribute must have zero or one value(s) on delete operations!", +-- +1.9.1 + + +From 895b1d2c9cbbde96646146a3c7b93bd326aada55 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 17:38:31 +0100 +Subject: [PATCH 04/13] CVE-2018-1057: s4:dsdb/acl: only call dsdb_acl_debug() + if we checked the acl in acl_check_password_rights() + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 8 ++++++++ + 1 file changed, 8 insertions(+) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 62e560f..aa1660c 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -989,12 +989,14 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + GUID_DRS_USER_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, + sid); ++ goto checked; + } + else if (rep_attr_cnt > 0 || (add_attr_cnt != del_attr_cnt)) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_FORCE_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, + sid); ++ goto checked; + } + else if (add_attr_cnt == 1 && del_attr_cnt == 1) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), +@@ -1005,7 +1007,13 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { + ret = LDB_ERR_CONSTRAINT_VIOLATION; + } ++ goto checked; + } ++ ++ talloc_free(tmp_ctx); ++ return LDB_SUCCESS; ++ ++checked: + if (ret != LDB_SUCCESS) { + dsdb_acl_debug(sd, acl_user_token(module), + req->op.mod.message->dn, +-- +1.9.1 + + +From db056b588d40c4c6995ee882286042dbf383f502 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 17:38:31 +0100 +Subject: [PATCH 05/13] CVE-2018-1057: s4:dsdb/acl: remove unused else branches + in acl_check_password_rights() + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 14 ++++++++++++-- + 1 file changed, 12 insertions(+), 2 deletions(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index aa1660c..5ec5fd3 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -991,14 +991,24 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + sid); + goto checked; + } +- else if (rep_attr_cnt > 0 || (add_attr_cnt != del_attr_cnt)) { ++ ++ if (rep_attr_cnt > 0) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_FORCE_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, + sid); + goto checked; + } +- else if (add_attr_cnt == 1 && del_attr_cnt == 1) { ++ ++ if (add_attr_cnt != del_attr_cnt) { ++ ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), ++ GUID_DRS_FORCE_CHANGE_PASSWORD, ++ SEC_ADS_CONTROL_ACCESS, ++ sid); ++ goto checked; ++ } ++ ++ if (add_attr_cnt == 1 && del_attr_cnt == 1) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_USER_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +-- +1.9.1 + + +From ff82d4c547476751f4506092517952ac682ec38c Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 22:59:24 +0100 +Subject: [PATCH 06/13] CVE-2018-1057: s4:dsdb/acl: check for internal controls + before other checks + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 37 ++++++++++++++++++++++-------------- + 1 file changed, 23 insertions(+), 14 deletions(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 5ec5fd3..56ba988 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -943,10 +943,33 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + unsigned int del_attr_cnt = 0, add_attr_cnt = 0, rep_attr_cnt = 0; + struct ldb_message_element *el; + struct ldb_message *msg; ++ struct ldb_control *c = NULL; + const char *passwordAttrs[] = { "userPassword", "clearTextPassword", + "unicodePwd", "dBCSPwd", NULL }, **l; + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); + ++ c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID); ++ if (c != NULL) { ++ /* ++ * The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we ++ * have a user password change and not a set as the message ++ * looks like. In it's value blob it contains the NT and/or LM ++ * hash of the old password specified by the user. This control ++ * is used by the SAMR and "kpasswd" password change mechanisms. ++ * ++ * This control can't be used by real LDAP clients, ++ * the only caller is samdb_set_password_internal(), ++ * so we don't have to strict verification of the input. ++ */ ++ ret = acl_check_extended_right(tmp_ctx, ++ sd, ++ acl_user_token(module), ++ GUID_DRS_USER_CHANGE_PASSWORD, ++ SEC_ADS_CONTROL_ACCESS, ++ sid); ++ goto checked; ++ } ++ + msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message); + if (msg == NULL) { + return ldb_module_oom(module); +@@ -977,20 +1000,6 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + return LDB_SUCCESS; + } + +- if (ldb_request_get_control(req, +- DSDB_CONTROL_PASSWORD_CHANGE_OID) != NULL) { +- /* The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we +- * have a user password change and not a set as the message +- * looks like. In it's value blob it contains the NT and/or LM +- * hash of the old password specified by the user. +- * This control is used by the SAMR and "kpasswd" password +- * change mechanisms. */ +- ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), +- GUID_DRS_USER_CHANGE_PASSWORD, +- SEC_ADS_CONTROL_ACCESS, +- sid); +- goto checked; +- } + + if (rep_attr_cnt > 0) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), +-- +1.9.1 + + +From 5c92da9918e2ccbcb39db2b060406f05973c0a24 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 17:43:43 +0100 +Subject: [PATCH 07/13] CVE-2018-1057: s4:dsdb/acl: add check for + DSDB_CONTROL_PASSWORD_HASH_VALUES_OID control + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 20 ++++++++++++++++++++ + 1 file changed, 20 insertions(+) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 56ba988..00d52fe 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -970,6 +970,26 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + goto checked; + } + ++ c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_HASH_VALUES_OID); ++ if (c != NULL) { ++ /* ++ * The "DSDB_CONTROL_PASSWORD_HASH_VALUES_OID" control, without ++ * "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we ++ * have a force password set. ++ * This control is used by the SAMR/NETLOGON/LSA password ++ * reset mechanisms. ++ * ++ * This control can't be used by real LDAP clients, ++ * the only caller is samdb_set_password_internal(), ++ * so we don't have to strict verification of the input. ++ */ ++ ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), ++ GUID_DRS_FORCE_CHANGE_PASSWORD, ++ SEC_ADS_CONTROL_ACCESS, ++ sid); ++ goto checked; ++ } ++ + msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message); + if (msg == NULL) { + return ldb_module_oom(module); +-- +1.9.1 + + +From 6417b18bc767d471e3c88935073acdc19448dc54 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Fri, 16 Feb 2018 15:17:26 +0100 +Subject: [PATCH 08/13] CVE-2018-1057: s4:dsdb/acl: add a NULL check for + talloc_new() in acl_check_password_rights() + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 00d52fe..4146cbc 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -948,6 +948,10 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + "unicodePwd", "dBCSPwd", NULL }, **l; + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); + ++ if (tmp_ctx == NULL) { ++ return LDB_ERR_OPERATIONS_ERROR; ++ } ++ + c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID); + if (c != NULL) { + /* +-- +1.9.1 + + +From bf6c7e1b4510242750de64b0a7a112c2024b4372 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 22 Feb 2018 10:54:37 +0100 +Subject: [PATCH 09/13] CVE-2018-1057: s4/dsdb: correctly detect password + resets + +This change ensures we correctly treat the following LDIF + + dn: cn=testuser,cn=users,... + changetype: modify + delete: userPassword + add: userPassword + userPassword: thatsAcomplPASS1 + +as a password reset. Because delete and add element counts are both +one, the ACL module wrongly treated this as a password change +request. + +For a password change we need at least one value to delete and one value +to add. This patch ensures we correctly check attributes and their +values. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + selftest/knownfail.d/samba4.ldap.passwords.python | 2 -- + source4/dsdb/samdb/ldb_modules/acl.c | 18 +++++++++++++++++- + 2 files changed, 17 insertions(+), 3 deletions(-) + delete mode 100644 selftest/knownfail.d/samba4.ldap.passwords.python + +diff --git a/selftest/knownfail.d/samba4.ldap.passwords.python b/selftest/knownfail.d/samba4.ldap.passwords.python +deleted file mode 100644 +index 343c5a7..0000000 +--- a/selftest/knownfail.d/samba4.ldap.passwords.python ++++ /dev/null +@@ -1,2 +0,0 @@ +-samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_userPassword +-samba4.ldap.passwords.python.*.__main__.PasswordTests.test_pw_change_delete_no_value_unicodePwd +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 4146cbc..7a003df 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -941,6 +941,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + { + int ret = LDB_SUCCESS; + unsigned int del_attr_cnt = 0, add_attr_cnt = 0, rep_attr_cnt = 0; ++ unsigned int del_val_cnt = 0, add_val_cnt = 0, rep_val_cnt = 0; + struct ldb_message_element *el; + struct ldb_message *msg; + struct ldb_control *c = NULL; +@@ -1006,12 +1007,15 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + while ((el = ldb_msg_find_element(msg, *l)) != NULL) { + if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_DELETE) { + ++del_attr_cnt; ++ del_val_cnt += el->num_values; + } + if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_ADD) { + ++add_attr_cnt; ++ add_val_cnt += el->num_values; + } + if (LDB_FLAG_MOD_TYPE(el->flags) == LDB_FLAG_MOD_REPLACE) { + ++rep_attr_cnt; ++ rep_val_cnt += el->num_values; + } + ldb_msg_remove_element(msg, el); + } +@@ -1041,7 +1045,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + goto checked; + } + +- if (add_attr_cnt == 1 && del_attr_cnt == 1) { ++ if (add_val_cnt == 1 && del_val_cnt == 1) { + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_USER_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +@@ -1053,6 +1057,18 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + goto checked; + } + ++ if (add_val_cnt == 1 && del_val_cnt == 0) { ++ ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), ++ GUID_DRS_FORCE_CHANGE_PASSWORD, ++ SEC_ADS_CONTROL_ACCESS, ++ sid); ++ /* Very strange, but we get constraint violation in this case */ ++ if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { ++ ret = LDB_ERR_CONSTRAINT_VIOLATION; ++ } ++ goto checked; ++ } ++ + talloc_free(tmp_ctx); + return LDB_SUCCESS; + +-- +1.9.1 + + +From fba762e9d7599e4e2f5022a1486f3ab777d18e6d Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Wed, 14 Feb 2018 19:15:49 +0100 +Subject: [PATCH 10/13] CVE-2018-1057: s4:dsdb/acl: run password checking only + once + +This is needed, because a later commit will let the acl module add a +control to the change request msg and we must ensure that this is only +done once. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 5 +++++ + 1 file changed, 5 insertions(+) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 7a003df..c239c01 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -1097,6 +1097,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) + struct ldb_control *as_system; + struct ldb_control *is_undelete; + bool userPassword; ++ bool password_rights_checked = false; + TALLOC_CTX *tmp_ctx; + const struct ldb_message *msg = req->op.mod.message; + static const char *acl_attrs[] = { +@@ -1242,6 +1243,9 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) + } else if (ldb_attr_cmp("unicodePwd", el->name) == 0 || + (userPassword && ldb_attr_cmp("userPassword", el->name) == 0) || + ldb_attr_cmp("clearTextPassword", el->name) == 0) { ++ if (password_rights_checked) { ++ continue; ++ } + ret = acl_check_password_rights(tmp_ctx, + module, + req, +@@ -1252,6 +1256,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) + if (ret != LDB_SUCCESS) { + goto fail; + } ++ password_rights_checked = true; + } else if (ldb_attr_cmp("servicePrincipalName", el->name) == 0) { + ret = acl_check_spn(tmp_ctx, + module, +-- +1.9.1 + + +From bc733fce398658e2c280dae4ba5041113e7cd500 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Fri, 16 Feb 2018 15:30:13 +0100 +Subject: [PATCH 11/13] CVE-2018-1057: s4:dsdb/samdb: define + DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID control + +Will be used to pass "user password change" vs "password reset" from the +ACL to the password_hash module, ensuring both modules treat the request +identical. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/samdb.h | 9 +++++++++ + source4/libcli/ldap/ldap_controls.c | 1 + + source4/setup/schema_samba4.ldif | 2 ++ + 3 files changed, 12 insertions(+) + +diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h +index 0a1d90d..98faa4f 100644 +--- a/source4/dsdb/samdb/samdb.h ++++ b/source4/dsdb/samdb/samdb.h +@@ -158,6 +158,15 @@ struct dsdb_control_password_change { + */ + #define DSDB_CONTROL_CHANGEREPLMETADATA_RESORT_OID "1.3.6.1.4.1.7165.4.3.25" + ++/* ++ * Used to pass "user password change" vs "password reset" from the ACL to the ++ * password_hash module, ensuring both modules treat the request identical. ++ */ ++#define DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID "1.3.6.1.4.1.7165.4.3.33" ++struct dsdb_control_password_acl_validation { ++ bool pwd_reset; ++}; ++ + #define DSDB_EXTENDED_REPLICATED_OBJECTS_OID "1.3.6.1.4.1.7165.4.4.1" + struct dsdb_extended_replicated_object { + struct ldb_message *msg; +diff --git a/source4/libcli/ldap/ldap_controls.c b/source4/libcli/ldap/ldap_controls.c +index 14a80af..7837e05 100644 +--- a/source4/libcli/ldap/ldap_controls.c ++++ b/source4/libcli/ldap/ldap_controls.c +@@ -1281,6 +1281,7 @@ static const struct ldap_control_handler ldap_known_controls[] = { + { DSDB_CONTROL_PASSWORD_CHANGE_STATUS_OID, NULL, NULL }, + { DSDB_CONTROL_PASSWORD_HASH_VALUES_OID, NULL, NULL }, + { DSDB_CONTROL_PASSWORD_CHANGE_OID, NULL, NULL }, ++ { DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID, NULL, NULL }, + { DSDB_CONTROL_APPLY_LINKS, NULL, NULL }, + { LDB_CONTROL_BYPASS_OPERATIONAL_OID, NULL, NULL }, + { DSDB_CONTROL_CHANGEREPLMETADATA_OID, NULL, NULL }, +diff --git a/source4/setup/schema_samba4.ldif b/source4/setup/schema_samba4.ldif +index 69aa363..6e184bc 100644 +--- a/source4/setup/schema_samba4.ldif ++++ b/source4/setup/schema_samba4.ldif +@@ -200,6 +200,8 @@ + #Allocated: DSDB_CONTROL_PERMIT_INTERDOMAIN_TRUST_UAC_OID 1.3.6.1.4.1.7165.4.3.23 + #Allocated: DSDB_CONTROL_RESTORE_TOMBSTONE_OID 1.3.6.1.4.1.7165.4.3.24 + #Allocated: DSDB_CONTROL_CHANGEREPLMETADATA_RESORT_OID 1.3.6.1.4.1.7165.4.3.25 ++#Allocated: DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID 1.3.6.1.4.1.7165.4.3.33 ++ + + # Extended 1.3.6.1.4.1.7165.4.4.x + #Allocated: DSDB_EXTENDED_REPLICATED_OBJECTS_OID 1.3.6.1.4.1.7165.4.4.1 +-- +1.9.1 + + +From 7fc6a5ef5b1bad171dd6d2c019a4fe4c0ec00eb6 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Fri, 16 Feb 2018 15:38:19 +0100 +Subject: [PATCH 12/13] CVE-2018-1057: s4:dsdb: use + DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID + +This is used to pass information about which password change operation (change +or reset) the acl module validated, down to the password_hash module. + +It's very important that both modules treat the request identical. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 41 ++++++++++++++++++++++++-- + source4/dsdb/samdb/ldb_modules/password_hash.c | 30 ++++++++++++++++++- + 2 files changed, 67 insertions(+), 4 deletions(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index c239c01..17e1e67 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -948,13 +948,22 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + const char *passwordAttrs[] = { "userPassword", "clearTextPassword", + "unicodePwd", "dBCSPwd", NULL }, **l; + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); ++ struct dsdb_control_password_acl_validation *pav = NULL; + + if (tmp_ctx == NULL) { + return LDB_ERR_OPERATIONS_ERROR; + } + ++ pav = talloc_zero(req, struct dsdb_control_password_acl_validation); ++ if (pav == NULL) { ++ talloc_free(tmp_ctx); ++ return LDB_ERR_OPERATIONS_ERROR; ++ } ++ + c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_CHANGE_OID); + if (c != NULL) { ++ pav->pwd_reset = false; ++ + /* + * The "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we + * have a user password change and not a set as the message +@@ -977,6 +986,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + + c = ldb_request_get_control(req, DSDB_CONTROL_PASSWORD_HASH_VALUES_OID); + if (c != NULL) { ++ pav->pwd_reset = true; ++ + /* + * The "DSDB_CONTROL_PASSWORD_HASH_VALUES_OID" control, without + * "DSDB_CONTROL_PASSWORD_CHANGE_OID" control means that we +@@ -1030,6 +1041,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + + + if (rep_attr_cnt > 0) { ++ pav->pwd_reset = true; ++ + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_FORCE_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +@@ -1038,6 +1051,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + } + + if (add_attr_cnt != del_attr_cnt) { ++ pav->pwd_reset = true; ++ + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_FORCE_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +@@ -1046,6 +1061,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + } + + if (add_val_cnt == 1 && del_val_cnt == 1) { ++ pav->pwd_reset = false; ++ + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_USER_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +@@ -1058,6 +1075,8 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + } + + if (add_val_cnt == 1 && del_val_cnt == 0) { ++ pav->pwd_reset = true; ++ + ret = acl_check_extended_right(tmp_ctx, sd, acl_user_token(module), + GUID_DRS_FORCE_CHANGE_PASSWORD, + SEC_ADS_CONTROL_ACCESS, +@@ -1069,6 +1088,14 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + goto checked; + } + ++ /* ++ * Everything else is handled by the password_hash module where it will ++ * fail, but with the correct error code when the module is again ++ * checking the attributes. As the change request will lack the ++ * DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID control, we can be sure that ++ * any modification attempt that went this way will be rejected. ++ */ ++ + talloc_free(tmp_ctx); + return LDB_SUCCESS; + +@@ -1078,11 +1105,19 @@ checked: + req->op.mod.message->dn, + true, + 10); ++ talloc_free(tmp_ctx); ++ return ret; + } +- talloc_free(tmp_ctx); +- return ret; +-} + ++ ret = ldb_request_add_control(req, ++ DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID, false, pav); ++ if (ret != LDB_SUCCESS) { ++ ldb_debug(ldb_module_get_ctx(module), LDB_DEBUG_ERROR, ++ "Unable to register ACL validation control!\n"); ++ return ret; ++ } ++ return LDB_SUCCESS; ++} + + static int acl_modify(struct ldb_module *module, struct ldb_request *req) + { +diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c +index 690bb98..de565bc 100644 +--- a/source4/dsdb/samdb/ldb_modules/password_hash.c ++++ b/source4/dsdb/samdb/ldb_modules/password_hash.c +@@ -2572,7 +2572,35 @@ static int setup_io(struct ph_context *ac, + /* On "add" we have only "password reset" */ + ac->pwd_reset = true; + } else if (ac->req->operation == LDB_MODIFY) { +- if (io->og.cleartext_utf8 || io->og.cleartext_utf16 ++ struct ldb_control *pav_ctrl = NULL; ++ struct dsdb_control_password_acl_validation *pav = NULL; ++ ++ pav_ctrl = ldb_request_get_control(ac->req, ++ DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID); ++ if (pav_ctrl != NULL) { ++ pav = talloc_get_type_abort(pav_ctrl->data, ++ struct dsdb_control_password_acl_validation); ++ } ++ ++ if (pav == NULL) { ++ bool ok; ++ ++ /* ++ * If the DSDB_CONTROL_PASSWORD_ACL_VALIDATION_OID ++ * control is missing, we require system access! ++ */ ++ ok = dsdb_module_am_system(ac->module); ++ if (!ok) { ++ return ldb_module_operr(ac->module); ++ } ++ } ++ ++ if (pav != NULL) { ++ /* ++ * We assume what the acl module has validated. ++ */ ++ ac->pwd_reset = pav->pwd_reset; ++ } else if (io->og.cleartext_utf8 || io->og.cleartext_utf16 + || io->og.nt_hash || io->og.lm_hash) { + /* If we have an old password specified then for sure it + * is a user "password change" */ +-- +1.9.1 + + +From 0815e8653277383918530f8dd8afaeadfe8085d5 Mon Sep 17 00:00:00 2001 +From: Ralph Boehme <slow@samba.org> +Date: Thu, 15 Feb 2018 23:11:38 +0100 +Subject: [PATCH 13/13] CVE-2018-1057: s4:dsdb/acl: changing dBCSPwd is only + allowed with a control + +This is not strictly needed to fig bug 13272, but it makes sense to also +fix this while fixing the overall ACL checking logic. + +Bug: https://bugzilla.samba.org/show_bug.cgi?id=13272 + +Signed-off-by: Ralph Boehme <slow@samba.org> +Reviewed-by: Stefan Metzmacher <metze@samba.org> +--- + source4/dsdb/samdb/ldb_modules/acl.c | 11 ++++++++++- + 1 file changed, 10 insertions(+), 1 deletion(-) + +diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c +index 17e1e67..8d9b780 100644 +--- a/source4/dsdb/samdb/ldb_modules/acl.c ++++ b/source4/dsdb/samdb/ldb_modules/acl.c +@@ -946,7 +946,7 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + struct ldb_message *msg; + struct ldb_control *c = NULL; + const char *passwordAttrs[] = { "userPassword", "clearTextPassword", +- "unicodePwd", "dBCSPwd", NULL }, **l; ++ "unicodePwd", NULL }, **l; + TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); + struct dsdb_control_password_acl_validation *pav = NULL; + +@@ -1006,6 +1006,15 @@ static int acl_check_password_rights(TALLOC_CTX *mem_ctx, + goto checked; + } + ++ el = ldb_msg_find_element(req->op.mod.message, "dBCSPwd"); ++ if (el != NULL) { ++ /* ++ * dBCSPwd is only allowed with a control. ++ */ ++ talloc_free(tmp_ctx); ++ return LDB_ERR_UNWILLING_TO_PERFORM; ++ } ++ + msg = ldb_msg_copy_shallow(tmp_ctx, req->op.mod.message); + if (msg == NULL) { + return ldb_module_oom(module); +-- +1.9.1 +
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201803140204.w2E24ahv099445>