On Sat, May 27, 2017 at 5:59 PM, Álvaro Hernández Tortosa <a...@8kdata.com> wrote: > On 25/05/17 17:17, Michael Paquier wrote: >> Please find attached a patch to add support for channel binding for >> SCRAM, to mitigate MITM attacks using this protocol. Per RFC5802 >> (https://tools.ietf.org/html/rfc5802), servers supporting channel >> binding need to add support for tls-unique, and this is what this >> patch does. > > This is awesome, Michael :) Thank you! You have prevented me eventually > writing the patch and having then PostgreSQL source tainted with Java-to-C > "transpiled" code ^_^
The thing would have been reviewed and rewritten a couple of times :) So that would have unlikely (hopefully) finished in the shape of a Java-C-like patch. > After a deeper analysis, I have some concerns/comments here: > > - tls-unique, as you mentioned, uses two undocumented APIs. This raises a > small flag about the stability and future of those APIs. Those are 5 lines of code each in OpenSSL, not that hard to maintain. Those APIs are clunky by the way as there is no way to know in advance the length of the message for memory allocation. > - More importantly, some drivers (not libpq-based) may have unexpected > difficulties implementing tls-unique. In particular, there is not an API in > Java to get the finished message. I expect other languages to maybe have > similar limitations. For Java I see two options: > * Also implement tls-server-end-point, which rather relies on the server > certificate. This information seems to be exposed as part of the Java APIs: > https://docs.oracle.com/javase/8/docs/api/java/security/cert/Certificate.html#getEncoded-- > * Use the BouncyCastle library (http://bouncycastle.org/), which > explicitly supports tls-unique > (https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/est/TLSUniqueProvider.html#getTLSUnique() > , > https://www.bouncycastle.org/docs/tlsdocs1.5on/org/bouncycastle/tls/ChannelBinding.html#tls_unique > ). This would require, however, non-trivial changes to the driver and, I > expect, a lot of extra effort. I am wondering how other languages are regarding that. Python has added a method in 2011: https://hg.python.org/cpython/rev/cb44fef5ea1d > - It seems to me that tls-unique might be a bit fragile. In particular, it > requires the server to be aware of TSL renegotiations and avoid performing > one while the SCRAM authentication is being performed. I don't know if this > is already guaranteed by the current patch, but it seems to me it requires > complex interactions between levels of abstraction that are quite separate > (SSL and SCRAM). This is explained by the RFC: RFC 5802, section 6 gives a good level of details on the matter: https://tools.ietf.org/html/rfc5802#section-6 There is indeed interaction between SSL and scram, and the patch makes use of USE_SSL for this purpose. > " > server applications MUST NOT request TLS renegotiation during phases of the > application protocol during which application-layer authentication occurs > " > (https://tools.ietf.org/html/rfc5929#section-3.1) The SSL hanshake happens only once at connection obtention, before the authentication exchange happens. So there is no renegociation. SSL renegotiation has been removed in PG anyway, disabled on back-branches, and removed as well in TLS 1.3 (right? I don't recall the spec in details). > Based on this facts, I'd suggest to either implement > tls-server-end-point or implement both tls-server-end-point and tls-unique. > The latter would require a more complete protocol message to advertise > separately SCRAM mechanisms and channel binding names. One of such > structures could be the one I suggested earlier: > https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638...@8kdata.com The RFC says that any server implementing channel binding must implement tls-unique, so having only end-point is not compliant. >> Before even that, the server needs to send to the client the list of >> SASL mechanisms that are supported. This adds SCRAM-SHA-256-PLUS if >> the server has been built with SSL support to the list. > > And I'd say the list of channel binding names supported. It seems to me as well that this gives us an indication that it should be the default channel binding used, or if the exchange list has no channel names included, the client can assume that tls-unique will be used. Such an approach has as well the merit to keep the patch simple. In short, I would advocate an incremental approach that adds tls-unique first. This has value anyway as there is no need for certificate configuration. This also does not require a message format extension. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
