On Tue, May 23, 2023 at 05:02:50PM -0400, Stephen Frost wrote: > * Jacob Champion (jchamp...@timescale.com) wrote: >> As touched on in past threads, our SCRAM implementation is slightly >> nonstandard and doesn't always protect the entirety of the >> authentication handshake: >> >> - the username in the startup packet is not covered by the SCRAM >> crypto and can be tampered with if channel binding is not in effect, >> or by an attacker holding only the server's key > > Encouraging channel binding when using TLS certainly makes a lot of > sense, particularly in environments where the trust anchors are not > strongly managed (that is- if you trust the huge number of root > certificates that a system may have installed). Perhaps explicitly > encouraging users to *not* trust every root server installed on their > client for their PG connections would also be a good idea. We should > probably add language to that effect to this section.
Currently the user name is not treated by the backend, like that in auth-scram.c: /* * Read username. Note: this is ignored. We use the username from the * startup message instead, still it is kept around if provided as it * proves to be useful for debugging purposes. */ state->client_username = read_attr_value(&p, 'n'); Could it make sense to cross-check that with the contents of the startup package instead, with or without channel binding? >> - low iteration counts accepted by the client make it easier than it >> probably should be for a MITM to brute-force passwords (note that >> PG16's scram_iterations GUC, being server-side, does not mitigate >> this) > > This would be good to improve on. Hmm. Interesting. > > - by default, a SCRAM exchange can be exited by the server prior to > > sending its verifier, skipping the client's server authentication step > > (this is mitigated by requiring channel binding, and PG16 adds > > require_auth=scram-sha-256 to help as well) > > Yeah, encouraging this would also be good and should be mentioned as > something to do when one is using SCRAM. Clearly that would go into a > PG16 version of this. Agreed to improve the docs about ways to mitigate any risks. -- Michael
signature.asc
Description: PGP signature