> On 24 May 2026, at 22:04, Sehrope Sarkuni <[email protected]> wrote: > > Hi, > > Attached are some SCRAM-related patches.
Thanks for looking, a few initial comments before looking further on these. > The first patch fixes parsing of SCRAM iteration counts in both the backend > SCRAM verifier parser and libpq's server-first-message parser. Previously > both paths parsed into a long and then stored the result in an int, so values > in (INT_MAX, LONG_MAX] could be accepted by strtol() and then narrowed > incorrectly. I don't think this allows for any invalid logins as the password > verifier would have tried to verify a different iteration count, but it's > still wrong. The iteration count parsing you refer to is extracting the iterations from the stored password, it's not parsing user input in any way. The iteration count in set in the password using scram_sha_256_iterations, which in turn can be set by the GUC scram_iterations which is limited to 1..INT_MAX (before being accessible as a GUC it was hardcoded to 4096). strtol() to an int without checking if the parsed value exceeds INT_MAX isn't good code hygiene, and we should fix that, but it cannot in practice cause truncation AFAICS. Can you show a sequence of generating a SCRAM secret which has an iteration count outside of 0..INT_MAX, or one which doesn't correspond to the setting in scram_iterations? > Patches 0003 through 0005 address a separate client-side resource issue. In a > SCRAM exchange, the PBKDF2 iteration count is supplied by the server. A > hostile or misconfigured server can advertise a very large iteration count > and keep a blocking libpq connection inside scram_SaltedPassword() beyond the > caller's connect_timeout. The backend has CHECK_FOR_INTERRUPTS() inside the > loop, but the frontend previously had no equivalent. A server which request the iteration count of the stored secret, whatever it is, cannot be considered misconfigured or hostile. A server which sends an incorrect iteration count in order to reject a connection after causing the client to perform CPU work is hostile (but I'd be quite glad if a hostile server rejected my connection rather than tricking me into logging in and stealing/corrupting data). Making the SCRAM calculation honor the connection timeout sounds like a good way to address the latter. > These patches mirror the blocking connection attempt's deadline into PGconn, > add an optional interrupt callback to the common SCRAM PBKDF2 helper, and > have libpq use that callback to abort once the in-flight bl:qaocking > connection deadline has expired. The test modifies a SCRAM verifier to > advertise a very large iteration count and verifies that connect_timeout > interrupts the PBKDF2 loop. > > This protection applies to the blocking connection path, such as > PQconnectdb(). It does not make connect_timeout apply automatically to > applications driving PQconnectPoll() themselves. > > I considered passing the connection deadline directly into the SCRAM PBKDF2 > helper, but used an interrupt callback instead. That keeps the common SCRAM > code independent of libpq's timeout representation and allows the same > mechanism to support other frontend abort conditions, such as cancellation of > an in-progress connection attempt. Making sure SCRAM secret calculation doesn't exceed connection_timeout sounds like a good idea. However, ode shared between frontend and backend should IMHO not have parameters which only work in the frontend. I'm also not convinced that it's a good idea to add a callback which in your case gets the entire PGconn object. I'd prefer a check more like the CFI we have in the backend code. Another issue is the interval for checking. One point of allowing configurable iteration counts was to allow constrained low-power IOT platforms to use SCRAM by using a much lower iteration count. Only checking for timeout every 4096 iterations might be well past the connection timeout. Perhaps the interval of checking should be a function of the iterations sent by the server? > The attached patch currently uses a default cap of 100K. That is well above > PostgreSQL's normal SCRAM iteration count (4K), but it is still a client-side > behavior change for installations using unusually high SCRAM iteration > counts. For reference, we added a similar patch to pgjdbc with the same 100K > default. I am off the cuff -1 on adding more connection parameters for niche usecases like this (we already have many enough to make it confusing), it will be very hard to document what an appropriate setting is, and getting it wrong might mean rejecting connections in err. -- Daniel Gustafsson
