Skip to content

Conversation

ircmaxell
Copy link
Contributor

Refactor the password_hash random entropy gathering for salt generation to internally use the random_bytes API to unify and consolidate randomness.

@KalleZ
Copy link
Member

KalleZ commented Oct 19, 2015

Although mentioned on internals that it would be better to wait for the 7.0.1 patch release, I'll would at least mark the function with PHPAPI prior gold

cc @weltling

@KalleZ
Copy link
Member

KalleZ commented Oct 19, 2015

Update on this.

Me and Anatol had a chat about this, and agree that we would rather have this go into master (for 7.1) instead, as it does seem to have a slight side effect of throwing exceptions in case of failure. I don't think the "auto-guess" part as mentioned on internals is too big a loss anyway, as its more of an edge case anyway.

@tom--
Copy link

tom-- commented Oct 19, 2015

@ircmaxell doesn't do his patch justice. It's not just refactoring. This is a security fix in which there's no way around app breakage in the "failure" case.

A password salt needs to be unique. It does not need to be drawn from a CSPRNG but that's the only way I know to be reasonably confident of uniqueness. I can seed php_rand() from my script but, other than using the platform CSPRNG, I have no idea how. Or I can let PHP seed it but its algorithm, a function of time and PID, shows PHP doesn't know how either. Neither passes a security sniff test.

I think it's better to break apps on a broken PHP runtime environment (rather than hide the problem) in 7.0.

@tom--
Copy link

tom-- commented Oct 19, 2015

I submitted bug 70743 to back this up. It's marked as private :(

@php-pulls
Copy link

Comment on behalf of ab at php.net:

Pulled this with a small correction to use php_random_bytes_silent() from #1614 which was merged already. Thanks!

@php-pulls php-pulls closed this Dec 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants