Re: [PATCH] pgcrypto: Fix sha-crypt rounds= values that do not fit in int (BUG #19743)

Hi Prashant

On 2026/10/23 23:30, Prashant Kumar wrote:
> Hi hackers,
>
> Attached is a patch for BUG #19743, reported by Ke Han [1]:
> pgcrypto's crypt() parses the rounds= option of a sha256crypt /
> sha512crypt salt with strtoint() and never checks errno, so a
> value that does not fit in an int is silently reduced modulo
> 2^32 before the supported-range clamping runs. For example,
> rounds=4294968296 produces a hash computed with the minimum of
> 1000 rounds and no NOTICE at all, and rounds=4294967295 is
> reported as "rounds=-1".
>
> I reproduced this on current master (20devel) before fixing it.
>
> The patch checks errno around the strtoint() call in
> px_crypt_shacrypt(). On ERANGE the value is out of range in
> the direction of its sign, so it is clamped to
> PX_SHACRYPT_ROUNDS_MIN or PX_SHACRYPT_ROUNDS_MAX, matching the
> existing clamp-with-NOTICE behaviour for in-range values, and
> the NOTICEs now report the value as supplied instead of the
> truncated conversion result.

Thank you for the patch. In practice, most people would use gen_salt()
to generate the salt. But directly passing the raw salt strings is also
valid, so it needs some fixing.

How about using strtol or strtoi64 instead of strtoint to handle the
rounds value that is out of the integer bounds? I attached my patch
using strtoi64. We can handle the out-of-bound positive and negative
value by taking advantage of current px_crypt_shacrypt logic. We can
omit the check if the round values is positive or negative. I use
strtoi64 so that we can cope with the platform difference. (Windows
treats long type as 4 bytes.) What do you think?

> + rounds_overflow = (errno == ERANGE);

Most of the similar postgresql code is just like "if (errno == ERANGE)"
instead of using another variable.

Regarding errno, I may be wrong but I'm thinking we can omit this check
by using strtoi64 (strtol) since underflow or overflow value will change
to LONG_MIN or LONG_MAX and fall into the current bound check logic.

This is another point. I noticed the reference implementation[*] assumes
round value is only positive since it just uses strtoul though there is
no value check if it's negative or not. We might be OK to turn negative
rounds value to error since negative rounds value does not make any sense.

[*] https://www.akkadia.org/drepper/SHA-crypt.txt

Regards,
Takeshi Ideriha