Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.3\))
Subject: Re: [PATCH] Add CHECK_FOR_INTERRUPTS in scram_SaltedPassword loop.
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <ZWFLwCjp8-DvBJ7n@paquier.xyz>
Date: Mon, 27 Nov 2023 10:05:49 +0100
Cc: Aleksander Alekseev <aleksander@timescale.com>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 Bowen Shi <zxwsbg12138@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Message-Id: <137DA8EC-397D-4DF8-A573-4A18D80DC0A1@yesql.se>
References: <CAM_vCueV6xfr08KczfaCEk5J_qeTZtgqN7+orkNLx=g+phE82Q@mail.gmail.com>
 <CAJ7c6TMdDdC5f1qmS=UVx22Bvjqz=PhY=aeefwoXxGc_bK+WKQ@mail.gmail.com>
 <C4DE43E8-AEC6-4ED2-8520-01C490F751AC@yesql.se>
 <1674901.1700665447@sss.pgh.pa.us>
 <CAM_vCufzLucPevjOC90cVD_AsPtpPHRLY2w+Zm985B-6JHxz1w@mail.gmail.com>
 <CAJ7c6TMLeqc6MR76LaJCk3jeBnnX=tuVUE=yv9+k7tkCb-OeYg@mail.gmail.com>
 <ZWFLwCjp8-DvBJ7n@paquier.xyz>
To: Michael Paquier <michael@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/137DA8EC-397D-4DF8-A573-4A18D80DC0A1%40yesql.se>
Precedence: bulk

> On 25 Nov 2023, at 02:20, Michael Paquier <michael@paquier.xyz> wrote:
>=20
> 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?
>>>=20
>>> 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.
>>=20
>> 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.
>=20
> 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.
>=20
>> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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