Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#2 Improper Use of crypt()

closed-fixed
nobody
Core (82)
2
2013-12-01
2007-04-05
Anonymous
No

Inside the pacrypt() function in functions.inc.php, crypt() is used for the 'system' encryption type. Salt is first calculated, with the below code:

if (ereg ("\$1\$", $pw_db))
{
$split_salt = preg_split ('/\$/', $pw_db);
$salt = $split_salt[2];
}
else
{
$salt = substr ($pw_db, 0, 2);
}

... however, that is improper according to the php.net documentation (http://www.php.net/crypt) for the crypt() call:

...
You should pass the entire results of crypt() as the salt for comparing a password, to avoid problems when different hashing algorithms are used. (As it says above, standard DES-based password hashing uses a 2-character salt, but MD5-based hashing uses 12.)
...

Simply modifying the code to read:
if ($pw_db)
{
$password = crypt ($pw, $pw_db);
}
else
{
$password = crypt ($pw);
}

... fixed the problem in my case.

Discussion

  • Logged In: YES
    user_id=593261
    Originator: NO

    Your arguments are valid, but the question is:

    Will this break existing passwords?

    (If yes, it will be problematic to do this change.)

     
    • milestone: 700207 --> SVN_(please_specify_revision!)
     
  • GingerDog
    GingerDog
    2007-10-09

    Logged In: YES
    user_id=1761957
    Originator: NO

    I'd be tempted to think not - after all people must (!?) be using crypt'ed passwords with other 3rd party applications (e.g. imap/pop3 clients).... doesn't this imply we can fix this without any side effects?

     
    • priority: 5 --> 2
     
  • Logged In: NO

    i changed it in following way:
    ....
    if (ereg ('\$1\$', $pw_db))
    {
    $split_salt = preg_split ('/\$/', $pw_db);
    $salt = "$1$".$split_salt[2];
    }
    ....

    i think that is without any side effects.

     
  • Logged In: NO

    I can certify this does not break anything on GNU/Linux or FreeBSD systems; passing the entire hash as salt is the proper way. Splitting, like you currently do in pacrypt(), does not work -- If you create a new user, he cannot log in. Copying /etc/passwd entries breaks also.

    --
    Morten Hustveit

     
  • Logged In: NO

    Same here. I had to patch this manually (for every PFA version so far) to make it work under FreeBSD with "long" (MD5 instead of DES) crypt passwords.
    Currently in 2.2.0 version this code is used:

    if ($CONF['encrypt'] == 'system') {
    if (ereg ("\$1\$", $pw_db)) {
    $split_salt = preg_split ('/\$/', $pw_db);
    $salt = $split_salt[2];
    }
    else {
    if (strlen($pw_db) == 0) {
    $salt = substr (md5 (mt_rand ()), 0, 2);
    }
    else {
    $salt = substr ($pw_db, 0, 2);
    }
    }
    $password = crypt ($pw, $salt);
    }

    which creates "short" (DES) passwords when creating new users. Using the code above (proposed in the description of this bug) elegantly fixes all the issues. Will this fix be included in the following PostfixAdmin release? I think it's about time.

     
    • status: open --> closed-fixed
     
  • Finally fixed in SVN trunk r1595 (which means PostfixAdmin 3.0 will have the new code).

    Thanks for your patience ;-)

    Personally, I don't use the 'system' hashing method, so I'm more than happy if someone who uses it can test if the latest code from SVN trunk really works ;-)