Fix infinite loop from setting scram_iterations to INT_MAX
Hi,
I stumbled upon a problem with the scram_iterations GUC where setting
scram_iterations to INT_MAX and then creating a user causes the command to
hang indefinitely.
postgres=# SET scram_iterations=2147483647;
SET
postgres=# CREATE ROLE maxscram WITH PASSWORD 'forever';
<hangs>
I looked into the relevant code and found the issue. Each SCRAM iteration
after the first is done in a loop with the following condition:
int i;
...
for (i = 2; i <= iterations; i++)
{
...
}
For iterations = INT_MAX, the loop will never terminate since the condition
is <= and adding 1 to INT_MAX will lead to i wrapping around to INT_MIN.
I've fixed this by modifying the loop condition to be i < iterations. I've
attached a patch with the fix. I considered adding a test as well, but
since generating a password with a high number of iterations is very
time-consuming, I'm not sure if that would be practical.
I also tried adding this to the current CommitFest, but my account hasn't
passed the cooldown period yet.
Thanks,
Kevin
Attachments:
fix_max_scram_iterations.patchapplication/octet-stream; name=fix_max_scram_iterations.patchDownload
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index 400900c51c5..bfd2a340016 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -74,7 +74,7 @@ scram_SaltedPassword(const char *password,
memcpy(result, Ui_prev, key_length);
/* Subsequent iterations */
- for (i = 2; i <= iterations; i++)
+ for (i = 1; i < iterations; i++)
{
#ifndef FRONTEND
/*
On Sun, Mar 23, 2025 at 10:41 PM Kevin K Biju <kevinkbiju@gmail.com> wrote:
int i;
...
for (i = 2; i <= iterations; i++)
{
...
}For iterations = INT_MAX, the loop will never terminate since the condition is <= and adding 1 to INT_MAX will lead to i wrapping around to INT_MIN.
I've fixed this by modifying the loop condition to be i < iterations. I've attached a patch with the fix. I considered adding a test as well, but since generating a password with a high number of iterations is very time-consuming, I'm not sure if that would be practical.
Nice catch. The fix looks good to me. It seems to me that it's fine
to go without a test case, since the fix is quite straightforward.
Thanks
Richard
On Mon, Mar 24, 2025 at 09:50:36AM +0900, Richard Guo wrote:
Nice catch. The fix looks good to me. It seems to me that it's fine
to go without a test case, since the fix is quite straightforward.
One could argue about using an injection point to force trick the
iteration loop to be cheaper, but I could not really get excited about
the coverage vs the cycles spent for it..
--
Michael
On Mon, Mar 24, 2025 at 9:54 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Mar 24, 2025 at 09:50:36AM +0900, Richard Guo wrote:
Nice catch. The fix looks good to me. It seems to me that it's fine
to go without a test case, since the fix is quite straightforward.One could argue about using an injection point to force trick the
iteration loop to be cheaper, but I could not really get excited about
the coverage vs the cycles spent for it..
Exactly.
Thanks
Richard
On Mon, Mar 24, 2025 at 9:59 AM Richard Guo <guofenglinux@gmail.com> wrote:
On Mon, Mar 24, 2025 at 9:54 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Mar 24, 2025 at 09:50:36AM +0900, Richard Guo wrote:
Nice catch. The fix looks good to me. It seems to me that it's fine
to go without a test case, since the fix is quite straightforward.
One could argue about using an injection point to force trick the
iteration loop to be cheaper, but I could not really get excited about
the coverage vs the cycles spent for it..
Exactly.
I plan to go ahead and push Kevin's fix, barring any objections.
Thanks
Richard
On 26 Mar 2025, at 07:41, Richard Guo <guofenglinux@gmail.com> wrote:
I plan to go ahead and push Kevin's fix, barring any objections.
Thanks for picking this up, I had missed this thread.
--
Daniel Gustafsson