Everything

php

[VOTE] Change crypt() behavior w/o salt

php.internals

Yasuo Ohgaki

12 years ago

Hi all,

It's been a whole from the discussion.
I setup vote for RFC: Change crypt() behavior w/o salt.

https://wiki.php.net/rfc/crypt_function_salt

If I missed something, please let me know.
Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Pierre Joye

12 years ago

On Sep 23, 2013 6:42 PM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:

>
> Hi all,
>
> It's been a whole from the discussion.
> I setup vote for RFC: Change crypt() behavior w/o salt.
>
> https://wiki.php.net/rfc/crypt_function_salt
>
> If I missed something, please let me know.

Thank you for pushing this RFC so far!

One point to keep in mind: BC breaks disqualify an option for any 5.x
releases. Warning and the likes are not BC breaks per se.

Cheers,
Pierre

William Bartlett

12 years ago

I think BC (like requiring salt) are unnecessary given that password_*
functions already exist with salt enabled by default.

But an E_NOTICE isn't a bad idea.

--
William Bartlett
College of Engineering | Cornell University '14
240-432-5189


On Mon, Sep 23, 2013 at 10:47 PM, Pierre Joye <pierre.php@gmail.com> wrote:

Alexey Zakhlestin

12 years ago

On 24.09.2013, at 5:41, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>
> It's been a whole from the discussion.
> I setup vote for RFC: Change crypt() behavior w/o salt.
>
> https://wiki.php.net/rfc/crypt_function_salt
>
> If I missed something, please let me know.

strangely, this RFC is not shown in "In voting phase" section here: https://wiki.php.net/rfc
is that done manually?

--
Alexey Zakhlestin
CTO at Grids.by/you
https://github.com/indeyets
PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc

Yasuo Ohgaki

12 years ago

Hi Alexey,

On Tue, Sep 24, 2013 at 11:22 PM, Alexey Zakhlestin <indeyets@gmail.com>wrote:

> strangely, this RFC is not shown in "In voting phase" section here:
> https://wiki.php.net/rfc
> is that done manually?
>

I should have done this.
Thank you for point it out.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

12 years ago

Hi all,

Vote period is ended and the result was

0   Generate strong salt by default
12 Generate E_NOTICE error
2   Keep current behavior (Use weak hash)

I'll prepare patch for it later.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net


On Wed, Sep 25, 2013 at 1:17 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

Yasuo Ohgaki

12 years ago

Hi all,

Any comments patch for this RFC?
Better E_NOTICE message is welcome.

diff --git a/UPGRADING b/UPGRADING
index b19c3dd..8f5033a 100755
--- a/UPGRADING
+++ b/UPGRADING
@@ -64,6 +64,10 @@ PHP X.Y UPGRADE NOTES
4. Changed Functions
========================================

+- Standard:
+  crypt() without salt parameter raises E_NOTICE.
+  See https://wiki.php.net/rfc/crypt_function_salt
+
- cURL:
CURLOPT_SAFE_UPLOAD is now turned on by default and uploads with @file
do not work unless it is explicitly set to false.
diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c
index 113a5bd..eea6f78 100644
--- a/ext/standard/crypt.c
+++ b/ext/standard/crypt.c
@@ -272,6 +272,8 @@ PHP_FUNCTION(crypt)

if (salt_in) {
memcpy(salt, salt_in, MIN(PHP_MAX_SALT_LEN, salt_in_len));
+ } else {
+ php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Weak hashed password is
generated without salt parameter");
}

/* The automatic salt generation covers standard DES, md5-crypt and
Blowfish (simple) */


Output:
[yohgaki@dev PHP-master]$ ./php-bin -d error_reporting=-1 -r
"crypt('asdfa');"
Notice: crypt(): Weak hashed password is generated without salt parameter
in Command line code on line 1

--
Yasuo Ohgaki
yohgaki@ohgaki.net


On Mon, Oct 7, 2013 at 5:51 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

Joe Watkins

12 years ago

On 10/22/2013 07:10 AM, Yasuo Ohgaki wrote:

> Hi all,
>
> Any comments patch for this RFC?
> Better E_NOTICE message is welcome.
>
> diff --git a/UPGRADING b/UPGRADING
> index b19c3dd..8f5033a 100755
> --- a/UPGRADING
> +++ b/UPGRADING
> @@ -64,6 +64,10 @@ PHP X.Y UPGRADE NOTES
>   4. Changed Functions
>   ========================================
>
> +- Standard:
> +  crypt() without salt parameter raises E_NOTICE.
> +  See https://wiki.php.net/rfc/crypt_function_salt
> +
>   - cURL:
>     CURLOPT_SAFE_UPLOAD is now turned on by default and uploads with @file
>     do not work unless it is explicitly set to false.
> diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c
> index 113a5bd..eea6f78 100644
> --- a/ext/standard/crypt.c
> +++ b/ext/standard/crypt.c
> @@ -272,6 +272,8 @@ PHP_FUNCTION(crypt)
>
>    if (salt_in) {
>    memcpy(salt, salt_in, MIN(PHP_MAX_SALT_LEN, salt_in_len));
> + } else {
> + php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Weak hashed password is
> generated without salt parameter");
>    }
>
>    /* The automatic salt generation covers standard DES, md5-crypt and
> Blowfish (simple) */
>
>
> Output:
> [yohgaki@dev PHP-master]$ ./php-bin -d error_reporting=-1 -r
> "crypt('asdfa');"
> Notice: crypt(): Weak hashed password is generated without salt parameter
> in Command line code on line 1
>
>
> --
> Yasuo Ohgaki
> yohgaki@ohgaki.net
>
>
> On Mon, Oct 7, 2013 at 5:51 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> Hi all,
>>
>> Vote period is ended and the result was
>>
>> 0   Generate strong salt by default
>> 12 Generate E_NOTICE error
>> 2   Keep current behavior (Use weak hash)
>>
>> I'll prepare patch for it later.
>>
>> Thank you.
>>
>>
>> --
>> Yasuo Ohgaki
>> yohgaki@ohgaki.net
>>
>>
>> On Wed, Sep 25, 2013 at 1:17 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>>
>>> Hi Alexey,
>>>
>>> On Tue, Sep 24, 2013 at 11:22 PM, Alexey Zakhlestin <indeyets@gmail.com>wrote:
>>>
>>>> strangely, this RFC is not shown in "In voting phase" section here:
>>>> https://wiki.php.net/rfc
>>>> is that done manually?
>>>>
>>>
>>> I should have done this.
>>> Thank you for point it out.
>>>
>>> Regards,
>>>
>>> --
>>> Yasuo Ohgaki
>>> yohgaki@ohgaki.net
>>>
>>
>>
>

The wording is not very good/does not make sense:

Notice: crypt(): weak hash generated without salt in Command line code
on line 1

Cheers
Joe

Kingsquare.nl - Robin Speekenbrink

12 years ago

As a non-english speaker i'd like to throw my 2cents into the bag: Notice:
crypt(): weak hash generated due to lack of given salt in Command line code
on line 1

Regards,

Robin Speekenbrink


2013/10/22 Joe Watkins <krakjoe@php.net>

Andrew Faulds

12 years ago

On 22/10/2013 07:10, Yasuo Ohgaki wrote:

> Hi all,
>
> Any comments patch for this RFC?
> Better E_NOTICE message is welcome.
>

I'm a native English speaker, how about "Calling crypt() without giving
a salt will not produce strong password hashes."?

It doesn't necessarily say you will produce a strong hash with it (other
factors are at play), but it does say that you can't without it.

Perhaps "secure" might be better than "strong".

Just my 2 pence.

--
Andrea Faulds
http://ajf.me/

Adam Harvey

12 years ago

On 22 October 2013 02:58, Andrea Faulds <ajf@ajf.me> wrote:

> On 22/10/2013 07:10, Yasuo Ohgaki wrote:
>>
>> Any comments patch for this RFC?
>> Better E_NOTICE message is welcome.
>
> I'm a native English speaker, how about "Calling crypt() without giving a
> salt will not produce strong password hashes."?
>
> It doesn't necessarily say you will produce a strong hash with it (other
> factors are at play), but it does say that you can't without it.
>
> Perhaps "secure" might be better than "strong".

I think I'd prefer the wording to be a little stronger, since this is
going to be shown when the user has actually done that. How about:

"Generating an insecure weak hash as no salt was given: please ensure
the salt parameter is specified and uses a strong hash type in order
to generate a cryptographically secure hash"

On the bright side, at least php_error_docref() will ensure there's a
link to the crypt() manual page in most setups. Rereading that, we may
actually want to be slightly more opinionated there about which hash
types are good and which are bad (it's not at all obvious that the DES
and MD5 types shouldn't generally be used).

Adam

Joe Watkins

12 years ago

On 10/22/2013 06:11 PM, Adam Harvey wrote:

> On 22 October 2013 02:58, Andrea Faulds <ajf@ajf.me> wrote:
>> On 22/10/2013 07:10, Yasuo Ohgaki wrote:
>>>
>>> Any comments patch for this RFC?
>>> Better E_NOTICE message is welcome.
>>
>> I'm a native English speaker, how about "Calling crypt() without giving a
>> salt will not produce strong password hashes."?
>>
>> It doesn't necessarily say you will produce a strong hash with it (other
>> factors are at play), but it does say that you can't without it.
>>
>> Perhaps "secure" might be better than "strong".
>
> I think I'd prefer the wording to be a little stronger, since this is
> going to be shown when the user has actually done that. How about:
>
> "Generating an insecure weak hash as no salt was given: please ensure
> the salt parameter is specified and uses a strong hash type in order
> to generate a cryptographically secure hash"
>
> On the bright side, at least php_error_docref() will ensure there's a
> link to the crypt() manual page in most setups. Rereading that, we may
> actually want to be slightly more opinionated there about which hash
> types are good and which are bad (it's not at all obvious that the DES
> and MD5 types shouldn't generally be used).
>
> Adam
>

+1 that's good too ...

Wonder how well it will translate ??

Generating should be Generated, no ??

Cheers
Joe

Adam Harvey

12 years ago

On 22 October 2013 10:21, Joe Watkins <krakjoe@php.net> wrote:

> On 10/22/2013 06:11 PM, Adam Harvey wrote:
>> "Generating an insecure weak hash as no salt was given: please ensure
>> the salt parameter is specified and uses a strong hash type in order
>> to generate a cryptographically secure hash"
>
> Wonder how well it will translate ??

I tried to use the obvious scary keywords to make it obvious. If you
think the grammar on that version is convoluted, you should have seen
the first draft. :)

> Generating should be Generated, no ??

I like the present tense here — it's what crypt() IS doing, not what
it did. Makes it urgent. That said, I'm not super fussed either way.

Adam

Yasuo Ohgaki

12 years ago

Hi all,

On Wed, Oct 23, 2013 at 2:11 AM, Adam Harvey <aharvey@php.net> wrote:

> "Generating an insecure weak hash as no salt was given: please ensure
> the salt parameter is specified and uses a strong hash type in order
> to generate a cryptographically secure hash"
>

I guess this would be one of the longest error message, but it does not
matter.

If there isn't better message, I'll commit with this message in a few days.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Andrew Faulds

12 years ago

On 23/10/2013 01:26, Yasuo Ohgaki wrote:

> On Wed, Oct 23, 2013 at 2:11 AM, Adam Harvey <aharvey@php.net
> <mailto:aharvey@php.net>> wrote:
>
>     "Generating an insecure weak hash as no salt was given: please ensure
>     the salt parameter is specified and uses a strong hash type in order
>     to generate a cryptographically secure hash"
>
>
> I guess this would be one of the longest error message, but it does not
> matter.
>
> If there isn't better message, I'll commit with this message in a few days.
>

How about "No salt parameter was specified. You must use a randomly
generated salt and a strong hash function to produce a secure hash."

It doesn't emphasise the "strong" and "weak" as much, but I feel it gets
the message acrosss well nonetheless.

--
Andrea Faulds
http://ajf.me/

Yasuo Ohgaki

12 years ago

Hi all,

On Wed, Oct 23, 2013 at 9:35 AM, Andrea Faulds <ajf@ajf.me> wrote:

> How about "No salt parameter was specified. You must use a randomly
> generated salt and a strong hash function to produce a secure hash."

I'll use this error message.
If anyone care to replace message, please do.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net