Message-ID: <1f3f264bd40b6eba7380e7da19013417029a93aa.camel@oopsware.de>
Subject: Re: Modern SHA2- based password hashes for pgcrypto
From: Bernd Helmle <mailings@oopsware.de>
To: Japin Li <japinli@hotmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, PostgreSQL Development
	 <pgsql-hackers@postgresql.org>
Date: Fri, 24 Jan 2025 18:14:45 +0100
In-Reply-To: <ME0P300MB0445F3242216930DE6D44A24B6E02@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
References: <202501131606.h6ctz533qhy6@alvherre.pgsql>
		<3e48cd7c02f437ed980ee57340cb5c54f60e359c.camel@oopsware.de>
	 <ME0P300MB0445F3242216930DE6D44A24B6E02@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
Autocrypt: addr=mailings@oopsware.de; prefer-encrypt=mutual;
 keydata=mQGiBEIM4dYRBACEnxQZ/CfR1xbCJzGOIjvh3fZpLALMm+tt237QlZm9nlLaEwCn317sT
 1khPAJwhyBHSHbNFbH+LOL6ezYrg4V2Q9KHyYJDStJtC8c+D7vHVB5Jfslynx89DnCH6jb3H0j+U0
 iNYmRmT18PWTZhD5RuXT+Toyz/0hH4YnJzha4VUwCggikXRuhIQ0KIzrps7/PsvasTPjUD/1aoEHR
 wVyk2C2hXh6GWJYq+1l+sB2d0TOnFJf2xy3Xo03Hi+BEJJdYGqJKqzd0KfsFyJYhO2GIGScSEj1hs
 TqAiY8OZeRqRGHe69OQEguUSVXp/jPaqU8fel8dbDDMnXkgULq8oO4T9e4GJSFqIvrlx0sJqSMFdi
 ihV+xCzDS40BACBrTq9tyXmyxJWzQ1/0Hx9S4sU+yeR8ztZH9xgWi1wksfHpEgZBdAkQynlI80/bC
 tgxfVu+LKgxeqGfMSnT9t8agpr1IGcmSbDntbn/K+3EmCze0jq8EQrZHJWiwCqraM8mnmILSEl5zn
 aoVoBIF062b60zZicuNeYaVgEKSHfirQgQmVybmQgSGVsbWxlIDxiZXJuZEBvb3Bzd2FyZS5kZT6I
 XgQTEQIAHgUCQhyP2gIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzMm4AJ9mW1np2
 RXi66WzEvcIanaJkDT+xgCfZmXG/Sziz5FchI3OjXfXDZfwnNm0J0Jlcm5kIEhlbG1sZSA8YmVybm
 QuaGVsbWxlQG9vcHN3YXJlLmRlPoheBBMRAgAeBQJCDOHWAhsDBgsJCAcDAgMVAgMDFgIBAh4BAhe
 AAAoJEFF1zY6F2UrMWZgAn3MVwDBzRS5dZu1yjx7WU3E3JSheAJ41CUYMIkX/oYfWssUVYnOuedgn
 8LQlQmVybmQgSGVsbWxlIDxiaEBzY2huYWVwcGNoZW5qYWdkLmRlPoheBBMRAgAeBQJCHI+pAhsDB
 gsJCAcDAgMVAgMDFgIBAh4BAheAAAoJEFF1zY6F2UrMYMUAn0rk8fEEY2tHGSUD7PNbk+Fwkd7MAJ
 4wMfOerrvsKTrB9xmFnZ2ZUy67w7QjQmVybmQgSGVsbWxlIDxtYWlsaW5nc0Bvb3Bzd2FyZS5kZT6
 IXgQTEQIAHgUCQhyPxgIbAwYLCQgHAwIDFQIDAxYCAQIeAQIXgAAKCRBRdc2OhdlKzOhrAJ4w+A3H
 cjTJb54lUDpyyQu6Y5dlVQCeJCFKygOVbpn4HSp4Z+R6+s4Rul60J0Jlcm5kIEhlbG1sZSA8YmVyb
 mQuaGVsbWxlQGN5YmVydGVjLmF0Poh4BBMRAgA4FiEEcLVWcrwQIdBqHtvMUXXNjoXZSswFAmDe1D
 kCGwMFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQUXXNjoXZSszs9QCfbbpKT0/Mh4PhfS6rRbm
 NNQC1zS8An2x1PLHPS6649DRkiYOMWTXzAlSE
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.54.3 (3.54.3-1.fc41) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/1f3f264bd40b6eba7380e7da19013417029a93aa.camel%40oopsware.de>
Precedence: bulk

Am Donnerstag, dem 23.01.2025 um 21:36 +0800 schrieb Japin Li:

Thanks for your review again. I am going to work on the other items,
but this one might need further discussion:

> 5.
> Does the following work as expected?
>=20
> postgres=3D# select crypt('hello',
> '$5$$6$rounds=3D10000$/Zg436s2vmTwsoSz');
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 crypt
> -------------------------------------------------
> =C2=A0$5$$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48
> (1 row)
>=20
> postgres=3D# select crypt('hello', '$5$rounds=3D10000$/Zg436s2vmTwsoSz');
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 crypt
> ---------------------------------------------------------------------
> ---------
> =C2=A0$5$rounds=3D10000$/Zg436s2vmTwsoSz$N4Ad0oGzE6ak30z4EGSEXOc2OXQOpmau=
icP
> T3560lY2
> (1 row)
>=20
> According to the documentation, I think the first should output as
> following:
>=20
> $5$rounds=3D5000$TCRu/ts4Npu8OJyeWy2WnUHCe/6bKVMSi0sROUrPh48

So the password hash/salt string used here is broken. '$' is exclusive
to define the boundaries of each part of the password hash string, so
it's very ambiguous what this preample in your example should do.

Note that the code i'm using is nearly identical to what the current
implementation of 'md5' does:

bernd@localhost:bernd :1 #=3D select crypt('hello',
'$1$$6$/Zg436s2vmTwsoSz');
DEBUG:  md5 salt len =3D 0, salt =3D $6$/Zg436s2vmTwsoSz
           crypt           =20
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80
 $1$$/PWPe740uvaQxKzRH.Zxj1
(1 row)

The DEBUG output was added by me. Since the salt len is evaluated to 0,
effectively no salt is handled at all.

So we behave exactly the same way as px_crypt_md5(): It stops after the
first '$' after the magic byte preamble. For shacrypt, this could be
the next '$' after the closing one of the non-mandatory 'rounds'
option, but with your example this doesn't happen since it gets never
parsed. The salt length will be set to 0.

Furthermore, EVP_DigestUpdate() will do nothing if a count parameter
with 0 is handed over, which happens in our case here when adding the
salt. It happily returns 1 (success), so px_update_digest() will never
ERROR out.

Btw., blowfish seems more strict about this: playing around with it a
little i couldn't make it accept a garbaged password hash string like
the current example.

So, I am not sure what to do about this. Originally i had the feeling
we just throw an ERROR if the salt couldn't be evaluated properly, e.g
when we couldn't examine any salt by checking its length being larger
than 0. And '$' is not even a valid character within a salt string. But
then decided to go along with px_crypt_md5().=20

In short: If you throw a garbaged password hash string as the 2nd
argument to crypt() for md5/shacrypt, you currently might get something
obscure back.

I'd like to hear other opinions on this issue.


	Bernd