#49524 password policy: minimum token length fails when the token length is equal to attribute length.
Closed: wontfix 5 years ago Opened 5 years ago by mreynolds.

Ticket was cloned from Red Hat Bugzilla (product Red Hat Enterprise Linux 7): Bug 1517788

Description of problem:

the function check_trivial_words is failing to recongnize a token that is
exactly same size as the token length.

Example:

dn: uid=Dord,ou=People,dc=parente,dc=local
sn: David
objectClass: top
objectClass: account
objectClass: posixaccount
objectClass: inetOrgPerson
objectClass: organizationalPerson
objectClass: person
uid: Dord
cn: David
loginShell: /bin/bash
homeDirectory: /home/dorda
uidNumber: 501003
gidNumber: 510003

passwordMinTokenLength: 4

ldapmodify -D "uid=Dord,ou=People,dc=parente,dc=local" -w secret12
dn: uid=Dord,ou=People,dc=parente,dc=local
changetype: modify
replace: userpassword
userPassword: dord

modifying entry "uid=Dord,ou=People,dc=parente,dc=local"


Actual results

the password including a token of 4 length is accepted (it's the exact length
of the attribute).

Version-Release number of selected component (if applicable):
389-ds-base-1.3.6.1-21


How reproducible: always


Actual results:

password is not refused.


Expected results:

password refused when the token length is same length than the attribute.


Additional info:


After some debugging, the function:

check_trivial_words calls

ldap_utf8prevn(sp, ep, toklen);

This function returns NULL when the token length is same length than the
attribute value in "sp".

IMHO, this should be the fix:

char
ldap_utf8prevn(char s, char from, int n)
{
char prev = from;
if (!s || !from || (s > from)) {
return NULL;
}
for (; n > 0; --n) {
prev = ldap_utf8prev(prev);
if ((prev <= s) && (n > 0)) { <============= should be (prev < s)
return NULL;
}
}
return prev;
}

For instance, in the previous example,

ldap_utf8prevn("dord", char *from,4)

will loop to find the postfix d, rd, ord, and will return NULL for dord.

So check_trivial_words will fail to refuse the password.

Metadata Update from @mreynolds:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1517788

5 years ago

Metadata Update from @mreynolds:
- Issue assigned to mreynolds

5 years ago

Metadata Update from @mreynolds:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None
- Issue set to the milestone: 1.3.6.0 (was: 0.0 NEEDS_TRIAGE)

5 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review (was: None)

5 years ago

A couple of small issue with the test:

It fails on Python 3 because 'passwd' is not byte type.

topo.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE, 'userpassword', passwd)])

I think you can use the user returned by the fixture and user.replace method.

Also, please, change the first line in the docstring and the :setup: step (to Standalone instance)

68 +    """Specify a test case purpose or name here
69 +
70 +    :id: dae9d916-2a03-4707-b454-9e901d295b13
71 +    :setup: Fill in set up configuration here

To follow on from @spichugi

If you make:

return users.create(....)

In def pwd_setup, then in your test case you can do:

user = pwd_setup

Then do:

user.replace('userPassword', "value")

Instead.

From now on direct calls to modify_s/add_s are generally going to break python 3, so we really really need to stop using them,a nd use the lib389 wrappers that actually take care of bytesafety.

Additionally, we shouldn't be calling Entry directly, because I want to eventually get rid of this type in favour of just using DSLdapObject.

Hope that helps,

For the C code:

+        if ((prev < s) && (n > 0)) {

You need to check n> 0 first, else you may over-read prev/s.

Thanks!

From now on direct calls to modify_s/add_s are generally going to break python 3, so we really really need to stop using them,a nd use the lib389 wrappers that actually take care of bytesafety.

Wow, okay I am really behind on the changes you are guys are doing to lib389. Do we have updated docs on how to do add/modifies/deletes/etc? Where can I find the new "wrappers"? There is nothing about this on Simon's Lib389 doc page.

The attached patch fixes the test case and the C code. On the C code, it has always had that order without issue, but I still changed it :-)

New patch:

0001-Ticket-49524-Password-policy-minimum-token-length-fa.patch

From now on direct calls to modify_s/add_s are generally going to break python 3, so we really really need to stop using them,a nd use the lib389 wrappers that actually take care of bytesafety.

Wow, okay I am really behind on the changes you are guys are doing to lib389. Do we have updated docs on how to do add/modifies/deletes/etc? Where can I find the new "wrappers"? There is nothing about this on Simon's Lib389 doc page.

But you've done everything right... You've used the wrappers. :) This is the wrappers - https://fedorapeople.org/~spichugi/html/user.html

You have my ack for Python code.
Though, please, change the first docstring line to the test related one

67 +    """Specify a test case purpose or name here

From now on direct calls to modify_s/add_s are generally going to break python 3, so we really really need to stop using them,a nd use the lib389 wrappers that actually take care of bytesafety.
Wow, okay I am really behind on the changes you are guys are doing to lib389. Do we have updated docs on how to do add/modifies/deletes/etc? Where can I find the new "wrappers"? There is nothing about this on Simon's Lib389 doc page.

But you've done everything right... You've used the wrappers. :) This is the wrappers - https://fedorapeople.org/~spichugi/html/user.html

Oh so we are doing wrappers for each type of object. One set of wrappers for users, one for plugins, config, schema, indexes, etc? I know this was somewhat implemented with the mappedDSobject, but I didn't realize how far its come. Nice! But that's a lot of testcases to rewrite :scream:

You have my ack for Python code.
Though, please, change the first docstring line to the test related one
67 + """Specify a test case purpose or name here

Will do, thanks!

d79102f..790be09 master -> master

b8f1b19..288e731 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

572f978..689b388 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

5 years ago

Great, much happier with the new patch :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2583

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

3 years ago

Login to comment on this ticket.

Metadata