Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114091
MIME-Version: 1.0
References: <CAESVnVpCK87GL=+nCvXuT7YS6-QPhF3=8LZSZuAusGFXoT006A@mail.gmail.com>
In-Reply-To: <CAESVnVpCK87GL=+nCvXuT7YS6-QPhF3=8LZSZuAusGFXoT006A@mail.gmail.com>
Date: Thu, 22 Apr 2021 07:57:45 +0200
Message-ID: <CANUQDCjVfdggrF3wjBCsQ=b_85cq1RhnO_kkVfP3QGubmZhg+g@mail.gmail.com>
To: Sara Golemon <pollita@php.net>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003b497605c0895ce6"
Subject: Re: [PHP-DEV] Binary (un)safety of password_hash() used with PASSWORD_BCRYPT
From: me@kelunik.com (Niklas Keller)

--0000000000003b497605c0895ce6
Content-Type: text/plain; charset="UTF-8"

Sara Golemon <pollita@php.net> schrieb am Do., 22. Apr. 2021, 03:47:

> 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.
>

Hey Sara,

Thank you for your work. I think this is a really good safety check to
have. I'd however go a step further and also throw on NUL in
password_verify.

You seem to assume that NUL bytes as input come from the end user, but I
think it's more likely the developer uses a hash function with raw output
as a pre-encoding, for reasons such as bringing long passwords below the
bcrypt character limit.

Best,
Niklas

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.
>
> -Sara
>

--0000000000003b497605c0895ce6--