[PATCH] Fix pwdLastSet behaviour in regards to Windows

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Sun Jan 17 23:28:53 UTC 2016


hi Adrian,

This is good. Some comments below.

On 18/01/16 10:00, adrianc at catalyst.net.nz wrote:
> Here's a patch for fixing https://bugzilla.samba.org/show_bug.cgi?id=9654
> 
> 0001-samdb-fix-pwdLastSet-behaviour-in-regards-to-Windows.patch
> 
> 
> From edd0c0de49c318535653fc9aab27734e1b36e6e7 Mon Sep 17 00:00:00 2001
> From: Adrian Cochrane <adrianc at catalyst.net.nz>
> Date: Thu, 14 Jan 2016 14:13:49 +1300
> Subject: [PATCH] samdb: fix pwdLastSet behaviour in regards to Windows
> 
> Windows interprets a -1 in pwdLastSet as the current time, this patch
> emulates that behaviour.
> 
> Fixes bug #9654
> 
> Signed-off-by: Adrian Cochrane <adrianc at catalyst.net.nz>
> ---
>  source4/dsdb/samdb/ldb_modules/operational.c | 51 ++++++++++++++++++++++
>  source4/dsdb/tests/python/passwords.py       | 64 ++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/source4/dsdb/samdb/ldb_modules/operational.c b/source4/dsdb/samdb/ldb_modules/operational.c
> index c5fd8e2..5a6eff9 100644
> --- a/source4/dsdb/samdb/ldb_modules/operational.c
> +++ b/source4/dsdb/samdb/ldb_modules/operational.c
> @@ -845,6 +845,35 @@ static int construct_msds_user_password_expiry_time_computed(struct ldb_module *
>  				   password_expiry_time);
>  }
>  
> +static int modify_pwdLastSet(const struct ldb_message *msg) {

This function is returns int, but the value it returns is only ever
LDB_SUCCESS. Which would be annoyingly tautological, but fortunately I
found some error cases below :)

Also the curly bracket should be on a new line, here and below.

> +	struct ldb_message_element *pwdLastSet_element = NULL;
> +	struct ldb_val *pwdLastSet_value = NULL;
> +	int i;
> +
> +	for (i = 0; i < msg->num_elements; i++) {
> +		if (strcmp(msg->elements[i].name, "pwdLastSet") == 0) {
> +			pwdLastSet_element = &msg->elements[i];
> +			break;
> +		}
> +	}
> +	if (pwdLastSet_element == NULL || pwdLastSet_element->num_values != 1) {
> +		return LDB_SUCCESS;
> +	}

Should `pwdLastSet_element->num_values != 1` count as success?

> +
> +	pwdLastSet_value = &pwdLastSet_element->values[0];
> +	if (ldb_val_string_cmp(pwdLastSet_value, "-1") == 0) {
> +		NTTIME now;
> +		char *now_s = NULL;
> +		unix_to_nt_time(&now, time(NULL));
> +		asprintf(&now_s, "%" PRIu64, (uint64_t) now);

This should be talloc_asprintf(). And check for errors.

> +

There's rogue whitespace here.

> +		pwdLastSet_value->data = (uint8_t*) now_s;
> +		pwdLastSet_value->length = strlen(now_s);
> +	}
> +
> +	return LDB_SUCCESS;
> +}
> +
>  
>  struct op_controls_flags {
>  	bool sd;
> @@ -1301,6 +1330,26 @@ static int operational_search(struct ldb_module *module, struct ldb_request *req
>  	return ldb_next_request(module, down_req);
>  }
>  
> +/* These callbacks are here so that a -1 in pwdLastSet can translate to the
> + * time it was modified. */
> +static int operational_add(struct ldb_module *ctx, struct ldb_request *req) {
> +	int ret = modify_pwdLastSet(req->op.add.message);
> +
> +	if (ret == LDB_SUCCESS) {
> +		return ldb_next_request(ctx, req);
> +	}
> +	return ret;
> +}
> +
> +static int operational_modify(struct ldb_module *ctx, struct ldb_request *req) {
> +	int ret = modify_pwdLastSet(req->op.mod.message);
> +
> +	if (ret == LDB_SUCCESS) {
> +		return ldb_next_request(ctx, req);
> +	}
> +	return ret;
> +}
> +
>  static int operational_init(struct ldb_module *ctx)
>  {
>  	struct operational_data *data;
> @@ -1325,6 +1374,8 @@ static int operational_init(struct ldb_module *ctx)
>  static const struct ldb_module_ops ldb_operational_module_ops = {
>  	.name              = "operational",
>  	.search            = operational_search,
> +	.add		   = operational_add,
> +	.modify		   = operational_modify,
>  	.init_context	   = operational_init
>  };
>  
> diff --git a/source4/dsdb/tests/python/passwords.py b/source4/dsdb/tests/python/passwords.py
> index fb3eee5..cb70372 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
> @@ -931,6 +933,68 @@ userPassword: thatsAcomplPASS4
>          # Reset the "minPwdLength" as it was before
>          self.ldb.set_minPwdLength(minPwdLength)
>  
> +    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)
> +
> +        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.assertTrue(now - 200000000 <= int(res1[0]["pwdLastSet"][0]) <= now + 200000000)

Watch the line length...

> +
> +        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.assertTrue(now - 200000000 <= int(res1[0]["pwdLastSet"][0]) <= now + 200000000)

...and here also.

> +
> +        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)
> -- 

cheers,
Douglas



More information about the samba-technical mailing list