#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

  • Christian Boltz

    Christian Boltz - 2007-10-07

    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.)

     
  • Christian Boltz

    Christian Boltz - 2007-10-08
    • 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?

     
  • Christian Boltz

    Christian Boltz - 2007-12-30
    • priority: 5 --> 2
     
  • Nobody/Anonymous

    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.

     
  • Nobody/Anonymous

    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

     
  • Nobody/Anonymous

    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.

     
  • Christian Boltz

    Christian Boltz - 2013-12-01
    • status: open --> closed-fixed
     
  • Christian Boltz

    Christian Boltz - 2013-12-01

    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 ;-)

     

Log in to post a comment.

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

Sign up for the SourceForge newsletter:





No, thanks