crypt: support $2b$ prefix for blowfish

Submitted by Tim van der Staaij on Oct. 15, 2020, 7:56 p.m.

Details

Message ID 1752dd63ca0.f45c706c130962.3068699904194055827@tim.vanderstaaij.email
State New
Series "crypt: support $2b$ prefix for blowfish"
Headers show

Commit Message

Tim van der Staaij Oct. 15, 2020, 7:56 p.m.
2b is functionally equivalent to 2y, i.e. no known bugs at this time.

openbsd, which created the original bcrypt implementation,
and several other implementations use this prefix since 2014:
https://marc.info/?l=openbsd-misc&m=139320023202696
---
 src/crypt/crypt_blowfish.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/crypt/crypt_blowfish.c b/src/crypt/crypt_blowfish.c
index d3f79851..a5feffe7 100644
--- a/src/crypt/crypt_blowfish.c
+++ b/src/crypt/crypt_blowfish.c
@@ -533,6 +533,7 @@  static void BF_set_key(const char *key, BF_key expanded, BF_key initial,
  * Valid combinations of settings are:
  *
  * Prefix "$2a$": bug = 0, safety = 0x10000
+ * Prefix "$2b$": bug = 0, safety = 0
  * Prefix "$2x$": bug = 1, safety = 0
  * Prefix "$2y$": bug = 0, safety = 0
  */
@@ -600,7 +601,7 @@  static char *BF_crypt(const char *key, const char *setting,
 	char *output, BF_word min)
 {
 	static const unsigned char flags_by_subtype[26] =
-		{2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+		{2, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 		0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 4, 0};
 	struct {
 		BF_ctx ctx;
@@ -748,7 +749,7 @@  char *__crypt_blowfish(const char *key, const char *setting, char *output)
 	const char *test_setting = "$2a$00$abcdefghijklmnopqrstuu";
 	static const char test_hash[2][34] =
 		{"VUrPmXD6q/nVSSp7pNDhCR9071IfIRe\0\x55", /* $2x$ */
-		"i1D709vfamulimlGcq0qq3UvuUasvEa\0\x55"}; /* $2a$, $2y$ */
+		"i1D709vfamulimlGcq0qq3UvuUasvEa\0\x55"}; /* $2a$, $2b$, $2y$ */
 	char *retval;
 	const char *p;
 	int ok;
@@ -777,14 +778,14 @@  char *__crypt_blowfish(const char *key, const char *setting, char *output)
 	ok = (p == buf.o &&
 	    !memcmp(p, buf.s, 7 + 22) &&
 	    !memcmp(p + (7 + 22),
-	    test_hash[buf.s[2] & 1],
+	    test_hash[buf.s[2] != 'x'],
 	    31 + 1 + 1 + 1));
 
 	{
 		const char *k = "\xff\xa3" "34" "\xff\xff\xff\xa3" "345";
 		BF_key ae, ai, ye, yi;
 		BF_set_key(k, ae, ai, 2); /* $2a$ */
-		BF_set_key(k, ye, yi, 4); /* $2y$ */
+		BF_set_key(k, ye, yi, 4); /* $2b$, $2y$ */
 		ai[0] ^= 0x10000; /* undo the safety (for comparison) */
 		ok = ok && ai[0] == 0xdb9c59bc && ye[17] == 0x33343500 &&
 		    !memcmp(ae, ye, sizeof(ae)) &&

Comments

Solar Designer Oct. 15, 2020, 8:32 p.m.
Hi,

Wow, I didn't realize we forgot to get this into musl.

On Thu, Oct 15, 2020 at 09:56:56PM +0200, Tim van der Staaij wrote:
> 2b is functionally equivalent to 2y, i.e. no known bugs at this time.
> 
> openbsd, which created the original bcrypt implementation,
> and several other implementations use this prefix since 2014:
> https://marc.info/?l=openbsd-misc&m=139320023202696
> ---
>  src/crypt/crypt_blowfish.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Your patch looks OK to me at first glance, but it'd benefit from
existing testing and perhaps it'd help further maintenance to merge my
upstream changes instead of making slightly different (even if smaller)
changes specific to musl.

https://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/glibc/crypt_blowfish/crypt_blowfish.c.diff?r1=1.30;r2=1.31

> -	    test_hash[buf.s[2] & 1],
> +	    test_hash[buf.s[2] != 'x'],

This is really subtle, but I recall deliberately avoiding the "!= 'x'"
comparison as it's more likely to result in a conditional branch, which
is an additional side-channel leak about the hash sub-type.  We accept
leaks through data cache access patterns, but we avoided branching on
hash sub-type so far (let alone on anything truly security-sensitive).

As I recall, this is why I had to move flags_by_subtype to global scope
in that source file.

Alexander
Tim van der Staaij Oct. 15, 2020, 8:59 p.m.
Hi,

> Wow, I didn't realize we forgot to get this into musl.

Haha yes, what lead me to submitting this patch was an afternoon of
debugging why my nginx, linked with musl, was rejecting correct
passwords with a bcrypt credential file generated with python-passlib.

I figured it was probably just an oversight and it didn't look too
difficult to fix.

> Your patch looks OK to me at first glance, but it'd benefit from
> existing testing and perhaps it'd help further maintenance to merge my
> upstream changes instead of making slightly different (even if smaller)
> changes specific to musl.

I didn't realize that this code had an upstream, should've paid more
attention to the header! Of course I agree that syncing it with your
version would be a better solution.

> This is really subtle, but I recall deliberately avoiding the "!= 'x'"
> comparison as it's more likely to result in a conditional branch, which
> is an additional side-channel leak about the hash sub-type. We accept
> leaks through data cache access patterns, but we avoided branching on
> hash sub-type so far (let alone on anything truly security-sensitive).

> As I recall, this is why I had to move flags_by_subtype to global scope
> in that source file.

I see. I read about this phenomenon before, but actively recognizing and
avoiding such patterns is still quite a bit out my league.

Tim
Solar Designer Oct. 17, 2020, 7:27 p.m.
On Thu, Oct 15, 2020 at 10:59:42PM +0200, Tim van der Staaij wrote:
> I didn't realize that this code had an upstream, should've paid more
> attention to the header! Of course I agree that syncing it with your
> version would be a better solution.

Tim, are you going to submit a revised patch?

Alexander
Julien Ramseier Oct. 18, 2020, 8:57 a.m.
> Le 17 oct. 2020 à 21:27, Solar Designer <solar@openwall.com> a écrit :
> 
> On Thu, Oct 15, 2020 at 10:59:42PM +0200, Tim van der Staaij wrote:
>> I didn't realize that this code had an upstream, should've paid more
>> attention to the header! Of course I agree that syncing it with your
>> version would be a better solution.
> 
> Tim, are you going to submit a revised patch?
> 
> 

I already tried to upstream your changes some time ago, but it was not merged:
https://www.openwall.com/lists/musl/2017/01/12/6 <https://www.openwall.com/lists/musl/2017/01/12/6>

- Julien
Tim van der Staaij Oct. 18, 2020, 2:51 p.m.
> Tim, are you going to submit a revised patch?

Oh, I should've been clearer -- I wrote the patch based on the wrong
assumption that this was standalone code. Considering that this has
been fixed upstream, you can consider my patch null and void and
instead just take this thread as a reminder/request to sync it.
I'm sure you know best what the proper patch is to do so.
Thanks in advance!
Rich Felker Oct. 18, 2020, 4:12 p.m.
On Sun, Oct 18, 2020 at 10:57:43AM +0200, Julien Ramseier wrote:
> 
> 
> > Le 17 oct. 2020 à 21:27, Solar Designer <solar@openwall.com> a écrit :
> > 
> > On Thu, Oct 15, 2020 at 10:59:42PM +0200, Tim van der Staaij wrote:
> >> I didn't realize that this code had an upstream, should've paid more
> >> attention to the header! Of course I agree that syncing it with your
> >> version would be a better solution.
> > 
> > Tim, are you going to submit a revised patch?
> 
> I already tried to upstream your changes some time ago, but it was not merged:
> https://www.openwall.com/lists/musl/2017/01/12/6 <https://www.openwall.com/lists/musl/2017/01/12/6>

Sorry for missing your follow-up then. I don't see anything that could
be a problem in it so I'll apply it now.

Rich