# crypt() should raise error without 2nd parameter - Thread: https://externals.io/message/68396 - Original archive: https://news-web.php.net/php.internals/68396 - Started by Yasuo Ohgaki on 2013-08-07T10:18:15+00:00 - Messages: 10 - 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 --- ## #68396 — Yasuo Ohgaki — 2013-08-07T10:18:15+00:00 URL: https://externals.io/message/68396#68396 Hi all, A user requested that `crypt()` should raise error without 2nd(slat) parameter. https://bugs.php.net/bug.php?id=55036 `crypt()` without salt generates extremely weak password hash. In addition to this, PHP 5.5 has `password_hash()` This change should be applied from 5.5, IMHO. Any comments? -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68397 — Leigh — 2013-08-07T11:00:00+00:00 URL: https://externals.io/message/68396#68397 · In reply to: #68396 > A user requested that `crypt()` should raise error without 2nd(slat) > parameter. > > https://bugs.php.net/bug.php?id=55036 > > `crypt()` without salt generates extremely weak password hash. > The docs seem to indicate that some implementations generate their own random salt if one is not supplied? It doesn't seem right to raise a warning if it doesn't apply to _all_ cases. --- ## #68398 — keisial@gmail.com — 2013-08-07T11:58:32+00:00 URL: https://externals.io/message/68396#68398 · In reply to: #68397 > > > > […] > The docs seem to indicate that some implementations generate their own > random salt if one is not supplied? It doesn't seem right to raise a > warning if it doesn't apply to _all_ cases. I do get a md5 with a salt when calling crypt, and looking at php that seems to be the intended behavior, not something system dependant (that's done since 5.3, according to the docs). I see a problem in that it uses php_rand() to generate the salt, but the solution should be to use php_password_make_salt for creating the salt, not the warning. --- ## #68399 — Kalle Sommer Nielsen — 2013-08-07T13:21:30+00:00 URL: https://externals.io/message/68396#68399 · In reply to: #68396 Hi Yasuo 2013/8/7 Yasuo Ohgaki : > Hi all, > > A user requested that `crypt()` should raise error without 2nd(slat) > parameter. > > https://bugs.php.net/bug.php?id=55036 > > `crypt()` without salt generates extremely weak password hash. In addition to > this, > PHP 5.5 has `password_hash()` > > This change should be applied from 5.5, IMHO. This is a BC break, as the second parameter as noted is optional, while I believe we can do it in 5.5, I don't think it is worth the effort, neither to put a notice or similar (E_STRICT even), if the second parameter is left out, and it should go in master instead which I'm in favor of. -- regards, Kalle Sommer Nielsen kalle@php.net --- ## #68400 — Anthony Ferrara — 2013-08-07T13:50:11+00:00 URL: https://externals.io/message/68396#68400 · In reply to: #68396 Yasuo, Hi all, > > A user requested that `crypt()` should raise error without 2nd(slat) > parameter. > > https://bugs.php.net/bug.php?id=55036 > > `crypt()` without salt generates extremely weak password hash. In addition to > this, > PHP 5.5 has `password_hash()` > > This change should be applied from 5.5, IMHO. > > Any comments? > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > Well, first off, a BC break like this should never go into a current release. So I'd support for master / NEXT only anyway. I did some digging, and it appears that the python module also does similar behavior (allowing a null salt). But their behavior is to generate a strong salt for the strongest algorithm available. So their behavior is actually useful. The other implementations that I've looked at would all error or simply ignore the fact that the salt was empty (as a valid DES salt). So, keeping with standard practices, I think we should E_DEPRECATE the usage with only 1 parameter (no salt), and then in NEXT.NEXT change the zend_parse_parameters definition to require 2 parameters... My $0.02... --- ## #68403 — Stas Malyshev — 2013-08-07T20:30:09+00:00 URL: https://externals.io/message/68396#68403 · In reply to: #68396 Hi! > A user requested that `crypt()` should raise error without 2nd(slat) > parameter. > > https://bugs.php.net/bug.php?id=55036 > > `crypt()` without salt generates extremely weak password hash. In addition to > this, I see that when I run crypt with one parameter, it generates salted password hash. I imagine since on many systems it will produce md5-based hash which is no longer considered adequate for many applications, it may be not the best way to use it, but I don't see how it is an error to do it. I'd rather have `crypt()` use stronger hash by default or maybe have parameter that sets which hash is being used. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68413 — Yasuo Ohgaki — 2013-08-08T01:20:42+00:00 URL: https://externals.io/message/68396#68413 · In reply to: #68403 Hi all, It seems there are 2 options for master branch when `crypt()`'s 2nd parameter is omitted. - raise E_DEPRECIATED that advice use of stronger salt or `password_hash()` and make 2nd parameter required for future release. - make `crypt()` use stronger default salt/hash w/o error Since `password_hash()` is supposed to do better job, first option seems better to me. Do I have to setup vote? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68423 — Hannes Magnusson — 2013-08-08T04:22:36+00:00 URL: https://externals.io/message/68396#68423 · In reply to: #68413 > Hi all, > > It seems there are 2 options for master branch when `crypt()`'s 2nd parameter > is omitted. > > - raise E_DEPRECIATED that advice use of stronger salt or `password_hash()` > and make 2nd parameter required for future release. > - make `crypt()` use stronger default salt/hash w/o error > > Since `password_hash()` is supposed to do better job, first option seems > better to me. Deprecating it means it will be removed in the future. Please leave the function alone. This should be solved with education, not a gun to peoples head. -Hannes --- ## #68426 — Yasuo Ohgaki — 2013-08-08T04:44:59+00:00 URL: https://externals.io/message/68396#68426 · In reply to: #68423 Hi Hannes, On Thu, Aug 8, 2013 at 1:22 PM, Hannes Magnusson wrote: > > > […] > parameter > > […] > `password_hash()` > > […] > > > Deprecating it means it will be removed in the future. > > Please leave the function alone. This should be solved with education, > not a gun to peoples head. This would be third option. I agree that good documentation is always good. `E_NOTICE` might be better as E_DEPRECIATED means obsolete. I'll write RFC for voting later. Please comment so that your comments are in RFC. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68596 — Yasuo Ohgaki — 2013-08-22T11:30:06+00:00 URL: https://externals.io/message/68396#68596 · In reply to: #68396 Hi all, > A user requested that `crypt()` should raise error without 2nd(slat) > parameter. > > https://bugs.php.net/bug.php?id=55036 > > `crypt()` without salt generates extremely weak password hash. In addition > to this, > PHP 5.5 has `password_hash()` > > This change should be applied from 5.5, IMHO. > > Any comments? > I've made RFC for this issue. https://wiki.php.net/rfc/crypt_function_salt Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net