[RCD] Encryption problem in the Password plugin

Rimas Kudelis rq at akl.lt
Tue May 3 22:42:21 CEST 2011


There is a problem with Password plugin's password encryption in Debian 
Squeeze. Because of a known bug in Debian's PHP (CRYPT_SALT_LENGTH is 
not set*), the password is currently being encrypted with an empty salt 
in SQL driver (but I think this might apply to the LDAP driver too, 
because it too uses crypt()).

The simple fix is to replace this block of code in drivers/sql.php 

         $salt = '';
         if (CRYPT_MD5) {
             $len = rand(3, CRYPT_SALT_LENGTH);
         } else if (CRYPT_STD_DES) {
             $len = 2;
         } else {
             return PASSWORD_CRYPT_ERROR;
         for ($i = 0; $i < $len ; $i++) {
             $salt .= chr(rand(ord('.'), ord('z')));
         $sql = str_replace('%c',  $db->quote(crypt($passwd, CRYPT_MD5 ? 
'$1$'.$salt.'$' : $salt)), $sql);

with a single line:

         $sql = str_replace('%c',  $db->quote(crypt($passwd)), $sql);

When called without the second parameter, crypt() will automatically 
generate the salt itself. Which makes me wonder: why does the plugin try 
to generate this random salt instead of simply relying on PHP to do 
that? As far as I know, crypt($text, $salt) is useful when you actually 
want to check whether the password is correct, but there's absolutely no 
need to use your own random salt when setting user's password – PHP will 
take care.

May I suggest to apply this change in SVN? I really don't see why the 
plugin should use 12 lines of code to achieve what can be achieved in 
one line.

* http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=603012


List info: http://lists.roundcube.net/dev/

More information about the Dev mailing list