Re: Modern SHA2- based password hashes for pgcrypto

Am Freitag, dem 03.01.2025 um 23:57 +0800 schrieb Japin Li:
>
> 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.
>

Hmm, can you provide a backtrace? I am currently debugging the CI
results and i currently have a thinko in my code at crypt-sha.c:530, i
am using the wrong length to copy the result buffer, see

https://api.cirrus-ci.com/v1/artifact/task/6395274957946880/log/contrib/pgcrypto/log/postmaster.log

==33750==ERROR: AddressSanitizer: stack-buffer-overflow on address
0x7ffc385204c7 at pc 0x7fd65a24814b bp 0x7ffc38520240 sp 0x7ffc3851f9f0
READ of size 128 at 0x7ffc385204c7 thread T0
#0 0x7fd65a24814a in __interceptor_memcpy
../../../../src/libsanitizer/sanitizer_common/sanitizer_common_intercep
tors.inc:827
#1 0x7fd64c37c25f in px_crypt_shacrypt /tmp/cirrus-ci-
build/contrib/pgcrypto/crypt-sha.c:530
#2 0x7fd64c3993f0 in run_crypt_sha /tmp/cirrus-ci-
build/contrib/pgcrypto/px-crypt.c:76

But i cannot reproduce your crash in this case, maybe this is related
to the above issue...

> @@ -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]);
>

Yeah, seems the check is not strict enough for the magic byte and i
need to be more strict here. I am going to look into this. "$5$$6$"
certainly is bogus enough that we should error out instead.

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

Hmm not sure how expensive it is to check for interrupts for each
block. Maybe its better to check for each 10.000 blocks or so. But i
like the idea. Will investigate.

Thanks for your review,

Bernd