# password_hash() deprecate salt option - thoughts? - Thread: https://externals.io/message/85595 - Original archive: https://news-web.php.net/php.internals/85595 - Started by Anthony Ferrara on 2015-03-31T18:49:24+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 --- ## #85595 — Anthony Ferrara — 2015-03-31T18:49:24+00:00 URL: https://externals.io/message/85595#85595 All, Ever since we introduced `password_hash()` in 5.5, I've been watching its usage as much as possible. I've setup google alerts and such, as well as auditing implementations I've found on github to try to understand how it's used. One thing has become abundantly clear to me: the salt option is dangerous. I've yet to see a single usage of the salt option that has been even decent. Every usage ranges from bad (passing `mt_rand()` output) to dangerous (static strings) to insane (passing the password as its own salt). I've come to the conclusion that I don't think we should allow users to specify the salt. The `crypt()` API still exists if users have a need to generate their own salt. Having it in the simplified API is simply adding a risk factor without any significant justification. So I'd like to hear your thoughts about raising `E_DEPRECATED` when the salt option is specified in 7.0, with ultimately removing the option in a later version. Additionally, I know this is after the RFC freeze deadline, so if you want to postpone the deprecation to 7.1, that's fine. I just think it's worth discussion (and if there's consensus to put it in 7.0, then great). Thanks, Anthony --- ## #85597 — Alex Bowers — 2015-03-31T18:58:10+00:00 URL: https://externals.io/message/85595#85597 · In reply to: #85595 I think deprecating it is a good idea, and looking at the documentation it does mention that not providing it is the intended option; so it isn't a complete surprise for it to become deprecated. > All, > > Ever since we introduced `password_hash()` in 5.5, I've been watching > its usage as much as possible. I've setup google alerts and such, as > well as auditing implementations I've found on github to try to > understand how it's used. > > One thing has become abundantly clear to me: the salt option is > dangerous. I've yet to see a single usage of the salt option that has > been even decent. Every usage ranges from bad (passing `mt_rand()` > output) to dangerous (static strings) to insane (passing the password > as its own salt). > > I've come to the conclusion that I don't think we should allow users > to specify the salt. The `crypt()` API still exists if users have a need > to generate their own salt. Having it in the simplified API is simply > adding a risk factor without any significant justification. > > So I'd like to hear your thoughts about raising `E_DEPRECATED` when the > salt option is specified in 7.0, with ultimately removing the option > in a later version. > > Additionally, I know this is after the RFC freeze deadline, so if you > want to postpone the deprecation to 7.1, that's fine. I just think > it's worth discussion (and if there's consensus to put it in 7.0, then > great). > > Thanks, > > Anthony --- ## #85598 — Nicolas Oelgart — 2015-03-31T19:13:50+00:00 URL: https://externals.io/message/85595#85598 · In reply to: #85595 > > > All, > > Ever since we introduced `password_hash()` in 5.5, I've been watching > its usage as much as possible. I've setup google alerts and such, as > well as auditing implementations I've found on github to try to > understand how it's used. > > One thing has become abundantly clear to me: the salt option is > dangerous. I've yet to see a single usage of the salt option that has > been even decent. Every usage ranges from bad (passing `mt_rand()` > output) to dangerous (static strings) to insane (passing the password > as its own salt). > > I've come to the conclusion that I don't think we should allow users > to specify the salt. The `crypt()` API still exists if users have a need > to generate their own salt. Having it in the simplified API is simply > adding a risk factor without any significant justification. > > So I'd like to hear your thoughts about raising `E_DEPRECATED` when the > salt option is specified in 7.0, with ultimately removing the option > in a later version. > > Additionally, I know this is after the RFC freeze deadline, so if you > want to postpone the deprecation to 7.1, that's fine. I just think > it's worth discussion (and if there's consensus to put it in 7.0, then > great). > > Thanks, > > Anthony > > -- > > +1 I'd even go as far as adding a big red warning about custom salts to the manual page. --- ## #85599 — Christoph Becker — 2015-03-31T19:32:50+00:00 URL: https://externals.io/message/85595#85599 · In reply to: #85598 Nicolas Oelgart wrote: > > […] > > +1 > > I'd even go as far as adding a big red warning about custom salts to the manual page. FWIW, there is already the following note: | Caution It is strongly recommended that you do not generate your own | salt for this function. It will create a secure salt automatically | for you if you do not specify one. -- Christoph M. Becker --- ## #85600 — Nikita Popov — 2015-03-31T19:41:47+00:00 URL: https://externals.io/message/85595#85600 · In reply to: #85595 On Tue, Mar 31, 2015 at 8:49 PM, Anthony Ferrara wrote: > All, > > Ever since we introduced `password_hash()` in 5.5, I've been watching > its usage as much as possible. I've setup google alerts and such, as > well as auditing implementations I've found on github to try to > understand how it's used. > > One thing has become abundantly clear to me: the salt option is > dangerous. I've yet to see a single usage of the salt option that has > been even decent. Every usage ranges from bad (passing `mt_rand()` > output) to dangerous (static strings) to insane (passing the password > as its own salt). > > I've come to the conclusion that I don't think we should allow users > to specify the salt. The `crypt()` API still exists if users have a need > to generate their own salt. Having it in the simplified API is simply > adding a risk factor without any significant justification. > > So I'd like to hear your thoughts about raising `E_DEPRECATED` when the > salt option is specified in 7.0, with ultimately removing the option > in a later version. > > Additionally, I know this is after the RFC freeze deadline, so if you > want to postpone the deprecation to 7.1, that's fine. I just think > it's worth discussion (and if there's consensus to put it in 7.0, then > great). > Agree with deprecating the custom salt option and also okay with doing it in 7.0. Nikita --- ## #85602 — Nico — 2015-03-31T20:02:26+00:00 URL: https://externals.io/message/85595#85602 · In reply to: #85599 > > > Nicolas Oelgart wrote: > > > […] > > FWIW, there is already the following note: > > | Caution It is strongly recommended that you do not generate your own > | salt for this function. It will create a secure salt automatically > | for you if you do not specify one. > > -- > Christoph M. Becker > Yeah, I’m aware. But I don’t think it’s enough. I’d suggest moving it further to the top, and making it red. As Anthony’s research shows, the current note is not enough. People are still doing it wrong. — Nico --- ## #85606 — Paul Dragoonis — 2015-03-31T20:32:37+00:00 URL: https://externals.io/message/85595#85606 · In reply to: #85595 On Tue, Mar 31, 2015 at 7:49 PM, Anthony Ferrara wrote: > All, > > Ever since we introduced `password_hash()` in 5.5, I've been watching > its usage as much as possible. I've setup google alerts and such, as > well as auditing implementations I've found on github to try to > understand how it's used. > > One thing has become abundantly clear to me: the salt option is > dangerous. I've yet to see a single usage of the salt option that has > been even decent. Every usage ranges from bad (passing `mt_rand()` > output) to dangerous (static strings) to insane (passing the password > as its own salt). > > I've come to the conclusion that I don't think we should allow users > to specify the salt. The `crypt()` API still exists if users have a need > to generate their own salt. Having it in the simplified API is simply > adding a risk factor without any significant justification. > > So I'd like to hear your thoughts about raising `E_DEPRECATED` when the > salt option is specified in 7.0, with ultimately removing the option > in a later version. > > Additionally, I know this is after the RFC freeze deadline, so if you > want to postpone the deprecation to 7.1, that's fine. I just think > it's worth discussion (and if there's consensus to put it in 7.0, then > great). > > Thanks, > > Anthony > No objections here. You're going with your gut on this and it seems like a good call. We did make a freeze and although I'd like it in 7.0 we should probably stick to our process here and put it in 7.1. To be fair, there's always a quick *.1 release anyway once we find real-world bugs that need patching quickly ;-) --- ## #85614 — keisial@gmail.com — 2015-03-31T22:21:10+00:00 URL: https://externals.io/message/85595#85614 · In reply to: #85606 No objections here either. --- ## #85645 — Anthony Ferrara — 2015-04-01T18:26:27+00:00 URL: https://externals.io/message/85595#85645 · In reply to: #85614 All, I've added a PR for this: https://github.com/php/php-src/pull/1213 Please review the implementation and the wording (as well as the behavior). I plan on merging this on Friday if there is no objection (as it seems the support has already been unanimous with no hesitation). Thanks! Anthony > No objections here either. --- ## #85760 — Anthony Ferrara — 2015-04-09T20:02:20+00:00 URL: https://externals.io/message/85595#85760 · In reply to: #85645 I gave it some additional time in case others raised concern. I have since merged the deprecation of the salt option to `password_hash()` Thanks! Anthony > All, > > I've added a PR for this: https://github.com/php/php-src/pull/1213 > > Please review the implementation and the wording (as well as the behavior). > > I plan on merging this on Friday if there is no objection (as it seems > the support has already been unanimous with no hesitation). > > Thanks! > > Anthony > > > > […]