Re: Password_hash salt generation refactor

On 10/30/15 2:36 PM, Anatol Belski wrote:

Hi Anthony,

-----Original Message----- From: Anthony Ferrara [mailto:ircmaxell@gmail.com] Sent: Friday, October 30, 2015 11:58 AM All, On Tue, Oct 20, 2015 at 11:35 PM, Anatol Belski <anatol.php@belski.net> wrote:

Could php_random_bytes() be extended with a flag that would tell exceptions

to pass? Or maybe exceptions could be moved out from the php_random_bytes() and thrown directly in the new functions that was undergoing the RFC but not password_hash() ? I'd be actually for the second variant.

That ways FAILURE would be returned, but a decision about what to do about

it would be made outside. IMHO it were also useful as the API is intended to be exported. So for the cases where php_random_bytes() came to use as a library function outside immediate PHP userspace (for example in SAPI), it should not throw exceptions from itself.

Do this suggestions sound eligible?

I have created a PR for this: https://github.com/php/php-src/pull/1614 It changes the public API of the internal php_random_bytes function to have a third "should_throw" parameter, as well as defining two macros: php_random_bytes_throw(dest, len) and php_random_bytes_silent(dest, len). If this looks acceptable, it can be pulled into 7.0, and then the move for password_hash can be done at a later date.

IMHO this looks fine for 7.0. The API is new and was not exported, so no ABI breach and no behavior change when used for internal refactoring.

This change to php_random_bytes() should make it easier to migrate various old stuff (e.g. Leigh proposed sessions) to the new source, and I like it for that. But silenced failure should probably not be used for anything new and it would be good if a developer refactoring an old API would comment how the new code processes FAILURE together with a justification. Could comments be included in this PR to encourage it? As it stands, this change allows hazardous use without mentioning it. Tom