[PATCH] crypt_blowfish: allow short salt strings

[PATCH] crypt_blowfish: allow short salt strings

[Timo Teräs]

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?

There seems to be some test suites that even verify that short
salt strings should succeed.

See: http://bugs.alpinelinux.org/issues/5141

diff --git a/src/crypt/crypt_blowfish.c b/src/crypt/crypt_blowfish.c
index d3f7985..d1f5588 100644
--- a/src/crypt/crypt_blowfish.c
+++ b/src/crypt/crypt_blowfish.c
@@ -365,6 +365,7 @@ static const unsigned char BF_atoi64[0x60] = {
 #define BF_safe_atoi64(dst, src) \
 { \
 	tmp = (unsigned char)(src); \
+	if (tmp == 0 || tmp == '$') break; \
 	if ((unsigned int)(tmp -= 0x20) >= 0x60) return -1; \
 	tmp = BF_atoi64[tmp]; \
 	if (tmp > 63) return -1; \
@@ -624,6 +625,7 @@ static char *BF_crypt(const char *key, const char *setting,
 		return NULL;
 	}
 
+	memset(data.binary.salt, 0, sizeof data.binary.salt);
 	count = (BF_word)1 << ((setting[4] - '0') * 10 + (setting[5] - '0'));
 	if (count < min || BF_decode(data.binary.salt, &setting[7], 16)) {
 		return NULL;
-- 
2.7.4

Re: [PATCH] crypt_blowfish: allow short salt strings

[Solar Designer]

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

Re: [PATCH] crypt_blowfish: allow short salt strings

[Solar Designer]

On Sun, Mar 27, 2016 at 05:11:21AM +0300, Solar Designer wrote:
> On Fri, Mar 25, 2016 at 02:12:35PM +0200, Timo Ter??s wrote:
> > 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.

I found that PHP's hack was introduced in commit:

commit 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9
Author: Pierre Joye <pajoye@php.net>
Date:   Mon Jul 18 21:26:29 2011 +0000

    - update blowfish to 1.2 (Solar Designer)

$ git show 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9 | fgrep -i hack
+       if (tmp == '$') break; /* PHP hack */ \
+       while (dptr < end) /* PHP hack */

I think they shouldn't have.  Perhaps someone complained at the time,
but since then this hack resulted in more incorrect PHP code written,
relying on the hack.

Alexander

Re: [PATCH] crypt_blowfish: allow short salt strings

[Solar Designer]

On Sun, Mar 27, 2016 at 05:54:04AM +0300, Solar Designer wrote:
> I found that PHP's hack was introduced in commit:
> 
> commit 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9
> Author: Pierre Joye <pajoye@php.net>
> Date:   Mon Jul 18 21:26:29 2011 +0000
> 
>     - update blowfish to 1.2 (Solar Designer)
> 
> $ git show 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9 | fgrep -i hack
> +       if (tmp == '$') break; /* PHP hack */ \
> +       while (dptr < end) /* PHP hack */

Correction: this commit merely documented the hack with those comments,
but the hack itself was in there before.

I just brought the issue up on the PHP internals list:

http://news.php.net/php.internals/91969

A sub-issue is that the padding appears to vary between PHP versions or
builds: some pad with zero bits, and some (5.4.x only?) with '$' signs.

Alexander

Re: [PATCH] crypt_blowfish: allow short salt strings

[Solar Designer]

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).

Alexander

Re: [PATCH] crypt_blowfish: allow short salt strings

[Rich Felker]

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