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
