[ postfixadmin-Bugs-1694669 ] Improper Use of crypt()

[SourceForge.net <no...@so...>]

Bugs item #1694669, was opened at 2007-04-04 17:14
Message generated for change (Comment added) made by nobody
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=937964&aid=1694669&group_id=191583

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core
Group: SVN (please specify revision!)
Status: Open
Resolution: None
Priority: 2
Private: No
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Nobody/Anonymous (nobody)
Summary: Improper Use of crypt()

Initial Comment:
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.

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2008-06-15 02:58

Message:
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.

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2008-01-31 03:05

Message:
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

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2008-01-27 09:56

Message:
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.

----------------------------------------------------------------------

Comment By: GingerDog (gingerdog)
Date: 2007-10-09 10:05

Message:
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?

----------------------------------------------------------------------

Comment By: Christian Boltz (christian_boltz)
Date: 2007-10-07 13:03

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

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=937964&aid=1694669&group_id=191583