Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:44201
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 72.14.220.156 as permitted sender)
DomainKey-Status: bad
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :cc:content-type:content-transfer-encoding;
        b=mqIJ8CW5ZSvGPPQk8OzE3RDZPpCYADIfVZqTIa5F6E4FzgHr59t092O5zuzZfJ+XRv
         6GCJ0giNThzrHxCPvRaqEeJ4ADT2DFu5mI7oujVSnRsZip2vrivPo3mNQ8svAUExv7Fi
         wU9/AktmWrkcrbgiu5Tkq+M41iZUsn+drUggo=
MIME-Version: 1.0
In-Reply-To: <20090607160019.GA30041@openwall.com>
References: <20090607160019.GA30041@openwall.com>
Date: Sun, 7 Jun 2009 23:06:45 +0200
Message-ID: <fe05d1540906071406m21556f9o767d46cb76296e8d@mail.gmail.com>
To: Solar Designer <solar@openwall.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: ext/standard/crypt_blowfish.c
From: pierre.php@gmail.com (Pierre Joye)

hi,

On Sun, Jun 7, 2009 at 6:00 PM, Solar Designer<solar@openwall.com> wrote:
> Hi,
>
> First of all, thank you for getting this functionality into PHP proper.

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 :).

> It appears that the file was very slightly out of date. =A0crypt_blowfish
> 1.0.2 additionally made this change:
>
> -#elif defined(__alpha__) || defined(__hppa__)
> +#elif defined(__x86_64__) || defined(__alpha__) || defined(__hppa__)
>
> which improved performance on x86_64. =A0I recommend that you apply the
> change to the copy in PHP as well.
>
> The function php_crypt_gensalt_blowfish_rn() appears to be unused.
> If so, I suggest #if 0'ing it for now.

Thanks for the review and the notice, I'll update the code tomorrow
(5.3+ have it).

> 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.

> --- ../crypt_blowfish-1.0.2/crypt_blowfish.c =A0 =A02006-05-22 23:52:41 +=
0000
> +++ ext/standard/crypt_blowfish.c =A0 =A0 =A0 2008-08-14 01:13:18 +0000
> [...]
> @@ -380,6 +387,7 @@
> =A0#define BF_safe_atoi64(dst, src) \
> =A0{ \
> =A0 =A0 =A0 =A0tmp =3D (unsigned char)(src); \
> + =A0 =A0 =A0 if (tmp =3D=3D '$') break; \
> =A0 =A0 =A0 =A0if ((unsigned int)(tmp -=3D 0x20) >=3D 0x60) return -1; \
> =A0 =A0 =A0 =A0tmp =3D BF_atoi64[tmp]; \
> =A0 =A0 =A0 =A0if (tmp > 63) return -1; \
> @@ -407,6 +415,9 @@ static int BF_decode(BF_word *dst, __CON
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*dptr++ =3D ((c3 & 0x03) << 6) | c4;
> =A0 =A0 =A0 =A0} while (dptr < end);
>
> + =A0 =A0 =A0 while (dptr < end)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *dptr++ =3D 0;
> +
> =A0 =A0 =A0 =A0return 0;
> =A0}
>
> My understanding is that this "adds support" for salt strings shorter
> than those bcrypt (the password hashing method we're talking about)
> normally requires, but only as long as they're terminated with a dollar
> sign. =A0Why is this needed, and is it? =A0Do we really want to encourage
> sloppy programming? =A0I don't think this may support any extra existing
> bcrypt-like hashes, which might have been generated by sloppy
> implementations, because the encodings for newly computed hashes (during
> authentication) would be full-length anyway. =A0Am I missing something?
>
> Thanks again,
>
> Alexander
>
> P.S. For those who don't know yet, my "upstream" version of the code is
> available here:
>
> http://www.openwall.com/crypt/
> http://cvsweb.openwall.com/crypt

Cheers,
--=20
Pierre

http://blog.thepimp.net | http://www.libgd.org