Re: [PATCH] crypt_blowfish: allow short salt strings

[Rich Felker <dalias@libc.org>]

On Sun, Mar 27, 2016 at 06:30:07AM +0300, Solar Designer wrote:
> On Sun, Mar 27, 2016 at 05:11:21AM +0300, Solar Designer wrote:
> > 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.
> 
> I just discussed this with Rich.
> 
> Yes, musl's modified crypt_blowfish returns "*" on error.  No, this
> isn't a security bug in musl as a whole, because that code path is not
> reached when the setting argument is "*" as well.  So no match.
> 
> However, there's risk for code reuse from musl by other projects, and
> for potential cut-down revisions of musl (with only bcrypt left, invoked
> unconditionally).  So I think a change is needed, either reintroducing
> the "*0" / "*1" behavior (my preference) or returning NULL (Rich's
> preference) on error like glibc does (and unfortunately crashing older
> programs that don't expect this).

The reason I prefer returning null is that applications get a
specified diagnostic that the provided setting string failed to
produce a matchable hash; this allows correct applications to avoid
storing an unmatchable hash that would render password authentication
impossible (always-failing) and instead retry with different settings.
This also seems to be the only reasonable way to runtime-probe for
which hashes are supported.

Solar has pointed out to me that you can use strlen(result)<13 as a
failure condition, and that robust programs may already be doing this,
but it seems hackish and it's not documented/specified anywhere.

Rich