On Tue, May 23, 2023 at 05:02:50PM -0400, Stephen Frost wrote:
> * Jacob Champion (jchampion@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