#146 crypt() is not used according to specs

v_2.3
closed-fixed
nobody
Core (82)
5
2009-07-27
2009-06-30
No

In functions.inc.php, pacrypt() uses the php crypt() function the wrong way when processing MD5 hashes. For the salt, all 12 characters of the input password hash salt must be used, including the magic sequence and the closing dollar sign, for example: "$1$abcdefgh$". Otherwise the crypt() function generates the shorter DES password hashes and password comparison always fails. This problem has become apparent for me when I was trying to migrate a user db for postfixadmin, where user password hashes are stored as DES for some users, and as MD5 for others. pacrypt() would be able to handle both, but currently it doesn't because of the forementioned mistake. Simply using $salt = "\$1\$${split_salt[2]}\$"; solves the problem.

Discussion

  • Anonymous - 2009-06-30

    And one thing i forgot, related to this problem: the ereg expression is written as ereg ("\$1\$", $pw_db), and that is wrong. Instead it should be written as ereg ("\\$1\\$", $pw_db)), with double backslashes. Otherwise the if () will never evaluate true when an MD5 hash is passed.

     
  • GingerDog

    GingerDog - 2009-07-19

    err http://php.net/crypt doesn't mention anything about a trailing $ for crypt's salt field.

    David.

     
  • Anonymous - 2009-07-19

    Indeed, but it does work this way: if the salt length is less than 12, the short crypt sequence is used. If it is 12, then the salted MD5 crypt variant is used. If you pass the 12 characters of the salt that includes all separator dollar signs, it creates the proper MD5 hash. Otherwise it falls back to the shorter variant.

    I've applied the mentioned fixes in my installation and they work as I have already described; encodes both kind of hashes properly and all my users can log in. Maybe I should ask for a clarification of the PHP docs from the PHP documentation maintainers? Whatever the case, it is not working properly in PostfixAdmin right now.

     
  • Anonymous - 2009-07-19

    Additionally, please have a look at the official examples provided on the documentation page you have linked (and their included comments). Althought they're not explicitly formed documentation paragraphs, they seem to back what I have already said on the subject. If you see my reasoning acceptable, I'm happy to provide a patch you might want to apply.

     
  • GingerDog

    GingerDog - 2009-07-19

    a patch against svn-current would be great -then there's no chance for me to fsck stuff up ;-)

    David

     
  • Anonymous - 2009-07-24

    Patch attached.

     
  • GingerDog

    GingerDog - 2009-07-27

    patch applied; see 2.3rc7 . thanks.

     
  • GingerDog

    GingerDog - 2009-07-27

    Thanks for the bug report; we believe this has been fixed in subversion.

     
  • GingerDog

    GingerDog - 2009-07-27
    • status: open --> closed-fixed
     
  • Christian Boltz

    Christian Boltz - 2009-07-28

    Your patch re-introduced an ereg() call which is deprecated in PHP 5.3. I replaced it with preg_match().

    Please test with SVN r696 to make sure I didn't accidently break it ;-)

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks