Full_Name: Ryan Tandy Version: master/33e12f4 RE24/b000d95 OS: Debian unstable URL: Submission from: (NULL) (24.68.121.206) Hi, The apr1 passwd plugin calls do_phk_hash with the arguments in the wrong order, so the digest updates are done in a different order than md5crypt does. The following patch fixes that, restoring compatibility with existing htpasswd files. However, existing {APR1} hashes that were generated while the bug existed are going to be broken... I'm not sure what to do about that. :/ thanks, Ryan From f9ad46e3c8264ffa1420aa3b24cfc69cae7bed65 Mon Sep 17 00:00:00 2001 From: Ryan Tandy <ryan@nardis.ca> Date: Sun, 1 Jun 2014 22:41:23 -0700 Subject: [PATCH] contrib passwd/apr1 fix do_phk_hash arguments --- contrib/slapd-modules/passwd/apr1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/slapd-modules/passwd/apr1.c b/contrib/slapd-modules/passwd/apr1.c index ce7b8c7..463d8d1 100644 --- a/contrib/slapd-modules/passwd/apr1.c +++ b/contrib/slapd-modules/passwd/apr1.c @@ -143,7 +143,7 @@ static int chk_phk( salt.bv_val = (char *) &orig_pass[sizeof(digest)]; salt.bv_len = rc - sizeof(digest); - do_phk_hash(cred, magic, &salt, digest); + do_phk_hash(cred, &salt, magic, digest); if (text) *text = NULL; @@ -197,7 +197,7 @@ static int hash_phk( for (n = 0; n < salt.bv_len; n++) salt.bv_val[n] = apr64[salt.bv_val[n] % (sizeof(apr64) - 1)]; - do_phk_hash(passwd, magic, &salt, digest_buf); + do_phk_hash(passwd, &salt, magic, digest_buf); if (text) *text = NULL; -- 2.0.0
ryan@nardis.ca wrote: > Full_Name: Ryan Tandy > Version: master/33e12f4 RE24/b000d95 > OS: Debian unstable > URL: > Submission from: (NULL) (24.68.121.206) > > > Hi, > > The apr1 passwd plugin calls do_phk_hash with the arguments in the wrong order, > so the digest updates are done in a different order than md5crypt does. The > following patch fixes that, restoring compatibility with existing htpasswd > files. > > However, existing {APR1} hashes that were generated while the bug existed are > going to be broken... I'm not sure what to do about that. :/ According to ITS#6826, where this code came from originally, the generated {APR1} hashes are currently compatible with htpasswd. As such, your patch would break htpasswd compatibility. As such it seems like a bad idea to commit your change. > > thanks, > Ryan > > > >>From f9ad46e3c8264ffa1420aa3b24cfc69cae7bed65 Mon Sep 17 00:00:00 2001 > From: Ryan Tandy <ryan@nardis.ca> > Date: Sun, 1 Jun 2014 22:41:23 -0700 > Subject: [PATCH] contrib passwd/apr1 fix do_phk_hash arguments > > --- > contrib/slapd-modules/passwd/apr1.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/contrib/slapd-modules/passwd/apr1.c > b/contrib/slapd-modules/passwd/apr1.c > index ce7b8c7..463d8d1 100644 > --- a/contrib/slapd-modules/passwd/apr1.c > +++ b/contrib/slapd-modules/passwd/apr1.c > @@ -143,7 +143,7 @@ static int chk_phk( > salt.bv_val = (char *) &orig_pass[sizeof(digest)]; > salt.bv_len = rc - sizeof(digest); > > - do_phk_hash(cred, magic, &salt, digest); > + do_phk_hash(cred, &salt, magic, digest); > > if (text) > *text = NULL; > @@ -197,7 +197,7 @@ static int hash_phk( > for (n = 0; n < salt.bv_len; n++) > salt.bv_val[n] = apr64[salt.bv_val[n] % (sizeof(apr64) - 1)]; > > - do_phk_hash(passwd, magic, &salt, digest_buf); > + do_phk_hash(passwd, &salt, magic, digest_buf); > > if (text) > *text = NULL; > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu wrote: > ryan@nardis.ca wrote: >> Full_Name: Ryan Tandy >> Version: master/33e12f4 RE24/b000d95 >> OS: Debian unstable >> URL: >> Submission from: (NULL) (24.68.121.206) >> >> >> Hi, >> >> The apr1 passwd plugin calls do_phk_hash with the arguments in the wrong order, >> so the digest updates are done in a different order than md5crypt does. The >> following patch fixes that, restoring compatibility with existing htpasswd >> files. >> >> However, existing {APR1} hashes that were generated while the bug existed are >> going to be broken... I'm not sure what to do about that. :/ > > According to ITS#6826, where this code came from originally, the generated > {APR1} hashes are currently compatible with htpasswd. As such, your patch > would break htpasswd compatibility. As such it seems like a bad idea to commit > your change. I've also confirmed, using perl Crypt::PasswdMD5, that the hashes generated by the current code are compatible. In particular, a password generated by this script: ### use strict; use warnings; use Crypt::PasswdMD5; my($password) = 'seekrit'; my($salt) = 'pepperoni'; my($unix_crypted) = unix_md5_crypt($password, $salt); my($apache_crypted) = apache_md5_crypt($password, $salt); print "$unix_crypted\n"; print "$apache_crypted\n"; ### can be converted to OpenLDAP {BSDMD5} and {APR1} format, respectively, and matches the output generated by the current module using the same salt and plaintext. Rejecting this patch, closing this ITS. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Thanks for following up. On Thu, Jul 17, 2014 at 2:00 PM, Howard Chu <hyc@symas.com> wrote: > my($password) = 'seekrit'; > my($salt) = 'pepperoni'; > my($apache_crypted) = apache_md5_crypt($password, $salt); $apr1$pepperon$VBD3GaYfoFXuIcZrTw/Y// > can be converted to OpenLDAP {BSDMD5} and {APR1} format, respectively {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u correct? > and matches the output generated by the current module using the same salt and plaintext. dn: uid=test,dc=example,dc=com objectClass: account objectClass: simpleSecurityObject userPassword: {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u
Shortcut key fail, sorry. On Thu, Jul 17, 2014 at 3:18 PM, Ryan Tandy <ryan@nardis.ca> wrote: > dn: uid=test,dc=example,dc=com > objectClass: account > objectClass: simpleSecurityObject > userPassword: {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u What I meant to say was: I can't bind to that entry with the password "seekrit".
Ryan Tandy wrote: > Shortcut key fail, sorry. > > On Thu, Jul 17, 2014 at 3:18 PM, Ryan Tandy <ryan@nardis.ca> wrote: >> dn: uid=test,dc=example,dc=com >> objectClass: account >> objectClass: simpleSecurityObject >> userPassword: {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u > > What I meant to say was: I can't bind to that entry with the password "seekrit". > Works for me: dn: cn=joe,ou=people,dc=example,dc=com objectclass: person cn: joe sn: bob userpassword: {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u violino:~/OD/hobj/tests> ../servers/slapd/slapd -F /tmp/testr/slapd.d -h ldap://:9011 -s0 -dnone & [1] 325 violino:~/OD/hobj/tests> 53c86f8c @(#) $OpenLDAP: slapd 2.X (Jul 13 2014 19:25:04) $ hyc@violino:/home/hyc/OD/hobj/servers/slapd 53c86f8c slapd starting violino:~/OD/hobj/tests> ../clients/tools/ldapwhoami -x -H ldap://:9011 -D cn=joe,ou=people,dc=example,dc=com -w seekrit dn:cn=joe,ou=People,dc=example,dc=com Also works with {BSDMD5}TvI7yUE++iEAGjN3LfD3l3BlcHBlcm9u which was converted from $1$pepperon$hcjHk5Wwr1kCLeFmyAHEr/ -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Ryan Tandy wrote: > Shortcut key fail, sorry. > > On Thu, Jul 17, 2014 at 3:18 PM, Ryan Tandy <ryan@nardis.ca> wrote: >> dn: uid=test,dc=example,dc=com >> objectClass: account >> objectClass: simpleSecurityObject >> userPassword: {APR1}FK7q3pAf80k0Wh9BYZJ0FHBlcHBlcm9u > > What I meant to say was: I can't bind to that entry with the password "seekrit". > My mistake; I had been testing with the patch applied. Thanks for the report, fixed in git master. Since it appears no one has ever used this module yet, we can ignore any backward-compatibility issues. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
changed notes changed state Open to Test moved from Incoming to Contrib
changed notes changed state Test to Release
fixed in master fixed in RE25 fixed in RE24
changed notes changed state Release to Closed