From 9f9db5815d380bcb3ed9509cfb3b448600e93b48 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 fcc491e7662..3d99470b3d2 100644 --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c @@ -142,6 +142,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++; } @@ -2042,6 +2047,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) { @@ -2181,6 +2190,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 d8de52b979bbe071e6cc57405d5bc428f2b4ea6f 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 5ad58a9f0d8826e14b7acd82c133c3579d53639b 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 96113b542d9..ceb0a4f2257 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -4234,17 +4234,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!", @@ -4252,7 +4255,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 e2acd0d259d7200486d06c16abdc3232c8f1c3f2 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 ceb0a4f2257..e8af7e81c19 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -4235,6 +4235,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; @@ -4245,18 +4246,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 50eb427844493630f835de0cddb4f9d3c83d20aa 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 31088faf09fd5dc71d0dcc050628f68c43d0981e 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 32384ea708c3e529c2f3d83169aba4945aa577fd 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 a6221ea75e4d333e704d3df2f5df37f394faf19f 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 bb2ab8e3b86fed1c284919fec1b5455de8d36b91 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 946bab0ad8311912433b873501b877c334586f09 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 32d65d8ba602cc426da668a99d027db954f26cfe 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 f11f3ccb776f9055d8911326903c93a04dc1ca44 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 | 1 + 3 files changed, 11 insertions(+) diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index 894df0f17e1..8790ff5abfd 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -194,6 +194,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 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 5dcbe9ac5ff..0e2b6ecceb6 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 -- 2.11.0 From 86b41e9c618a8b0455a5ea571a9078fcaddc4eec 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 e8af7e81c19..3a2582106d3 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -3500,7 +3500,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 11fbafc9169c0f8442e84c3f4a48ad3ced84a550 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