Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114095
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
In-Reply-To: <CAESVnVpCK87GL=+nCvXuT7YS6-QPhF3=8LZSZuAusGFXoT006A@mail.gmail.com>
Date: Thu, 22 Apr 2021 10:19:56 +0200
Cc: PHP internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <AD48D597-6189-4035-B890-C5F5257BDCDF@gmail.com>
References: <CAESVnVpCK87GL=+nCvXuT7YS6-QPhF3=8LZSZuAusGFXoT006A@mail.gmail.com>
To: Sara Golemon <pollita@php.net>
Subject: Re: [PHP-DEV] Binary (un)safety of password_hash() used with
 PASSWORD_BCRYPT
From: claude.pache@gmail.com (Claude Pache)



> Le 22 avr. 2021 =C3=A0 03:47, Sara Golemon <pollita@php.net> a =C3=A9cri=
t :
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> -Sara

Hi,

The intention is good, but the solution not so, especially for =
password_verify. You introduce a new, subtle way for this function to =
behave unexpectedly; because currently, password_verify does not =
currently trigger warning when given input of correct type. (Or if it =
does, it must be documented and fixed, because it would be a bad thing.)

Also, the warning message you introduced contains =E2=80=9C... this hash =
should be regenerated using ...=E2=80=9D. There is already a dedicated =
function for conveying that information in a coder-friendly way, namely =
`password_needs_rehash()`: let=E2=80=99s use that (it implies that the =
default algorithm should be changed).

=E2=80=94Claude=