# [VOTE] Change crypt() behavior w/o salt - Thread: https://externals.io/message/69302 - Original archive: https://news-web.php.net/php.internals/69302 - Started by Yasuo Ohgaki on 2013-09-24T01:41:47+00:00 - Messages: 16 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #69302 — Yasuo Ohgaki — 2013-09-24T01:41:47+00:00 URL: https://externals.io/message/69302#69302 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 --- ## #69305 — Pierre Joye — 2013-09-24T02:47:12+00:00 URL: https://externals.io/message/69302#69305 · In reply to: #69302 > > 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 --- ## #69316 — William Bartlett — 2013-09-24T14:13:30+00:00 URL: https://externals.io/message/69302#69316 · In reply to: #69305 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 > > > […] > > 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 --- ## #69317 — Alexey Zakhlestin — 2013-09-24T14:22:18+00:00 URL: https://externals.io/message/69302#69317 · In reply to: #69302 > 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 --- ## #69330 — Yasuo Ohgaki — 2013-09-25T04:17:56+00:00 URL: https://externals.io/message/69302#69330 · In reply to: #69317 Hi Alexey, On Tue, Sep 24, 2013 at 11:22 PM, Alexey Zakhlestin 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 --- ## #69492 — Yasuo Ohgaki — 2013-10-07T08:51:13+00:00 URL: https://externals.io/message/69302#69492 · In reply to: #69330 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 > Hi Alexey, > > On Tue, Sep 24, 2013 at 11:22 PM, Alexey Zakhlestin wrote: > > > […] > > I should have done this. > Thank you for point it out. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #69738 — Yasuo Ohgaki — 2013-10-22T06:10:35+00:00 URL: https://externals.io/message/69302#69738 · In reply to: #69492 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 > 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 > > > > > > […] --- ## #69742 — Joe Watkins — 2013-10-22T07:23:54+00:00 URL: https://externals.io/message/69302#69742 · In reply to: #69738 > 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 > > > > > > […] > 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 --- ## #69743 — Kingsquare.nl - Robin Speekenbrink — 2013-10-22T07:35:16+00:00 URL: https://externals.io/message/69302#69743 · In reply to: #69742 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 > > > > […] > > Notice: `crypt()`: weak hash generated without salt in Command line code on > line 1 > > Cheers > Joe --- ## #69751 — Andrea Faulds — 2013-10-22T09:58:48+00:00 URL: https://externals.io/message/69302#69751 · In reply to: #69738 > 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/ --- ## #69768 — Adam Harvey — 2013-10-22T17:11:00+00:00 URL: https://externals.io/message/69302#69768 · In reply to: #69751 > > > […] > > 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 --- ## #69770 — Joe Watkins — 2013-10-22T17:21:08+00:00 URL: https://externals.io/message/69302#69770 · In reply to: #69768 > > > […] > > 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 --- ## #69771 — Adam Harvey — 2013-10-22T17:23:56+00:00 URL: https://externals.io/message/69302#69771 · In reply to: #69770 > > > […] > > 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 --- ## #69790 — Yasuo Ohgaki — 2013-10-23T00:26:39+00:00 URL: https://externals.io/message/69302#69790 · In reply to: #69768 Hi all, > "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 --- ## #69791 — Andrea Faulds — 2013-10-23T00:35:02+00:00 URL: https://externals.io/message/69302#69791 · In reply to: #69790 > On Wed, Oct 23, 2013 at 2:11 AM, 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" > > > 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/ --- ## #69927 — Yasuo Ohgaki — 2013-10-29T08:49:00+00:00 URL: https://externals.io/message/69302#69927 · In reply to: #69791 Hi all, > 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