On Thu, Apr 22, 2021 at 3:48 AM Sara Golemon <poll...@php.net> wrote:
> I have this notion that we've discussed this before, I'm certain I knew > that bcrypt wasn't binary safe, but someone reminded me that > password_hash() could be called with null bytes in the password itself and > that is just SCREAMING to have some safety-rails put on IMO. > > So I've thrown together https://github.com/php/php-src/pull/6897 to test > that argon2 algos behave well (they do!), and modified the bcrypt algo to > throw an exception if you try to hash a new password containing a null, but > only warns if you've got a null when running password_verify(). My > reasoning for the latter is because anyone trying to auth with a null > character that succeeds does definitely know enough of the password to pass > by simply not passing the null, so we shouldn't break existing users who > already have hashes. This only aims to break users who would otherwise be > able to include a null, because they would have a false sense of security > having their password truncated and can remedy that in their password > choosing. > > Since this does introduce a (small) break, I'm putting it to the list for > opinions. > If nobody objects, I'll merge this (8.1 only) at the end of the month. > I don't think this is a good idea. This adds an error condition that is based on the input string, which is generally user-provided. As there is nothing in the default stack preventing null bytes from passing in via HTTP (correct me if I'm wrong), this means the error can be trivially triggered, probably on about any PHP application using password_hash(). I don't think this is acceptable, for much the same reason that having file_exists() throw on null bytes is a bad idea (something we recently changed). I'm not sure what problem you're actually trying to solve here. Ultimately, all this allows is a user to shoot themselves in the foot if they want to -- and they do need to try actively, it's not like you can end up with a null byte in a password by accident. Regards, Nikita
