ext/standard/crypt_blowfish.c View as Markdown

hi Alexander,

Thanks for the feedbacks! these questions are the same I had when I
was making crypt portable (having all algos available in any
situation).

Sadly I do not have the time yet to fix the behavior (remove the
workaround/BC hack or the on error behavior). If you like to (or have
the time to :), you can five it a try, that will be very helpful. Or I
can work on that next week, at the soonest.

Cheers,

Thank for your work, I'm the one who merged your implementation to PHP
(we had a discussion about it btw, per email if you remember :).

Yes, I recalled that, and this is why I CC'ed you on my posting.

Thanks for the review and the notice, I'll update the code tomorrow
(5.3+ have it).

I see that the x86_64 check went in.  Thanks!

Finally, I'd like to ask whoever made the following change about the
rationale behind it:

Backward compatibility was the reason. I like to drop this thing in
php6 but it was not possible to do it in a minor release (I discussed
it with Stefan Esser last year). I'm not sure yet how but certainly by
providing another function or changing the API to allow one to disable
the old behavior.

Here's what I found out:

1. The documentation at http://www.php.net/manual/en/function.crypt.php
   remains wrong.  It says:

"CRYPT_BLOWFISH - Blowfish encryption with a sixteen character salt
starting with $2$ or $2a$"

Well, the salt string should be 22 characters (after the "$2a$nn$"
prefix, or 29 characters including this prefix).  The salt is 16 bytes,
not 16 characters.

2. "Example #3 Using crypt() with different encryption types" at
   http://www.php.net/manual/en/function.crypt.php is also wrong, in a
   different way.  It has:

   echo 'Blowfish:     ' . crypt('rasmuslerdorf', '$2a$07$rasmuslerd...........$') . "\n";

Apparently, this works with the workaround in the Suhosin patch, and
thus with current PHP 5.3.0 development code, but it fails on systems
that have bcrypt hashes supported natively (in libc or libcrypt) and do
not have the workaround in there.  This comment is just right:

http://www.php.net/manual/en/function.crypt.php#71748

3. Yes, I now see that the workaround came from a revision of my code in
   the Suhosin patch.  However, it is not clear to me what exactly this
   workaround was for, and backwards compatibility with what it provides
   now.  Is it just compatibility with the above incorrect example in the
   documentation?  If so, I see no problem with dropping it now, because
   the example did not work universally anyway (it failed on many or
   probably even on most systems that had support for bcrypt hashes at all).

I hope these observations are of some help.

A somewhat related concern: what does PHP's crypt() return on error?

In my crypt_blowfish package, I had a higher-level wrapper function that
would guarantee return of a string not matching the input "salt" string,
thus guaranteeing that authentication will fail.  I don't think PHP
currently has similar code; perhaps it should.  Also, documentation
should be revised to document behavior of crypt() on error, for both
historical and current/future versions of PHP.  Because in many cases
this depends on the underlying system libraries, I suggest that advice
be given to treat any return strings shorter than 13 characters as
indicating an error.

In fact, PHP itself could start to implement the safe behavior by
checking whether the would-be crypt() return string is shorter than 13
characters (regardless of where it came from).  If so, return "*0" if
the input salt string does not start with "*0", or return "*1" if the
input salt string starts with "*0".

Here's an excerpt from wrapper.c in my crypt_blowfish package:

       output[0] = '*';
       output[1] = '0';
       output[2] = '\0';

       if (setting[0] == '*' && setting[1] == '0')
               output[1] = '1';

Thanks,

Alexander

--
Pierre

http://blog.thepimp.net | http://www.libgd.org