Re: ext/standard/crypt_blowfish.c

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, On Mon, Jun 8, 2009 at 5:11 PM, Solar Designer<solar@openwall.com> wrote: > On Sun, Jun 07, 2009 at 11:06:45PM +0200, Pierre Joye wrote: >> 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