Re: [PATCH] Add CHECK_FOR_INTERRUPTS in scram_SaltedPassword loop.

> On 25 Nov 2023, at 02:20, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Nov 23, 2023 at 11:19:51AM +0300, Aleksander Alekseev wrote:
>>>> I don't think it would be useful to limit this at an arbitrary point, iteration
>>>> count can be set per password and if someone wants a specific password to be
>>>> super-hard to brute force then why should we limit that?
>>>
>>> I agree with that. Maybe some users do want a super-hard password.
>>> RFC 7677 and RFC 5802 don't specify the maximum number of iterations.
>>
>> That's a fairly good point. However we are not obligated not to
>> implement everything that is missing in RFC. Also in fact we already
>> limit the number of iterations to INT_MAX.
>
> INT_MAX, as in the limit that we have for integer GUCs and the
> routines building the hashed entry, so the Postgres internals are what
> defines the limit here. I doubt that we'll see cases where somebody
> would want more than that, but who knows in 10/20 years.
>
>> If we decide to limit this number even further the actual problem is
>> to figure out what the new practical limit would be. Regardless of the
>> chosen number there is a possibility of breaking backward
>> compatibility for certain users.
>
> No idea what the limit should be if it were to be lowered down, but
> I suspect that even a new lower limit could be an issue for hosts in
> the low-end specs when it comes to DOS. It's not like there are no
> ways to eat CPU when you are already logged in.

The whole point of this GUC (and the iteration count construct in the spec) is
to allow hardened setups to make brute forcing passwords as hard as they choose
them to be, setting an upper limit (apart from the INT_MAX implementation
detail) where one isn't even mentioned in the RFC makes little sense when the
loop can be canceled.

On the flip side, setups which have low end clients can choose to reduce it
from the default to make scram an option at all where it previously was too
expensive and less secure schemes had to be used.

>> For this reason I believe merging the proposed patch would be the
>> right move at this point. It doesn't make anything worse for the
>> existing users and solves a potential problem for some of them.
>
> Yeah, agreed. Being stuck on a potential large tight loops is
> something we tend to avoid in the backend, so I agree that this is a
> thing to keep in the backend especially because we have
> scram_iterations and that it is user-settable.
>
> I think that we should backpatch that down to v16 at least where the
> GUC has been introduced as it's more like a nuisance if one sets the
> GUC to an incorrect value, and I'd like to apply the patch this way.
> Any objections or comments regarding that?

I don't see any reason to backpatch further down than 16 given how low the
hardcoded value is set there, scanning the archives I see no complaints about
it either. As a reference, CREATE ROLE using 4096 iterations takes 14ms on my
10 year old laptop (1M iterations, 244x the default, takes less than a second).

--
Daniel Gustafsson