mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] crypt: support $2b$ prefix for blowfish
@ 2020-10-15 19:56 Tim van der Staaij
  2020-10-15 20:32 ` Solar Designer
  0 siblings, 1 reply; 7+ messages in thread
From: Tim van der Staaij @ 2020-10-15 19:56 UTC (permalink / raw)
  To: musl

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

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)) &&
-- 
2.23.0





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-15 19:56 [musl] [PATCH] crypt: support $2b$ prefix for blowfish Tim van der Staaij
@ 2020-10-15 20:32 ` Solar Designer
  2020-10-15 20:59   ` Tim van der Staaij
  0 siblings, 1 reply; 7+ messages in thread
From: Solar Designer @ 2020-10-15 20:32 UTC (permalink / raw)
  To: Tim van der Staaij; +Cc: musl

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-15 20:32 ` Solar Designer
@ 2020-10-15 20:59   ` Tim van der Staaij
  2020-10-17 19:27     ` Solar Designer
  0 siblings, 1 reply; 7+ messages in thread
From: Tim van der Staaij @ 2020-10-15 20:59 UTC (permalink / raw)
  To: musl

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-15 20:59   ` Tim van der Staaij
@ 2020-10-17 19:27     ` Solar Designer
  2020-10-18  8:57       ` Julien Ramseier
  0 siblings, 1 reply; 7+ messages in thread
From: Solar Designer @ 2020-10-17 19:27 UTC (permalink / raw)
  To: Tim van der Staaij; +Cc: musl

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-17 19:27     ` Solar Designer
@ 2020-10-18  8:57       ` Julien Ramseier
  2020-10-18 14:51         ` Tim van der Staaij
  2020-10-18 16:12         ` Rich Felker
  0 siblings, 2 replies; 7+ messages in thread
From: Julien Ramseier @ 2020-10-18  8:57 UTC (permalink / raw)
  To: musl; +Cc: Tim van der Staaij

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]



> 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


[-- Attachment #2: Type: text/html, Size: 1233 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-18  8:57       ` Julien Ramseier
@ 2020-10-18 14:51         ` Tim van der Staaij
  2020-10-18 16:12         ` Rich Felker
  1 sibling, 0 replies; 7+ messages in thread
From: Tim van der Staaij @ 2020-10-18 14:51 UTC (permalink / raw)
  To: musl

> 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!


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [musl] [PATCH] crypt: support $2b$ prefix for blowfish
  2020-10-18  8:57       ` Julien Ramseier
  2020-10-18 14:51         ` Tim van der Staaij
@ 2020-10-18 16:12         ` Rich Felker
  1 sibling, 0 replies; 7+ messages in thread
From: Rich Felker @ 2020-10-18 16:12 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-18 16:12 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 19:56 [musl] [PATCH] crypt: support $2b$ prefix for blowfish Tim van der Staaij
2020-10-15 20:32 ` Solar Designer
2020-10-15 20:59   ` Tim van der Staaij
2020-10-17 19:27     ` Solar Designer
2020-10-18  8:57       ` Julien Ramseier
2020-10-18 14:51         ` Tim van der Staaij
2020-10-18 16:12         ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).