Re: Modern SHA2- based password hashes for pgcrypto

On Tue, 31 Dec 2024 at 17:06, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> Hi Hackers,
>
> Some of you might already arrived 2025, so first a Happy New Year to
> everyone already there ;)
>
> Please find attached a patch to pgcrypto to add modern SHA-2 based
> password hashes sha256crypt (256 bit) and sha512crypt (512 bit) for
> crypt() and gen_salt() respectively. This is compatible on what crypt()
> currently does on FreeBSD and Linux and both algorithms are considered
> more secure than the currently implemented hashes.
>
> I adapted the code from the publicly available reference implementation
> at [1]. It's based on our existing OpenSSL infrastructure in pgcrypto
> and produces compatible password hashes with crypt() and "openssl
> passwd" with "-5" and "-6" switches.
>
> I documented the new supported hashes for pgcrypto, but didn't do
> anything to update the benchmark table for the supported password
> hashes.
>
> Modern OS (at least Linux, BSDs) implementations for crypt() also
> support yescrypt, which is the recommended (and default) password hash
> algorithm there. I am also looking to implement that, but thought it
> would be useful to have the SHA-2 based hashes first in the review.
>
> I am going to add this patch to the upcoming january commitfest for
> initial review.
>
> [1] https://www.akkadia.org/drepper/SHA-crypt.txt
>

Greate! I have some questions after using it.

When I use the following query, it crashed!

[local]:4012204 postgres=# select crypt('hello', '$5$$6$rounds=10000$/Zg436s2vmTwsoSz');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
: !?>

It is caused by checking the shacrypt digest. The following can fix this crash,
but I'm unsure if it is correct.

@@ -151,8 +152,7 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
type = PGCRYPTO_SHA256CRYPT;
dec_salt_binary += strlen(magic_bytes[0]);
}
-
- if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)
+ else if (strncmp(dec_salt_binary, magic_bytes[1], strlen(magic_bytes[1])) == 0)
{
type = PGCRYPTO_SHA512CRYPT;
dec_salt_binary += strlen(magic_bytes[1]);

Another, if I run crypt with big rounds, it cannot be canceled, for example:

select crypt('hello', '$5$rounds=9999999999$/Zg436s2vmTwsoSz');

Maybe we should add CHECK_FOR_INTERRUPTS() in step 21.

@@ -411,6 +411,7 @@ px_crypt_shacrypt(const char *pw, const char *salt, char *passwd, unsigned dstle
* uses the notation "digest A/B" to describe this behavior.
*/
for (block = 0; block < rounds; block++) {
+ CHECK_FOR_INTERRUPTS();

/* a) start digest B */
px_md_reset(digestB);

--
Regrads,
Japin Li