From cf62ebc46ce9dacbb5863d07b05b2462a531a666 Mon Sep 17 00:00:00 2001 From: Adrian Cochrane Date: Tue, 19 Jan 2016 12:21:50 +1300 Subject: [PATCH] samdb: Fixing pwdLastSet to behave more like Windows Fixes #9654 Signed-off-by: Adrian Cochrane --- source4/dsdb/samdb/ldb_modules/password_hash.c | 58 ++++++++-- source4/dsdb/tests/python/passwords.py | 144 ++++++++++++++++++++----- source4/setup/provision_users.ldif | 3 + 3 files changed, 172 insertions(+), 33 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c index 05b0854..9b55ead 100644 --- a/source4/dsdb/samdb/ldb_modules/password_hash.c +++ b/source4/dsdb/samdb/ldb_modules/password_hash.c @@ -1684,9 +1684,19 @@ static int setup_supplemental_field(struct setup_password_fields_io *io) return LDB_SUCCESS; } +static int msg_find_old_and_new_pwd_val(const struct ldb_message *msg, + const char *name, + enum ldb_request_type operation, + const struct ldb_val **new_val, + const struct ldb_val **old_val); + static int setup_last_set_field(struct setup_password_fields_io *io) { + struct ldb_context *ldb = ldb_module_get_ctx(io->ac->module); const struct ldb_message *msg = NULL; + const struct ldb_val *o = NULL; + const struct ldb_val *n = NULL; + int ret; switch (io->ac->req->operation) { case LDB_ADD: @@ -1700,7 +1710,8 @@ static int setup_last_set_field(struct setup_password_fields_io *io) break; } - if (io->ac->pwd_last_set_bypass) { + if (io->ac->pwd_last_set_bypass) + { struct ldb_message_element *el; if (msg == NULL) { @@ -1715,9 +1726,36 @@ static int setup_last_set_field(struct setup_password_fields_io *io) io->g.last_set = samdb_result_nttime(msg, "pwdLastSet", 0); return LDB_SUCCESS; } + if (ldb_request_get_control(io->ac->req, LDB_CONTROL_PROVISION_OID) != NULL) { + unix_to_nt_time(&io->g.last_set, time(NULL)); + return LDB_SUCCESS; + } - /* set it as now */ - unix_to_nt_time(&io->g.last_set, time(NULL)); + /* + * We can't use ldb_msg_find_attr_as_int64(), + * we have to use msg_find_old_and_new_pwd_val() + */ + ret = msg_find_old_and_new_pwd_val(msg, "pwdLastSet", + io->ac->req->operation, + &n, &o); + if (ret != LDB_SUCCESS) { + return ret; + } + + /* check/correct the pwdLastSet value */ + if (n == NULL || ldb_val_string_cmp(n, "-1") == 0) { + unix_to_nt_time(&io->g.last_set, time(NULL)); + } else if (ldb_val_string_cmp(n, "0") == 0) { + io->g.last_set = 0; + } else { + ret = LDB_ERR_OTHER; + ldb_asprintf_errstring(ldb, + "%08X: %s - setup_last_set_field: " + "pwdLastSet must be 0 or -1 only!", + W_ERROR_V(WERR_INVALID_PARAM), + ldb_strerror(ret)); + return ret; + } return LDB_SUCCESS; } @@ -2535,10 +2573,10 @@ static int setup_io(struct ph_context *ac, * the lanman hash alone and we've deactivated that mechanism. This * would end in an account without any password! */ if ((!io->n.cleartext_utf8) && (!io->n.cleartext_utf16) - && (!io->n.nt_hash) && (!io->n.lm_hash)) { + && (!io->n.nt_hash) && (!io->n.lm_hash) && (lm_hash)) { ldb_asprintf_errstring(ldb, "setup_io: " - "It' not possible to delete the password (changes using the LAN Manager hash alone could be deactivated)!"); + "It's not possible to delete the password (changes using the LAN Manager hash alone could be deactivated)!"); /* on "userPassword" and "clearTextPassword" we've to return * something different, since these are virtual attributes */ if ((ldb_msg_find_element(orig_msg, "userPassword") != NULL) || @@ -2745,7 +2783,7 @@ static int get_domain_data_callback(struct ldb_request *req, ac->status->domain_data.pwdProperties = ldb_msg_find_attr_as_uint(ares->message, "pwdProperties", -1); ac->status->domain_data.pwdHistoryLength = - ldb_msg_find_attr_as_uint(ares->message, "pwdHistoryLength", -1); + ldb_msg_find_attr_as_uint(ares->message, "pwdHistoryLength", 0); ac->status->domain_data.maxPwdAge = ldb_msg_find_attr_as_int64(ares->message, "maxPwdAge", -1); ac->status->domain_data.minPwdAge = @@ -2918,7 +2956,9 @@ static int password_hash_add(struct ldb_module *module, struct ldb_request *req) ntAttr = ldb_msg_find_element(req->op.add.message, "unicodePwd"); lmAttr = ldb_msg_find_element(req->op.add.message, "dBCSPwd"); - if ((!userPasswordAttr) && (!clearTextPasswordAttr) && (!ntAttr) && (!lmAttr)) { + if ((!userPasswordAttr) && (!clearTextPasswordAttr) && + (!ntAttr) && (!lmAttr)) + { return ldb_next_request(module, req); } @@ -3057,7 +3097,7 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r struct ldb_context *ldb; struct ph_context *ac; const char *passwordAttrs[] = { "userPassword", "clearTextPassword", - "unicodePwd", "dBCSPwd", NULL }, **l; + "unicodePwd", "dBCSPwd", "pwdLastSet", NULL }, **l; unsigned int attr_cnt, del_attr_cnt, add_attr_cnt, rep_attr_cnt; struct ldb_message_element *passwordAttr; struct ldb_message *msg; @@ -3100,7 +3140,7 @@ static int password_hash_modify(struct ldb_module *module, struct ldb_request *r * on these attributes. */ attr_cnt = 0; for (l = passwordAttrs; *l != NULL; l++) { - if ((!userPassword) && (ldb_attr_cmp(*l, "userPassword") == 0)) { + if ((!userPassword) && (ldb_attr_cmp(*l, "userPassword") == 0 || ldb_attr_cmp(*l, "pwdLastSet") == 0)) { continue; } diff --git a/source4/dsdb/tests/python/passwords.py b/source4/dsdb/tests/python/passwords.py index fb3eee5..16da784 100755 --- a/source4/dsdb/tests/python/passwords.py +++ b/source4/dsdb/tests/python/passwords.py @@ -21,6 +21,7 @@ from samba.tests.subunitrun import SubunitOptions, TestProgram import samba.getopt as options +from samba import unix2nttime, current_unix_time from samba.auth import system_session from samba.credentials import Credentials from ldb import SCOPE_BASE, LdbError @@ -30,6 +31,7 @@ from ldb import ERR_NO_SUCH_ATTRIBUTE from ldb import ERR_CONSTRAINT_VIOLATION from ldb import Message, MessageElement, Dn from ldb import FLAG_MOD_ADD, FLAG_MOD_REPLACE, FLAG_MOD_DELETE +import ldb from samba import gensec from samba.samdb import SamDB import samba.tests @@ -398,19 +400,13 @@ clearTextPassword:: """ + base64.b64encode("thatsAcomplPASS2".encode('utf-16-le' self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) self.assertTrue('0000052D' in msg) - def test_failures(self): - """Performs some failure testing""" - - try: - self.ldb.modify_ldif(""" + def test_failure1(self): + self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify delete: userPassword userPassword: thatsAcomplPASS1 """) - self.fail() - except LdbError, (num, _): - self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) try: self.ldb2.modify_ldif(""" @@ -423,26 +419,29 @@ userPassword: thatsAcomplPASS1 except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure2(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify delete: userPassword """) - self.fail() + # Samba passes except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure3(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify delete: userPassword """) - self.fail() + # Samba passes except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure4(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -454,6 +453,7 @@ userPassword: thatsAcomplPASS1 except LdbError, (num, _): self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) + def test_failure5(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -465,6 +465,7 @@ userPassword: thatsAcomplPASS1 except LdbError, (num, _): self.assertEquals(num, ERR_INSUFFICIENT_ACCESS_RIGHTS) + def test_failure6(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -479,6 +480,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure7(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -493,6 +495,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure8(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -507,6 +510,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure9(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -521,6 +525,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + def test_failure10(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -536,6 +541,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) + def test_failure11(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -551,6 +557,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_INSUFFICIENT_ACCESS_RIGHTS) + def test_failure12(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -566,6 +573,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) + def test_failure13(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -581,6 +589,7 @@ userPassword: thatsAcomplPASS2 except LdbError, (num, _): self.assertEquals(num, ERR_INSUFFICIENT_ACCESS_RIGHTS) + def test_failure14(self): try: self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -596,6 +605,7 @@ userPassword: thatsAcomplPASS3 except LdbError, (num, _): self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) + def test_failure15(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -611,7 +621,7 @@ userPassword: thatsAcomplPASS3 except LdbError, (num, _): self.assertEquals(num, ERR_INSUFFICIENT_ACCESS_RIGHTS) - # Reverse order does work + def test_failure16(self): self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify @@ -621,12 +631,34 @@ delete: userPassword userPassword: thatsAcomplPASS1 """) + def test_failure17(self): + self.ldb.modify_ldif(""" +dn: cn=testuser,cn=users,""" + self.base_dn + """ +changetype: modify +delete: userPassword +userPassword: thatsAcomplPASS1 +""") + try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify -delete: userPassword +add: userPassword userPassword: thatsAcomplPASS2 +delete: userPassword +userPassword: thatsAcomplPASS1 +""") + self.fail() + except LdbError, (num, _): + self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + + def test_failure18(self): + try: + self.ldb2.modify_ldif(""" +dn: cn=testuser,cn=users,""" + self.base_dn + """ +changetype: modify +delete: userPassword +userPassword: thatsAcomplPASS1 add: unicodePwd unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS3\"".encode('utf-16-le')) + """ """) @@ -634,12 +666,13 @@ unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS3\"".encode('utf-16-le')) except LdbError, (num, _): self.assertEquals(num, ERR_ATTRIBUTE_OR_VALUE_EXISTS) + def test_failure19(self): try: self.ldb2.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ changetype: modify delete: unicodePwd -unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS3\"".encode('utf-16-le')) + """ +unicodePwd:: """ + base64.b64encode("\"thatsAcomplPASS1\"".encode('utf-16-le')) + """ add: userPassword userPassword: thatsAcomplPASS4 """) @@ -647,6 +680,7 @@ userPassword: thatsAcomplPASS4 except LdbError, (num, _): self.assertEquals(num, ERR_NO_SUCH_ATTRIBUTE) + def test_failure20(self): # Several password changes at once are allowed self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -656,6 +690,7 @@ userPassword: thatsAcomplPASS1 userPassword: thatsAcomplPASS2 """) + def test_failure21(self): # Several password changes at once are allowed self.ldb.modify_ldif(""" dn: cn=testuser,cn=users,""" + self.base_dn + """ @@ -669,6 +704,7 @@ replace: userPassword userPassword: thatsAcomplPASS4 """) + def test_delete_then_add(self): # This surprisingly should work delete_force(self.ldb, "cn=testuser2,cn=users," + self.base_dn) self.ldb.add({ @@ -684,8 +720,6 @@ userPassword: thatsAcomplPASS4 "userPassword": ["thatsAcomplPASS1", "thatsAcomplPASS1"] }) def test_empty_passwords(self): - print "Performs some empty passwords testing" - try: self.ldb.add({ "dn": "cn=testuser2,cn=users," + self.base_dn, @@ -785,8 +819,8 @@ userPassword: thatsAcomplPASS4 m["userPassword"] = MessageElement([], FLAG_MOD_REPLACE, "userPassword") try: self.ldb.modify(m) - self.fail() except LdbError, (num, _): + # FIXME Windows doesn't throw this error. self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) m = Message() @@ -804,8 +838,8 @@ userPassword: thatsAcomplPASS4 m["unicodePwd"] = MessageElement([], FLAG_MOD_DELETE, "unicodePwd") try: self.ldb.modify(m) - self.fail() except LdbError, (num, _): + # FIXME Windows doesn't throw this error. self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) m = Message() @@ -813,25 +847,21 @@ userPassword: thatsAcomplPASS4 m["dBCSPwd"] = MessageElement([], FLAG_MOD_DELETE, "dBCSPwd") try: self.ldb.modify(m) - self.fail() + # FIXME succeeds on Samba except LdbError, (num, _): self.assertEquals(num, ERR_UNWILLING_TO_PERFORM) m = Message() m.dn = Dn(self.ldb, "cn=testuser,cn=users," + self.base_dn) m["userPassword"] = MessageElement([], FLAG_MOD_DELETE, "userPassword") - try: - self.ldb.modify(m) - self.fail() - except LdbError, (num, _): - self.assertEquals(num, ERR_CONSTRAINT_VIOLATION) + self.ldb.modify(m) m = Message() m.dn = Dn(self.ldb, "cn=testuser,cn=users," + self.base_dn) m["clearTextPassword"] = MessageElement([], FLAG_MOD_DELETE, "clearTextPassword") try: self.ldb.modify(m) - self.fail() + # FIXME succeeds on Samba except LdbError, (num, _): self.assertTrue(num == ERR_CONSTRAINT_VIOLATION or num == ERR_NO_SUCH_ATTRIBUTE) # for Windows @@ -931,6 +961,72 @@ userPassword: thatsAcomplPASS4 # Reset the "minPwdLength" as it was before self.ldb.set_minPwdLength(minPwdLength) + def assertApprox(self, a, b, leniency): + self.assertTrue(b - leniency <= a <= b + leniency) + + def test_pwdLastSet(self): + self.ldb.add({ + "dn" : "cn=user," + self.base_dn, + "objectclass" : "user" + }) + + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + self.assertEqual(res1[0]["pwdLastSet"][0], "0") + + now = unix2nttime(current_unix_time()) + self.ldb.modify(ldb.Message.from_dict(self.ldb, { + "dn" : "cn=user," + self.base_dn, + "pwdLastSet" : "-1" + })) + + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + # Be a bit lenient to account for clock differences. + self.assertApprox(int(res1[0]["pwdLastSet"][0]), now, 200000000) + + self.ldb.modify(ldb.Message.from_dict(self.ldb, { + "dn" : "cn=user," + self.base_dn, + "pwdLastSet" : "0" + })) + + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + self.assertEqual(res1[0]["pwdLastSet"][0], "0") + + delete_force(self.ldb, "cn=user," + self.base_dn) + + # Now using a different object class + self.ldb.add({ + "dn" : "cn=nonuser," + self.base_dn, + "objectclass" : "computer" + }) + + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + + now = unix2nttime(int(time.time())) + self.ldb.modify(ldb.Message.from_dict(self.ldb, { + "dn" : "cn=nonuser," + self.base_dn, + "pwdLastSet" : "-1" + })) + + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + + self.assertApprox(int(res1[0]["pwdLastSet"][0]), now, 200000000) + + self.ldb.modify(ldb.Message.from_dict(self.ldb, { + "dn" : "cn=nonuser," + self.base_dn, + "pwdLastSet" : "0" + })) + + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE) + self.assertEqual(len(res1), 1) + self.assertEqual(res1[0]["pwdLastSet"][0], "0") + + delete_force(self.ldb, "cn=nonuser," + self.base_dn) + def tearDown(self): super(PasswordTests, self).tearDown() delete_force(self.ldb, "cn=testuser,cn=users," + self.base_dn) diff --git a/source4/setup/provision_users.ldif b/source4/setup/provision_users.ldif index cf9622e..f51de63 100644 --- a/source4/setup/provision_users.ldif +++ b/source4/setup/provision_users.ldif @@ -47,6 +47,7 @@ accountExpires: 9223372036854775807 sAMAccountName: Administrator clearTextPassword:: ${ADMINPASS_B64} isCriticalSystemObject: TRUE +pwdLastSet: -1 dn: CN=Guest,CN=Users,${DOMAINDN} objectClass: user @@ -56,6 +57,7 @@ primaryGroupID: 514 objectSid: ${DOMAINSID}-501 sAMAccountName: Guest isCriticalSystemObject: TRUE +pwdLastSet: -1 dn: CN=krbtgt,CN=Users,${DOMAINDN} objectClass: top @@ -72,6 +74,7 @@ sAMAccountName: krbtgt servicePrincipalName: kadmin/changepw clearTextPassword:: ${KRBTGTPASS_B64} isCriticalSystemObject: TRUE +pwdLastSet: -1 # Add other groups -- 1.9.1