Re: [PATCH] crypt_blowfish: allow short salt strings

[Solar Designer <solar@openwall.com>]

Hi Timo,

On Fri, Mar 25, 2016 at 02:12:35PM +0200, Timo Ter??s wrote:
> assume the remainder of the salt to be zero bits.
> ---
>  src/crypt/crypt_blowfish.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Any comments if this makes sense?

Sort of, but we shouldn't do it.

crypt_blowfish is deliberately strict, and recent versions of it (after
the inclusion in musl, though) were cross-tested against OpenBSD's
original implementation resulting in the latter becoming stricter as
well, including on wrong inputs like this.  Prior to that cross-testing,
IIRC it was possible for OpenBSD's implementation to leak uninitialized
stack space on too-short salts like this.  This confirms that too-short
salts were never meant to be supported.  OpenBSD is upstream for bcrypt.

> There seems to be some test suites that even verify that short
> salt strings should succeed.
> 
> See: http://bugs.alpinelinux.org/issues/5141

This looks like a script testing PHP's behavior.  I vaguely recall PHP
relaxing the PHP-embedded crypt_blowfish code like this.  I think they
shouldn't have.  Especially they shouldn't have done that when at the
same time (apparently) continuing to detect and prefer the underlying
system's bcrypt support whenever that is available.

The behavior you're observing on a system with glibc is most likely
actually that of PHP's embedded copy of crypt_blowfish, since upstream
glibc lacks bcrypt support (or has this changed?)  Only some Linux
distros with glibc have bcrypt support patched into their glibc.

There is one maybe-bug seen in your test results: the "*" return on
error.  Normally, it would be "*0" or "*1" for crypt_blowfish, and it
would never match the salt input.  Rich, did musl retain this behavior?
Where does the bare "*" come from?  The concern here is that a "*" might
also be returned when crypt() is called with "*" for the salt input.
Then its output would match what may be a stored hash placeholder, where
"*" means locked account or an error having occurred when the password
was set.  We must deny access in such cases, but returning "*" on all
errors would grant access.  This could be a musl or PHP security bug, if
it's indeed as bad as it appears from that test.

Alexander