The branch, master has been updated
via af3c6a4... s4:passwords.py - proof the most important extended
error codes
via 3fcd762... s4:samdb_set_password - implement the extended LDAP
error code detection
via 2dbff00... s4:password_hash LDB module - introduce the extended
LDAP error codes on the important failure cases
via 33bb063... s4:password_hash LDB module - support this new password
set syntax
via 6dc0c07... s4:passwords.py - another special password test
via 28cfae7... s4:password_hash LDB module - allow to compare against
both NT and LM hashes on password change operations
via 9476c43... s4:ldap_backend.c - Windows returns WERROR codes in
majuscule HEX format
via fb58c0f... s4:ldap_backend.c - map error codes - add a change which
allows custom WERROR codes
from 08b628e... s3: Remove some unused code
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit af3c6a42426241704580f4440b09a8c155d212df
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 18:19:52 2010 +0200
s4:passwords.py - proof the most important extended error codes
commit 3fcd76237d1c621e6bb876c4c33706e0db2056e2
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 18:05:29 2010 +0200
s4:samdb_set_password - implement the extended LDAP error code detection
commit 2dbff00b6dd3affc95c717296d52343daf49361b
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 17:38:47 2010 +0200
s4:password_hash LDB module - introduce the extended LDAP error codes on
the important failure cases
commit 33bb063b053c24a84fdd13b866d1f80a964aeabf
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 10:20:48 2010 +0200
s4:password_hash LDB module - support this new password set syntax
commit 6dc0c07a51ee3d26ffc44e6178b6ae842190bd19
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 10:02:43 2010 +0200
s4:passwords.py - another special password test
This looks like a password change but it's rather a password set operation.
commit 28cfae774edf7bd4e2c4d9285b0d0508bee64284
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 09:36:25 2010 +0200
s4:password_hash LDB module - allow to compare against both NT and LM
hashes on password change operations
This is to match the SAMR password change behaviour.
commit 9476c43967af66e854cfda13bf09e21da7e63a44
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 18:45:09 2010 +0200
s4:ldap_backend.c - Windows returns WERROR codes in majuscule HEX format
commit fb58c0f36575510ca6572e695afdd81469ced3dd
Author: Matthias Dieter Wallnöfer <[email protected]>
Date: Sun Aug 15 09:25:58 2010 +0200
s4:ldap_backend.c - map error codes - add a change which allows custom
WERROR codes
This is strictly needed by my recent passwords work, since I want to remove
most of the password change stuff in "samr_password.c". Since AD gives us
CONSTRAINT_VIOLATION on all change problems I cannot distinguish on the SAMR
level which the real cause was about. Therefore I need the extended WERROR
codes
here.
-----------------------------------------------------------------------
Summary of changes:
source4/dsdb/common/util.c | 26 +++--
source4/dsdb/samdb/ldb_modules/password_hash.c | 138 ++++++++++++++----------
source4/dsdb/tests/python/passwords.py | 53 +++++++--
source4/ldap_server/ldap_backend.c | 15 +++-
4 files changed, 155 insertions(+), 77 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index be8e3a9..3ce0b2c 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -2021,7 +2021,7 @@ NTSTATUS samdb_set_password(struct ldb_context *ldb,
TALLOC_CTX *mem_ctx,
struct ldb_request *req;
struct dsdb_control_password_change_status *pwd_stat = NULL;
int ret;
- NTSTATUS status;
+ NTSTATUS status = NT_STATUS_OK;
#define CHECK_RET(x) \
if (x != LDB_SUCCESS) { \
@@ -2141,18 +2141,26 @@ NTSTATUS samdb_set_password(struct ldb_context *ldb,
TALLOC_CTX *mem_ctx,
talloc_free(pwd_stat);
}
- /* TODO: Error results taken from "password_hash" module. Are they
- correct? */
- if (ret == LDB_ERR_UNWILLING_TO_PERFORM) {
- status = NT_STATUS_WRONG_PASSWORD;
- } else if (ret == LDB_ERR_CONSTRAINT_VIOLATION) {
- status = NT_STATUS_PASSWORD_RESTRICTION;
+ if (ret == LDB_ERR_CONSTRAINT_VIOLATION) {
+ const char *errmsg = ldb_errstring(ldb);
+ char *endptr = NULL;
+ WERROR werr = WERR_GENERAL_FAILURE;
+ status = NT_STATUS_UNSUCCESSFUL;
+ if (errmsg != NULL) {
+ werr = W_ERROR(strtol(errmsg, &endptr, 16));
+ }
+ if (endptr != errmsg) {
+ if (W_ERROR_EQUAL(werr, WERR_INVALID_PASSWORD)) {
+ status = NT_STATUS_WRONG_PASSWORD;
+ }
+ if (W_ERROR_EQUAL(werr, WERR_PASSWORD_RESTRICTION)) {
+ status = NT_STATUS_PASSWORD_RESTRICTION;
}
+ }
} else if (ret == LDB_ERR_NO_SUCH_OBJECT) {
+ /* don't let the caller know if an account doesn't exist */
status = NT_STATUS_WRONG_PASSWORD;
} else if (ret != LDB_SUCCESS) {
status = NT_STATUS_UNSUCCESSFUL;
- } else {
- status = NT_STATUS_OK;
}
return status;
diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c
b/source4/dsdb/samdb/ldb_modules/password_hash.c
index cf239fb..60f0c3e 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -1439,7 +1439,7 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
if (!io->ac->pwd_reset && !io->ac->change_old_pw_checked) {
bool nt_hash_checked = false;
- /* we need to old nt or lm hash given by the client */
+ /* we need the old nt or lm hash given by the client */
if (!io->og.nt_hash && !io->og.lm_hash) {
ldb_asprintf_errstring(ldb,
"check_password_restrictions: "
@@ -1452,18 +1452,24 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
has no problems at all */
if (io->og.nt_hash) {
if (!io->o.nt_hash) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
+ "%08X: %s -
check_password_restrictions: "
"There's no old nt_hash, which is
needed "
- "in order to change your password!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "in order to change your password!",
+ W_ERROR_V(WERR_INVALID_PASSWORD),
+ ldb_strerror(ret));
+ return ret;
}
if (memcmp(io->og.nt_hash->hash, io->o.nt_hash->hash,
16) != 0) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "The old password specified doesn't
match!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s -
check_password_restrictions: "
+ "The old password specified doesn't
match!",
+ W_ERROR_V(WERR_INVALID_PASSWORD),
+ ldb_strerror(ret));
+ return ret;
}
nt_hash_checked = true;
@@ -1475,19 +1481,25 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
* (as the SAMR operations request it). */
if (io->og.lm_hash) {
if (!io->o.lm_hash && !nt_hash_checked) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
+ "%08X: %s -
check_password_restrictions: "
"There's no old lm_hash, which is
needed "
- "in order to change your password!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "in order to change your password!",
+ W_ERROR_V(WERR_INVALID_PASSWORD),
+ ldb_strerror(ret));
+ return ret;
}
if (io->o.lm_hash &&
memcmp(io->og.lm_hash->hash, io->o.lm_hash->hash,
16) != 0) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "The old password specified doesn't
match!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s -
check_password_restrictions: "
+ "The old password specified doesn't
match!",
+ W_ERROR_V(WERR_INVALID_PASSWORD),
+ ldb_strerror(ret));
+ return ret;
}
}
}
@@ -1512,28 +1524,34 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
break;
case SAMR_VALIDATION_STATUS_PWD_TOO_SHORT:
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "the password is too short. It should be equal
or longer than %i characters!",
+ "%08X: %s - check_password_restrictions: "
+ "the password is too short. It should be equal
or longer than %u characters!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret),
io->ac->status->domain_data.minPwdLength);
-
io->ac->status->reject_reason =
SAM_PWD_CHANGE_PASSWORD_TOO_SHORT;
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ return ret;
case SAMR_VALIDATION_STATUS_NOT_COMPLEX_ENOUGH:
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "the password does not meet the complexity
criterias!");
+ "%08X: %s - check_password_restrictions: "
+ "the password does not meet the complexity
criterias!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
io->ac->status->reject_reason =
SAM_PWD_CHANGE_NOT_COMPLEX;
-
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ return ret;
default:
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "the password doesn't fit by a certain
reason!");
-
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s - check_password_restrictions: "
+ "the password doesn't fit by a certain reason!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
+ return ret;
}
}
@@ -1548,13 +1566,14 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
for (i = 0; i < io->o.nt_history_len; i++) {
ret = memcmp(io->n.nt_hash, io->o.nt_history[i].hash,
16);
if (ret == 0) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "the password was already used (in
history)!");
-
+ "%08X: %s -
check_password_restrictions: "
+ "the password was already used (in
history)!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
io->ac->status->reject_reason =
SAM_PWD_CHANGE_PWD_IN_HISTORY;
-
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ return ret;
}
}
}
@@ -1566,39 +1585,49 @@ static int check_password_restrictions(struct
setup_password_fields_io *io)
for (i = 0; i < io->o.lm_history_len; i++) {
ret = memcmp(io->n.nt_hash, io->o.lm_history[i].hash,
16);
if (ret == 0) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "the password was already used (in
history)!");
-
+ "%08X: %s -
check_password_restrictions: "
+ "the password was already used (in
history)!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
io->ac->status->reject_reason =
SAM_PWD_CHANGE_PWD_IN_HISTORY;
-
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ return ret;
}
}
}
/* are all password changes disallowed? */
if (io->ac->status->domain_data.pwdProperties &
DOMAIN_REFUSE_PASSWORD_CHANGE) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "password changes disabled!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s - check_password_restrictions: "
+ "password changes disabled!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
+ return ret;
}
/* can this user change the password? */
if (io->u.userAccountControl & UF_PASSWD_CANT_CHANGE) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "password can't be changed on this account!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s - check_password_restrictions: "
+ "password can't be changed on this account!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
+ return ret;
}
/* Password minimum age: yes, this is a minus. The ages are in negative
100nsec units! */
if (io->u.pwdLastSet - io->ac->status->domain_data.minPwdAge >
io->g.last_set) {
+ ret = LDB_ERR_CONSTRAINT_VIOLATION;
ldb_asprintf_errstring(ldb,
- "check_password_restrictions: "
- "password is too young to change!");
- return LDB_ERR_CONSTRAINT_VIOLATION;
+ "%08X: %s - check_password_restrictions: "
+ "password is too young to change!",
+ W_ERROR_V(WERR_PASSWORD_RESTRICTION),
+ ldb_strerror(ret));
+ return ret;
}
return LDB_SUCCESS;
@@ -1878,15 +1907,6 @@ static int setup_io(struct ph_context *ac,
return LDB_ERR_UNWILLING_TO_PERFORM;
}
- /* refuse the change if someone wants to compare against both
- * hashes at the same time for a "password modify" operation... */
- if (io->og.nt_hash && io->og.lm_hash) {
- ldb_asprintf_errstring(ldb,
- "setup_io: "
- "it's only allowed to provide the old password in hash
format as 'unicodePwd' or as 'dBCSPwd'");
- return LDB_ERR_UNWILLING_TO_PERFORM;
- }
-
/* Decides if we have a password modify or password reset operation */
if (ac->req->operation == LDB_ADD) {
/* On "add" we have only "password reset" */
@@ -2440,10 +2460,18 @@ static int password_hash_modify(struct ldb_module
*module, struct ldb_request *r
++rep_attr_cnt;
}
if ((passwordAttr->num_values != 1) &&
- (passwordAttr->flags != LDB_FLAG_MOD_REPLACE)) {
+ (passwordAttr->flags == 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) &&
+ (passwordAttr->flags == LDB_FLAG_MOD_DELETE)) {
talloc_free(ac);
ldb_asprintf_errstring(ldb,
- "'%s' attributes must
have exactly one value!",
+ "'%s' attribute must
have zero or one value(s) on delete operations!",
*l);
return LDB_ERR_CONSTRAINT_VIOLATION;
}
diff --git a/source4/dsdb/tests/python/passwords.py
b/source4/dsdb/tests/python/passwords.py
index a8a19e1..314d71b 100755
--- a/source4/dsdb/tests/python/passwords.py
+++ b/source4/dsdb/tests/python/passwords.py
@@ -86,27 +86,50 @@ class PasswordTests(samba.tests.TestCase):
"objectclass": ["user", "person"],
"sAMAccountName": "testuser"})
- # Tests a password change when we don't have a password yet
+ # Tests a password change when we don't have any password yet with a
+ # wrong old password
try:
self.ldb.modify_ldif("""
dn: cn=testuser,cn=users,""" + self.base_dn + """
changetype: modify
delete: userPassword
-userPassword: thatsAcomplPASS1
+userPassword: noPassword
add: userPassword
userPassword: thatsAcomplPASS2
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
-
- # Sets the initial user password and enables the account
+ # Windows (2008 at least) seems to have some small bug here: it
+ # returns "0000056A" on longer (always wrong) previous passwords.
+ self.assertTrue('00000056' in msg)
+
+ # Sets the initial user password with a "special" password change
+ # I think that this internally is a password set operation and it can
+ # only be performed by someone which has password set privileges on the
+ # account (at least in s4 we do handle it like that).
self.ldb.modify_ldif("""
dn: cn=testuser,cn=users,""" + self.base_dn + """
changetype: modify
-replace: userPassword
+delete: userPassword
+add: userPassword
+userPassword: thatsAcomplPASS1
+""")
+
+ # But in the other way around this special syntax doesn't work
+ try:
+ self.ldb.modify_ldif("""
+dn: cn=testuser,cn=users,""" + self.base_dn + """
+changetype: modify
+delete: userPassword
userPassword: thatsAcomplPASS1
+add: userPassword
""")
+ self.fail()
+ except LdbError, (num, _):
+ self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+
+ # Enables the user account
self.ldb.enable_account("(sAMAccountName=testuser)")
# Open a second LDB connection with the user credentials. Use the
@@ -186,8 +209,9 @@ add: unicodePwd
unicodePwd:: """ +
base64.b64encode("\"thatsAcomplPASS4\"".encode('utf-16-le')) + """
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('00000056' in msg)
# A change to the same password again will not work (password history)
try:
@@ -200,8 +224,9 @@ add: unicodePwd
unicodePwd:: """ +
base64.b64encode("\"thatsAcomplPASS2\"".encode('utf-16-le')) + """
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('0000052D' in msg)
def test_dBCSPwd_hash_set(self):
print "Performs a password hash set operation on 'dBCSPwd' which
should be prevented"
@@ -270,8 +295,9 @@ add: userPassword
userPassword: thatsAcomplPASS4
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('00000056' in msg)
# A change to the same password again will not work (password history)
try:
@@ -284,8 +310,9 @@ add: userPassword
userPassword: thatsAcomplPASS2
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('0000052D' in msg)
def test_clearTextPassword_clear_set(self):
print "Performs a password cleartext set operation on
'clearTextPassword'"
@@ -333,10 +360,11 @@ add: clearTextPassword
clearTextPassword:: """ +
base64.b64encode("thatsAcomplPASS4".encode('utf-16-le')) + """
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
# "NO_SUCH_ATTRIBUTE" is returned by Windows -> ignore it
if num != ERR_NO_SUCH_ATTRIBUTE:
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('00000056' in msg)
# A change to the same password again will not work (password history)
try:
@@ -349,10 +377,11 @@ add: clearTextPassword
clearTextPassword:: """ +
base64.b64encode("thatsAcomplPASS2".encode('utf-16-le')) + """
""")
self.fail()
- except LdbError, (num, _):
+ except LdbError, (num, msg):
# "NO_SUCH_ATTRIBUTE" is returned by Windows -> ignore it
if num != ERR_NO_SUCH_ATTRIBUTE:
self.assertEquals(num, ERR_CONSTRAINT_VIOLATION)
+ self.assertTrue('0000052D' in msg)
def test_failures(self):
print "Performs some failure testing"
diff --git a/source4/ldap_server/ldap_backend.c
b/source4/ldap_server/ldap_backend.c
index d5b1a39..e0f9ee1 100644
--- a/source4/ldap_server/ldap_backend.c
+++ b/source4/ldap_server/ldap_backend.c
@@ -45,6 +45,19 @@ static int map_ldb_error(TALLOC_CTX *mem_ctx, int ldb_err,
{
WERROR err;
+ /* Certain LDB modules need to return very special WERROR codes. Proof
+ * for them here and if they exist skip the rest of the mapping. */
+ if (add_err_string != NULL) {
+ char *endptr;
+ strtol(add_err_string, &endptr, 16);
+ if (endptr != add_err_string) {
+ *errstring = add_err_string;
+ return ldb_err;
+ }
+ }
+
+ /* Otherwise we calculate here a generic, but appropriate WERROR. */
+
switch (ldb_err) {
case LDB_SUCCESS:
err = WERR_OK;
@@ -165,7 +178,7 @@ static int map_ldb_error(TALLOC_CTX *mem_ctx, int ldb_err,
break;
}
- *errstring = talloc_asprintf(mem_ctx, "%08x: %s", W_ERROR_V(err),
+ *errstring = talloc_asprintf(mem_ctx, "%08X: %s", W_ERROR_V(err),
ldb_strerror(ldb_err));
if (add_err_string != NULL) {
*errstring = talloc_asprintf(mem_ctx, "%s - %s", *errstring,
--
Samba Shared Repository
