Thread: Re: Modern SHA2- based password hashes for pgcrypto

On Tue, 31 Dec 2024 at 17:06, Bernd Helmle <mailings@oopsware.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