From dff5d43907fd9825f0ba03dab9aa63c31f5a014c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 2 Jan 2018 15:56:03 -0800 Subject: [PATCH 01/14] CVE-2018-1050: s3: RPC: spoolss server. Protect against null pointer derefs. BUG: https://bugzilla.samba.org/show_bug.cgi?id=11343 Signed-off-by: Jeremy Allison --- source3/rpc_server/spoolss/srv_spoolss_nt.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c index a3c3861202d..fb56e2bf9a6 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -178,6 +178,11 @@ static void prune_printername_cache(void); static const char *canon_servername(const char *servername) { const char *pservername = servername; + + if (servername == NULL) { + return ""; + } + while (*pservername == '\\') { pservername++; } @@ -2073,6 +2078,10 @@ WERROR _spoolss_DeletePrinterDriver(struct pipes_struct *p, return WERR_ACCESS_DENIED; } + if (r->in.architecture == NULL || r->in.driver == NULL) { + return WERR_INVALID_ENVIRONMENT; + } + /* check that we have a valid driver name first */ if ((version = get_version_id(r->in.architecture)) == -1) { @@ -2212,6 +2221,10 @@ WERROR _spoolss_DeletePrinterDriverEx(struct pipes_struct *p, return WERR_ACCESS_DENIED; } + if (r->in.architecture == NULL || r->in.driver == NULL) { + return WERR_INVALID_ENVIRONMENT; + } + /* check that we have a valid driver name first */ if (get_version_id(r->in.architecture) == -1) { /* this is what NT returns */ -- 2.11.0 From e5774640b0b93f6b03fee0734241192b8a02b73a Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 12:43:09 +0100 Subject: [PATCH 02/14] 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 00000000000..343c5a7867d --- /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 db013ea46ad..be1f34d2312 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) -- 2.11.0 From 7eabe3d477fd9c439ad160f42f537f404019b420 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 10:56:06 +0100 Subject: [PATCH 03/14] 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 e5bee6fae53..dfe2a0ef9f7 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -3921,17 +3921,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!", @@ -3939,7 +3942,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!", -- 2.11.0 From abf925c2d10d29b9a68017f89b4970c8a2d43310 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 14:40:59 +0100 Subject: [PATCH 04/14] 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 dfe2a0ef9f7..457059ed69f 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -3922,6 +3922,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; @@ -3932,18 +3933,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!", -- 2.11.0 From d552abe8a2b34e528a3b810f3d1f600495e73466 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 17:38:31 +0100 Subject: [PATCH 05/14] 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 27d4e7659cc..edc6733c7ab 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, -- 2.11.0 From 99f46aabe571a96c3d5334929b069d8b81bd5d5d Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 17:38:31 +0100 Subject: [PATCH 06/14] 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 edc6733c7ab..c1655f9eeba 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, -- 2.11.0 From 6d5caffbf5fe546b2f648f957ca8a41b12214f3f Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 22:59:24 +0100 Subject: [PATCH 07/14] 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 c1655f9eeba..b2aa20f4157 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), -- 2.11.0 From 54c363e01210b478e1da74e446763153a61e56d0 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 17:43:43 +0100 Subject: [PATCH 08/14] 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 b2aa20f4157..4bf9779d507 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); -- 2.11.0 From 6c980a03e54293d3044d32b0715c384903ed2baf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 16 Feb 2018 15:17:26 +0100 Subject: [PATCH 09/14] 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 4bf9779d507..2c0aee41edf 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) { /* -- 2.11.0 From 67fa44aaf2d81001260af85ed803f0ff6444397d Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 22 Feb 2018 10:54:37 +0100 Subject: [PATCH 10/14] 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 343c5a7867d..00000000000 --- 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 2c0aee41edf..d27ec80461e 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; -- 2.11.0 From bb43ab081a539b088afb8c82658d2d8f3715bae8 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 14 Feb 2018 19:15:49 +0100 Subject: [PATCH 11/14] 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 d27ec80461e..f2259261588 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, -- 2.11.0 From 4adcba5f6aecacde5b405bdd1bdc662d303137e3 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 16 Feb 2018 15:30:13 +0100 Subject: [PATCH 12/14] 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 | 2 ++ 3 files changed, 12 insertions(+) diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index 586a3bfaf0e..6390258141e 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -182,6 +182,15 @@ struct dsdb_control_password_user_account_control { /* passed when we want to thoroughly delete linked attributes */ #define DSDB_CONTROL_REPLMD_VANISH_LINKS "1.3.6.1.4.1.7165.4.3.29" +/* + * 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 9df95c32a9d..7ecc9080cd1 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 04505de2daf..0a27f8a991e 100644 --- a/source4/setup/schema_samba4.ldif +++ b/source4/setup/schema_samba4.ldif @@ -217,6 +217,8 @@ #Allocated: DSDB_CONTROL_PASSWORD_USER_ACCOUNT_CONTROL_OID 1.3.6.1.4.1.7165.4.3.27 #Allocated: DSDB_CONTROL_SKIP_DUPLICATES_CHECK_OID 1.3.6.1.4.1.7165.4.3.28 #Allocated: DSDB_CONTROL_REPLMD_VANISH_LINKS 1.3.6.1.4.1.7165.4.3.29 +#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 -- 2.11.0 From e5b8c81d2951401691ea6a5e8082edf81354d1a1 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 16 Feb 2018 15:38:19 +0100 Subject: [PATCH 13/14] 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 f2259261588..9b4be7b6909 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 457059ed69f..06abf66f1a7 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -3188,7 +3188,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" */ -- 2.11.0 From 36639815f34c374f6d6cf002e19ba1754035e7d1 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 15 Feb 2018 23:11:38 +0100 Subject: [PATCH 14/14] 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 9b4be7b6909..d750362c47f 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); -- 2.11.0