Re: SCRAM with channel binding downgrade attack
От | Michael Paquier |
---|---|
Тема | Re: SCRAM with channel binding downgrade attack |
Дата | |
Msg-id | 20180605064138.GC5840@paquier.xyz обсуждение исходный текст |
Ответ на | Re: SCRAM with channel binding downgrade attack (Heikki Linnakangas <hlinnaka@iki.fi>) |
Ответы |
Re: SCRAM with channel binding downgrade attack
(Peter Eisentraut <peter.eisentraut@2ndquadrant.com>)
Re: SCRAM with channel binding downgrade attack (Heikki Linnakangas <hlinnaka@iki.fi>) |
Список | pgsql-hackers |
On Sat, Jun 02, 2018 at 01:08:56PM -0400, Heikki Linnakangas wrote: > On 28/05/18 15:08, Michael Paquier wrote: >> On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote: >> > Sounds good. >> >> Okay. Done this way as attached. If the backend forces anything else >> than SCRAM then the client gets an immediate error if channel binding is >> required, without waiting for the password prompt. > > Thanks! The comments and error messages need some copy-editing: Thanks Heikki for the input. I have reworked all the points you raised in the attached. >> /* >> * Select the mechanism to use. Pick SCRAM-SHA-256-PLUS over anything >> * else if a channel binding mode is not 'disable'. Pick SCRAM-SHA-256 >> * if nothing else has already been picked. If we add more mechanisms, a >> * more refined priority mechanism might become necessary. >> */ > > "else if a channel binding mode is not 'disable'" sounds a bit awkward. A > double negative. (Also, "a" -> "the") Okay. I have completely rewritten this block. Hopefully that's clearer. >> + /* >> + * If client is willing to enforce the use the channel binding but >> + * it has not been previously selected, because it was either not >> + * published by the server or could not be selected, then complain >> + * back. If SSL is not used for this connection, still complain >> + * similarly, as the client may want channel binding but forgot >> + * to set it up to do so which could be the case with sslmode=prefer >> + * for example. This protects from any kind of downgrade attacks >> + * from rogue servers attempting to bypass channel binding. >> + */ > > Instead of "is willing to enforce the use the channel binding", perhaps > simply "requires channel binding". Check. >> + printfPQExpBuffer(&conn->errorMessage, >> + libpq_gettext("channel binding required for SASL authentication but no valid mechanism could be selected\n")); > > Channel binding is a property of SCRAM authentication specifically, it's not > applicable to other SASL mechanisms. So I'd suggest something like: > > "server does not support channel binding, and channel_binding_mode=require > was used" Changed as such, except that this is using scram_channel_binding_mode in the error message. >> + /* >> + * Complain if channel binding mechanism is chosen but no channel >> + * binding type is defined. >> + */ >> + if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0 && >> + conn->scram_channel_binding_type == NULL) >> + { >> + printfPQExpBuffer(&conn->errorMessage, >> + libpq_gettext("SASL authentication mechanism using channel binding supported but no channel binding type defined\n")); >> + goto error; >> + } > > It's not immediately clear to me from the error message or the comment, when > this error is thrown. Can this even happen? No, let's not make this happen then. I have added NULL handling in connectOptions2 related to the initialization of the options so both scram_channel_binding_mode and scram_channel_binding_type will never be NULL like sslmode. >> + /* >> + * Before processing any message, perform security-related sanity >> + * checks. If the client is willing to perform channel binding with >> + * SCRAM authentication, only a subset of messages can be used so >> + * as there is no transmission of any password data or such. >> + */ > > I'd suggest something like: > > "If SCRAM with channel binding is required, refuse all other authentication > methods. Requiring channel binding implies that the client doesn't trust the > server, until the SCRAM authentication is completed, so it's important that > we don't divulge the plaintext password, or perform some weaker > authentication, even if the server asks for it. In all other authentication > methods, it's currently assumed that the client trusts the server, either > implicitly or because the SSL certificate was already verified during the > SSL handshake." Check. Thanks for the suggestion. >> + printfPQExpBuffer(&conn->errorMessage, >> + libpq_gettext("channel binding required for authentication but invalid protocol used\n")); > > If I understand correctly, you get this error, e.g. if you have "password" > or "sspi" in pg_hba.conf, and the client uses > "channel_binding_mode=require". "Invalid protocol" doesn't sound right for > that. Perhaps "channel binding required, but the server requested %s > authentication". Right, even "md5" enters in this category if the user has a MD5 verifier, but not if it has a SCRAM verifier. I have done that with get_auth_request_str(), which maps authentication requests to the probable hba entry. It feels like cheating to map "trust" to AUTH_REQ_OK as all methods use it as final message though, even if it is not triggered by this patch. So I have used no mapping name for it. > Is it possible to have an error hint here? Perhaps add > "HINT: change the authentication method to scram-sha-256 in the server's > pg_hba.conf file". Hm. A HINT would require something similar to PQresultErrorField for error hints, which is a fight I not much willing to do just for this patch. Spawning a new error message with a newline would also be considered as a new error message. So I discard this one. I have also added a test with a "password" server which stresses this code path actually, and I just indented the patch. What do you think? -- Michael
Вложения
В списке pgsql-hackers по дате отправления: