Horton

Thread

Commits

  1. Remove support for tls-unique channel binding.

  1. Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-11T09:27:33Z

    Currently, there is no negotiation of the channel binding type between 
client and server. The server advertises that it supports channel 
binding, or not, and the client decides what channel binding to use. If 
the server doesn't support the binding type that the client chose, 
authentication will fail.

Based on recent discussions, it looks like there's going to be 
differences in this area [1]. OpenSSL can support both tls-unique and 
tls-server-end-point. Java only supports tls-server-end-point, while 
GnuTLS only supports tls-unique. And Mac OS Secure Transports supports 
neither one. Furthermore, it's not clear how TLS v1.3 affects this. 
tls-unique might no longer be available in TLS v1.3, but we might get 
new channel binding types to replace it. So this is about to get really 
messy, if there is no way to negotiate. (Yes, it's going to be messy 
even with negotiation.)

I think we must fix that before we release v11, because this affects the 
protocol. There needs to be a way for the server to advertise what 
channel binding types it supports.

I propose that the server list each supported channel binding type as a 
separate SASL mechanism. For example, where currently the server lists:

SCRAM-SHA-256-PLUS
SCRAM-SHA-256

as the supported mechanisms, we change that into:

SCRAM-SHA-256-PLUS tls-unique
SCRAM-SHA-256-PLUS tls-server-end-point
SCRAM-SHA-256

Thoughts? Unfortunately this breaks compatibility with current v11beta 
clients, but IMHO we must fix this. If we want to alleviate that, and 
save a few bytes of network traffic, we could decide that plain 
"SCRAM-SHA-256-PLUS" implies tls-unique, so the list would be:

SCRAM-SHA-256-PLUS
SCRAM-SHA-256-PLUS tls-server-end-point
SCRAM-SHA-256

That would be mostly compatible with current libpq clients.

[1] 
https://www.postgresql.org/message-id/flat/20180122072936.GB1772%40paquier.xyz

- Heikki

  2. Re: Negotiating the SCRAM channel binding type

    Bruce Momjian <bruce@momjian.us> — 2018-07-11T10:31:45Z

    On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:
> Currently, there is no negotiation of the channel binding type between
> client and server. The server advertises that it supports channel binding,
> or not, and the client decides what channel binding to use. If the server
> doesn't support the binding type that the client chose, authentication will
> fail.
> 
> Based on recent discussions, it looks like there's going to be differences
> in this area [1]. OpenSSL can support both tls-unique and
> tls-server-end-point. Java only supports tls-server-end-point, while GnuTLS
> only supports tls-unique. And Mac OS Secure Transports supports neither one.
> Furthermore, it's not clear how TLS v1.3 affects this. tls-unique might no
> longer be available in TLS v1.3, but we might get new channel binding types
> to replace it. So this is about to get really messy, if there is no way to
> negotiate. (Yes, it's going to be messy even with negotiation.)
> 
> I think we must fix that before we release v11, because this affects the
> protocol. There needs to be a way for the server to advertise what channel
> binding types it supports.

Yes, this does make sense.  From a security perspective, it doesn't
matter which channel binding method we use, assuming there are no
unfixed exploits.  The difference between the channel binding methods is
explained in our PG 11 docs:

	https://www.postgresql.org/docs/11/static/sasl-authentication.html#SASL-SCRAM-SHA-256

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  3. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-11T11:37:20Z

    On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:
> as the supported mechanisms, we change that into:
> 
> SCRAM-SHA-256-PLUS tls-unique
> SCRAM-SHA-256-PLUS tls-server-end-point
> SCRAM-SHA-256

Can we say for sure that there won't be other options associated to a
given SASL mechanism?  It seems to me that something like the following
is better long-term, with channel binding available as a comma-separated
list of items:

SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point
SCRAM-SHA-256

> Thoughts? Unfortunately this breaks compatibility with current v11beta
> clients, but IMHO we must fix this. If we want to alleviate that, and save a
> few bytes of network traffic, we could decide that plain
> "SCRAM-SHA-256-PLUS" implies tls-unique, so the list would be:
> 
> SCRAM-SHA-256-PLUS
> SCRAM-SHA-256-PLUS tls-server-end-point
> SCRAM-SHA-256
> 
> That would be mostly compatible with current libpq clients.

I am not sure it is worth caring about the compatibility at a beta2
stage, as v11 is not released yet.  Still I agree that not specifying
anything should mean the default, which is per the RFCs currently
existing tls-unique.

Another piece of the puzzle is here as well:
https://www.postgresql.org/message-id/flat/20180122072936.GB1772@paquier.xyz
We need to allow a given SSL implementation to specify what the list of
channel bindings it supports is.
--
Michael

  4. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-11T12:01:03Z

    On 11/07/18 14:37, Michael Paquier wrote:
> On Wed, Jul 11, 2018 at 12:27:33PM +0300, Heikki Linnakangas wrote:
>> as the supported mechanisms, we change that into:
>>
>> SCRAM-SHA-256-PLUS tls-unique
>> SCRAM-SHA-256-PLUS tls-server-end-point
>> SCRAM-SHA-256
> 
> Can we say for sure that there won't be other options associated to a
> given SASL mechanism?  It seems to me that something like the following
> is better long-term, with channel binding available as a comma-separated
> list of items:
> 
> SCRAM-SHA-256-PLUS channel_bindings=tls-unique,tls-server-end-point
> SCRAM-SHA-256

That would be more complicated to parse. Yeah, we might need further 
options for some SASL mechanisms in the future, but we can cross that 
bridge when we get there. I don't see any need to complicate this case 
for that.

I started digging into this more closely, and ran into a little problem. 
If channel binding is not used, the client sends a flag to the server to 
indicate if it's because the client doesn't support channel binding, or 
because it thought that the server doesn't support it. This is the 
gs2-cbind-flag. How should the flag be set, if the server supports 
channel binding type A, while client supports only type B? The purpose 
of the flag is to detect downgrade attacks, where a man-in-the-middle 
removes the PLUS variants from the list of supported mechanisms. If we 
treat incompatible channel binding types as "client doesn't support 
channel binding", then the downgrade attack becomes possible (the 
attacker can replace the legit PLUS variants with bogus channel binding 
types that the client surely doesn't support). If we treat it as "server 
doesn't support channel binding", then we won't downgrade to the 
non-channel-binding variant, in the legitimate case that the client and 
server both support channel binding, but with incompatible types.

What we'd really want, is to include the full list of server's supported 
mechanisms as part of the exchange, not just a boolean "y/n" flag. But 
that's not what the spec says :-(.

I guess we should err on the side of caution, and fail the 
authentication in that case. That's unfortunate, but it's better than 
not negotiating at all. At least with the negotiation, the 
authentication will work if there is any mutually supported channel 
binding type.

- Heikki

  5. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-11T13:00:47Z

    On 11/07/18 12:27, Heikki Linnakangas wrote:
> Based on recent discussions, it looks like there's going to be
> differences in this area [1]. OpenSSL can support both tls-unique and
> tls-server-end-point. Java only supports tls-server-end-point, while
> GnuTLS only supports tls-unique. And Mac OS Secure Transports supports
> neither one. Furthermore, it's not clear how TLS v1.3 affects this.
> tls-unique might no longer be available in TLS v1.3, but we might get
> new channel binding types to replace it. So this is about to get really
> messy, if there is no way to negotiate. (Yes, it's going to be messy
> even with negotiation.)

I've been reading up on the discussions on GnuTLS and Secure Transport, 
as well as the specs for tls-server-end-point.

In a nutshell, to get the token for tls-server-end-point, you need to 
get the peer's certificate from the TLS library, in raw DER format, and 
calculate a hash over it. The hash algorithm depends on the 
signatureAlgorithm in the certificate, so you need to parse the 
certificate to extract that. We don't want to re-implement X509 parsing, 
so realistically we need the TLS library to have support functions for that.

Looking at the GnuTLS docs, I believe it has everything we need. 
gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be 
used to get the certificate, and 
gnutls_x509_crt_get_signature_algorithm() gets the signatureAlgorithm.

The macOS Secure Transport documentation is a bit harder to understand, 
but I think it has everything we need as well. 
SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData() 
functions get you the certificate in DER format. You can get the 
signature algorithm with SecCertificateCopyValues(), with the right 
constants.

Am I missing something? I think we can support tls-server-end-point with 
all TLS implementations we might care about.

- Heikki

  6. Re: Negotiating the SCRAM channel binding type

    Bruce Momjian <bruce@momjian.us> — 2018-07-11T14:23:07Z

    On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote:
> In a nutshell, to get the token for tls-server-end-point, you need to get
> the peer's certificate from the TLS library, in raw DER format, and
> calculate a hash over it. The hash algorithm depends on the
> signatureAlgorithm in the certificate, so you need to parse the certificate
> to extract that. We don't want to re-implement X509 parsing, so
> realistically we need the TLS library to have support functions for that.
> 
> Looking at the GnuTLS docs, I believe it has everything we need.
> gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used
> to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets
> the signatureAlgorithm.
> 
> The macOS Secure Transport documentation is a bit harder to understand, but
> I think it has everything we need as well.
> SSLCopyPeerTrust()+SecTrustGetCertificateAtIndex()+SecCertificateCopyData()
> functions get you the certificate in DER format. You can get the signature
> algorithm with SecCertificateCopyValues(), with the right constants.
> 
> Am I missing something? I think we can support tls-server-end-point with all
> TLS implementations we might care about.

That seems right to me.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  7. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-12T04:14:10Z

    On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:
> That would be more complicated to parse. Yeah, we might need further options
> for some SASL mechanisms in the future, but we can cross that bridge when we
> get there. I don't see any need to complicate this case for that.

Okay, fine for me.

> I started digging into this more closely, and ran into a little problem. If
> channel binding is not used, the client sends a flag to the server to
> indicate if it's because the client doesn't support channel binding, or
> because it thought that the server doesn't support it. This is the
> gs2-cbind-flag. How should the flag be set, if the server supports channel
> binding type A, while client supports only type B? The purpose of the flag
> is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
> variants from the list of supported mechanisms. If we treat incompatible
> channel binding types as "client doesn't support channel binding", then the
> downgrade attack becomes possible (the attacker can replace the legit PLUS
> variants with bogus channel binding types that the client surely doesn't
> support). If we treat it as "server doesn't support channel binding", then
> we won't downgrade to the non-channel-binding variant, in the legitimate
> case that the client and server both support channel binding, but with
> incompatible types.
> 
> What we'd really want, is to include the full list of server's supported
> mechanisms as part of the exchange, not just a boolean "y/n" flag. But
> that's not what the spec says :-(.

Let's not break the spec :)  I understand from it that the client is in
charge of the choice, so we are rather free to choose the reaction the
client should have.  In the second phase of the exchange, the client
communicates back to the server the channel binding it has decided to
choose, it is not up to the server to pick up one if the client thinks
that it can use multiple ones.

> I guess we should err on the side of caution, and fail the authentication in
> that case. That's unfortunate, but it's better than not negotiating at all.
> At least with the negotiation, the authentication will work if there is any
> mutually supported channel binding type.

I think that in this case the client should throw an error
unconditionally if it wants to use channel A but server supports only B.
It is always possible for the client to adjust the channel binding type
it wants to use by using the connection parameter scram_channel_binding,
or to recompile.  If there is incompatibility between channel binding
types, it would actually mean that the client and the server are not
compiled with the same SSL implementation, would that actually work? Say
a server has only tls-unique with gnu's library and the client uses JDBC
which only has tls-server-end-point..

So, to keep things simple, it seems to me that we should just make the
server send the list, and then check at client-level if the list sent by
server includes what the client wants to use, complaining if the option
is not present.
--
Michael

  8. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-12T08:26:30Z

    On 12/07/18 07:14, Michael Paquier wrote:
> On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:
>> I started digging into this more closely, and ran into a little problem. If
>> channel binding is not used, the client sends a flag to the server to
>> indicate if it's because the client doesn't support channel binding, or
>> because it thought that the server doesn't support it. This is the
>> gs2-cbind-flag. How should the flag be set, if the server supports channel
>> binding type A, while client supports only type B? The purpose of the flag
>> is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
>> variants from the list of supported mechanisms. If we treat incompatible
>> channel binding types as "client doesn't support channel binding", then the
>> downgrade attack becomes possible (the attacker can replace the legit PLUS
>> variants with bogus channel binding types that the client surely doesn't
>> support). If we treat it as "server doesn't support channel binding", then
>> we won't downgrade to the non-channel-binding variant, in the legitimate
>> case that the client and server both support channel binding, but with
>> incompatible types.
>>
>> What we'd really want, is to include the full list of server's supported
>> mechanisms as part of the exchange, not just a boolean "y/n" flag. But
>> that's not what the spec says :-(.
> 
> Let's not break the spec :)  I understand from it that the client is in
> charge of the choice, so we are rather free to choose the reaction the
> client should have.  In the second phase of the exchange, the client
> communicates back to the server the channel binding it has decided to
> choose, it is not up to the server to pick up one if the client thinks
> that it can use multiple ones.

The case where the client and the server both support the same channel 
binding type, we're OK. The problematic case is when e.g. the server 
only supports tls-unique and the client only supports 
tls-server-end-point. What we would (usually) like to happen, is to fall 
back to not using channel binding. But it's not clear how to make that 
work, and still protect from downgrade attacks. If the client responds 
"y", meaning "the client supports channel binding, but it looks like the 
server doesn't", then the server will reject the authentication, because 
it did actually support channel binding. Just not the same one that the 
client did. If the client could reply "y", and also echo back the 
server's list of supported channel bindings in the same message, that 
would solve the problem. But the spec doesn't do that.

>> I guess we should err on the side of caution, and fail the authentication in
>> that case. That's unfortunate, but it's better than not negotiating at all.
>> At least with the negotiation, the authentication will work if there is any
>> mutually supported channel binding type.
> 
> I think that in this case the client should throw an error
> unconditionally if it wants to use channel A but server supports only B.
> It is always possible for the client to adjust the channel binding type
> it wants to use by using the connection parameter scram_channel_binding,
> or to recompile.  If there is incompatibility between channel binding
> types, it would actually mean that the client and the server are not
> compiled with the same SSL implementation, would that actually work? Say
> a server has only tls-unique with gnu's library and the client uses JDBC
> which only has tls-server-end-point..

We would like to fall back to not using channel binding at all in that 
scenario, assuming the configuration doesn't require channel binding. 
But yeah, rejecting the connection seems to be the best we can do, given 
what the protocol is.

> So, to keep things simple, it seems to me that we should just make the
> server send the list, and then check at client-level if the list sent by
> server includes what the client wants to use, complaining if the option
> is not present.

Yep.

It seems that all implementations can support tls-server-end-point, 
after all, so I'm not too worried about this anymore. The spec says that 
it's the default, but I don't actually see any advantage to using it 
over tls-server-end-point. I think the main reason for tls-unique to 
exist is that it doesn't require the server to have a TLS certificate, 
but PostgreSQL requires that anyway.

Actually, I wonder if we should just rip out the tls-unique support, 
after all? We can add it back at a later version, if there is a need for 
it, but I don't think we will. With security-related code, simpler is 
better.

- Heikki

  9. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-12T09:06:16Z

    On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:
> It seems that all implementations can support tls-server-end-point, after
> all, so I'm not too worried about this anymore. The spec says that it's the
> default, but I don't actually see any advantage to using it over
> tls-server-end-point. I think the main reason for tls-unique to exist is
> that it doesn't require the server to have a TLS certificate, but PostgreSQL
> requires that anyway.

Er.  My memories about the spec are a bit different: tls-unique must be
implemented and is the default.

[ ... digging ... ]

Here you go:
https://tools.ietf.org/html/rfc5802#section-6.1
--
Michael

  10. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-12T09:34:51Z

    On 12/07/18 12:06, Michael Paquier wrote:
> On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:
>> It seems that all implementations can support tls-server-end-point, after
>> all, so I'm not too worried about this anymore. The spec says that it's the
>> default, but I don't actually see any advantage to using it over
>> tls-server-end-point. I think the main reason for tls-unique to exist is
>> that it doesn't require the server to have a TLS certificate, but PostgreSQL
>> requires that anyway.
> 
> Er.  My memories about the spec are a bit different: tls-unique must be
> implemented and is the default.
> 
> [ ... digging ... ]
> 
> Here you go:
> https://tools.ietf.org/html/rfc5802#section-6.1

Meh. We're not going implement tls-unique, anyway, in some of the 
upcoming non-OpenSSL TLS implementations that don't support it.

Hmm. That is actually in a section called "Default Channel Binding". And 
the first paragraph says "A default channel binding type agreement 
process for all SASL application protocols that do not provide their own 
channel binding type agreement is provided as follows". Given that, it's 
not entirely clear to me if the requirement to support tls-unique is for 
all implementations of SCRAM, or only those applications that don't 
provide their own channel binding type agreement.

- Heikki

  11. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-12T13:08:18Z

    On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote:
> Meh. We're not going implement tls-unique, anyway, in some of the upcoming
> non-OpenSSL TLS implementations that don't support it.

True enough.  Only GnuTLS supports it:
https://www.gnutls.org/manual/html_node/Channel-Bindings.html

> Hmm. That is actually in a section called "Default Channel Binding". And the
> first paragraph says "A default channel binding type agreement process for
> all SASL application protocols that do not provide their own channel binding
> type agreement is provided as follows". Given that, it's not entirely clear
> to me if the requirement to support tls-unique is for all implementations of
> SCRAM, or only those applications that don't provide their own channel
> binding type agreement.

I am not sure, but I get that as tls-unique must be the default if
available, so if it is technically possible to have it we should have it
in priority.  If not, then a channel binding type which is supported by
both the server and the client can be chosen.
--
Michael

  12. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-12T13:10:56Z

    On Wed, Jul 11, 2018 at 04:00:47PM +0300, Heikki Linnakangas wrote:
> Looking at the GnuTLS docs, I believe it has everything we need.
> gnutls_certificate_get_peers() and gnutls_certificate_get_ours() can be used
> to get the certificate, and gnutls_x509_crt_get_signature_algorithm() gets
> the signatureAlgorithm.

Looking at the docs, there is gnutls_x509_crt_get_fingerprint() which
can provide the certificate hash.  So if the signature algorithm  is MD5
or SHA-1, it would be simple enough to upgrade it to SHA-256 and
calculate the hash.  They have way better docs than OpenSSL, which is
nice.
--
Michael

  13. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-07-20T17:05:04Z

    On 12/07/18 16:08, Michael Paquier wrote:
> On Thu, Jul 12, 2018 at 12:34:51PM +0300, Heikki Linnakangas wrote:
>> Hmm. That is actually in a section called "Default Channel Binding". And the
>> first paragraph says "A default channel binding type agreement process for
>> all SASL application protocols that do not provide their own channel binding
>> type agreement is provided as follows". Given that, it's not entirely clear
>> to me if the requirement to support tls-unique is for all implementations of
>> SCRAM, or only those applications that don't provide their own channel
>> binding type agreement.
> 
> I am not sure, but I get that as tls-unique must be the default if
> available, so if it is technically possible to have it we should have it
> in priority.  If not, then a channel binding type which is supported by
> both the server and the client can be chosen.

Another interesting piece of legalese is in RFC 5929 Channel Bindings 
for TLS:

>    For many applications, there may be two or more potentially
>    applicable TLS channel binding types.  Existing security frameworks
>    (such as the GSS-API [RFC2743] or the SASL [RFC4422] GS2 framework
>    [RFC5801]) and security mechanisms generally do not support
>    negotiation of channel binding types.  Therefore, application peers
>    need to agree a priori as to what channel binding type to use (or
>    agree to rules for deciding what channel binding type to use).
> 
>    The specifics of whether and how to negotiate channel binding types
>    are beyond the scope of this document.  However, it is RECOMMENDED
>    that application protocols making use of TLS channel bindings, use
>    'tls-unique' exclusively, except, perhaps, where server-side proxies
>    are common in deployments of an application protocol.  In the latter
>    case an application protocol MAY specify that 'tls-server-end-point'
>    channel bindings must be used when available, with 'tls-unique' being
>    used when 'tls-server-end-point' channel bindings are not available.
>    Alternatively, the application may negotiate which channel binding
>    type to use, or may make the choice of channel binding type
>    configurable.

In any case, I'm quite convinced now that we should just remove 
tls-unique, and stick to tls-server-end-point. Patch attached, please 
take a look.

- Heikki

  14. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-07-22T13:54:35Z

    On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote:
> In any case, I'm quite convinced now that we should just remove tls-unique,
> and stick to tls-server-end-point. Patch attached, please take a look.

You are making good points here.

> This removes the scram_channel_binding libpq option altogether, since there
> is now only one supported channel binding type.

This can also be used to switch off channel binding on the client-side,
which is the behavior you could get only by using a version of libpq
compiled with v10 in the context of an SSL connection.  Do we really
want to lose this switch as well?  I like feature switches.

-   "SCRAM authentication with invalid channel binding");
+   "Basic SCRAM authentication");
Or "SCRAM authentication with SSL and channel binding?"

+PostgreSQL is <literal>tls-server-end-point</literal>.  If other channel
+binding types are supported in the future, the server will advertise
+them as separate SASL mechanisms.
I don't think that this is actually true, per my remark of upthread we
could also use an option-based approach with each SASL mechanism sent by
the server.  I would recommend to remove this last sentence.

+   man-in-the-middle attacks by mixing the signature of the server's
+   certificate into the transmitted password hash. While a fake server can
+   retransmit the real server's certificate, it doesn't have access to the
+   private key matching that certificate, and therefore cannot prove it is
+   the owner, causing SSL connection failure
Nit: insisting on the fact that tls-server-end-point is used in this
context.

+void
+pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
+{
+   /*
+    * Advertise the mechanisms in decreasing order of importance.  So the
+    * channel-binding variants go first, if they are supported. Channel
+    * binding is only supported with SSL, and only if the SSL implementation
+    * has a function to get the certificate's hash
[...]
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+   if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
+       state->channel_binding_in_use = true;
+   else
+#endif
Hm.  I think that this should be part of the set of APIs that each SSL
implementation has to provide.  It is not clear to me yet if using the
flavor of SSL in Windows or macos universe will allow end-point to work,
and this could make this proposal more complicated.  And
HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would
recommend to remove all SSL-implementation-specific code from auth*.c
and fe-auth*.c, keeping those in their own file.  One simple way to
address this problem would be to make each SSL implementation return a
boolean to tell if it supports SCRAM channel binding or not, with Port*
of PGconn* in input to be able to filter connections using SSL or not.

+    if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_PROTOCOL_VIOLATION),
+                (errmsg("unsupported SCRAM channel-binding type
\"%s\"",
+                        sanitize_str(channel_binding_type)))));
Let's give up on sanitize_str.  I am not sure that it is a good idea to
print in error messages server-side something that the client has sent.

-    * is advertised in decreasing order of importance.  So the
-    * channel-binding variants go first, if they are supported. Channel
-    * binding is only supported in SSL builds.
+    * is advertised in decreasing order of importance.
This comment is a duplicate of what is with pg_be_scram_get_mechanisms.

+#ifdef HAVE_X509_GET_SIGNATURE_NID
char *
pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
{
-#ifdef HAVE_X509_GET_SIGNATURE_NID
I would have kept the ifdef at its original place, to keep the
OpenSSL-specific CFLAGS [be|fe]-secure-openssl.c.

And a couple of lines down the call to be_tls_get_certificate_hash in
auth-scram.c is only protected by USE_SSL...  So compilation would
likely break once a new SSL implementation is added, and libpq-be.h gets
uglier.

+ * The username to  was provided by the client in the startup message, and is
Nit: two spaces between "to" and "was".

+   errdetail("The client selected SCRAM-SHA-256 without channel
binding, but the SCRAM message includes channel binding data,")));
Why a comma at the end?

+       /*
+        * Chose channel binding, but the SSL library doesn't support it.
+        * Shouldn't happen.
+        */
+       termPQExpBuffer(&buf);
+       return NULL;
An error message is misisng here?
--
Michael

  15. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-05T11:00:04Z

    Thanks for the review!

On 22/07/18 16:54, Michael Paquier wrote:
> On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote:
>> This removes the scram_channel_binding libpq option altogether, since there
>> is now only one supported channel binding type.
> 
> This can also be used to switch off channel binding on the client-side,
> which is the behavior you could get only by using a version of libpq
> compiled with v10 in the context of an SSL connection.  Do we really
> want to lose this switch as well?  I like feature switches.

Well, it'd be useless for users, there is no reason to switch off 
channel binding if both the client and server support it. It might not 
add any security you care about, but it won't do any harm either. The 
non-channel-binding codepath is still exercised with non-SSL connections.

> +PostgreSQL is <literal>tls-server-end-point</literal>.  If other channel
> +binding types are supported in the future, the server will advertise
> +them as separate SASL mechanisms.
> I don't think that this is actually true, per my remark of upthread we
> could also use an option-based approach with each SASL mechanism sent by
> the server.  I would recommend to remove this last sentence.

Ok. That's how I'm envisioning we'd add future binding types, but since 
we're not sure, I'll leave it out.

> +   man-in-the-middle attacks by mixing the signature of the server's
> +   certificate into the transmitted password hash. While a fake server can
> +   retransmit the real server's certificate, it doesn't have access to the
> +   private key matching that certificate, and therefore cannot prove it is
> +   the owner, causing SSL connection failure
> Nit: insisting on the fact that tls-server-end-point is used in this
> context.

Yeah, that's the assumption. Now that we only do tls-server-end-point, I 
think we can assume that without further explanation.

> +void
> +pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
> +{
> +   /*
> +    * Advertise the mechanisms in decreasing order of importance.  So the
> +    * channel-binding variants go first, if they are supported. Channel
> +    * binding is only supported with SSL, and only if the SSL implementation
> +    * has a function to get the certificate's hash
> [...]
> +#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
> +   if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
> +       state->channel_binding_in_use = true;
> +   else
> +#endif
> Hm.  I think that this should be part of the set of APIs that each SSL
> implementation has to provide.  It is not clear to me yet if using the
> flavor of SSL in Windows or macos universe will allow end-point to work,
> and this could make this proposal more complicated.  And
> HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would
> recommend to remove all SSL-implementation-specific code from auth*.c
> and fe-auth*.c, keeping those in their own file.  One simple way to
> address this problem would be to make each SSL implementation return a
> boolean to tell if it supports SCRAM channel binding or not, with Port*
> of PGconn* in input to be able to filter connections using SSL or not.

The idea here is that if the SSL implementation implements the required 
functions, it #defines HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH (in the 
client) and/or HAVE_BE_TLS_GET_CERTIFICATE_HASH (in the server). So the 
code above is not implementation-specific; it doesn't know the details 
of OpenSSL, it only refers to the compile-time flag which the SSL 
implementation-specific code defines. The flag is part of the API that 
the SSL implementation provides, it's just a compile-time flag rather 
than run-time.

I'll try to clarify the comments on this.

> +    if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                (errmsg("unsupported SCRAM channel-binding type
> \"%s\"",
> +                        sanitize_str(channel_binding_type)))));
> Let's give up on sanitize_str.  I am not sure that it is a good idea to
> print in error messages server-side something that the client has sent.

Well, the point of sanitize_str is to make it safe. You're right that 
it's not strictly necessary, but I think it would be helpful to print 
out the channel binding type that the client attempted to use.

> And a couple of lines down the call to be_tls_get_certificate_hash in
> auth-scram.c is only protected by USE_SSL...  So compilation would
> likely break once a new SSL implementation is added, and libpq-be.h gets
> uglier.

Fixed by changing "#ifdef USE_SSL" to "#ifdef 
HAVE_BE_TLS_GET_CERTIFICATE_HASH".

It's true that there is some risk for libpq-be.h (and libpq-int.h) to 
become ugly, if we add more SSL implementations, and if those 
implementations have complicated conditions on whether they can get the 
certificate hashes. In practice, I think it's going to be OK. All the 
SSL implementations we've talked about - GnuTLS, macOS, Windows - do 
support the functionality, so we don't need complicated #ifdefs in the 
header. But we can revisit this if it gets messy.

I did some further testing with this, compiling with and without 
HAVE_BE_TLS_GET_CERTIFICATE_HASH and 
HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH, and fixed a few combinations that 
did not work. And I fixed the other comment typos etc. that you pointed out.

I have committed this now, because I think it's important to get this 
into the next beta version, and I'd like to get a full cycle on the 
buildfarm before that. But if you have the chance, please have one more 
look at the committed version, to make sure I didn't mess something up.

- Heikki

  16. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-08-05T12:08:08Z

    On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote:
> I did some further testing with this, compiling with and without
> HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH,
> and fixed a few combinations that did not work. And I fixed the other
> comment typos etc. that you pointed out.

Two things that I am really unhappy about is first that you completely
wiped out the test suite for channel binding.  We know that channel
binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why
didn't you keep the check on supports_tls_server_end_point to determine
if the connection should be a failure or a success?

Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and
the other flags over-complicated, but I won't fight hard on that point
if you want to go your way.

> I have committed this now, because I think it's important to get this into
> the next beta version, and I'd like to get a full cycle on the buildfarm
> before that. But if you have the chance, please have one more look at the
> committed version, to make sure I didn't mess something up.

This I definitely agree with, getting this patch in before beta 3 is the
best thing to do now.
--
Michael

  17. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-05T12:30:43Z

    On 05/08/18 15:08, Michael Paquier wrote:
> On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote:
>> I did some further testing with this, compiling with and without
>> HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH,
>> and fixed a few combinations that did not work. And I fixed the other
>> comment typos etc. that you pointed out.
> 
> Two things that I am really unhappy about is first that you completely
> wiped out the test suite for channel binding.  We know that channel
> binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why
> didn't you keep the check on supports_tls_server_end_point to determine
> if the connection should be a failure or a success?

That test just tested that the scram_channel_binding libpq option works, 
but I removed the option. I know you wanted to keep it as a feature 
flag, but as discussed earlier, I don't think that'd be useful.

> Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and
> the other flags over-complicated, but I won't fight hard on that point
> if you want to go your way.

I don't feel too strongly about this either, so if you want to write a 
patch to refactor that, I'm all ears. Note that I had to do something, 
so that the server code knows whether to advertise SCRAM-SHA-256-PLUS or 
not, and likewise that the client knows whether to choose channel 
binding or not.

- Heikki

  18. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-08-05T12:45:34Z

    On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote:
> That test just tested that the scram_channel_binding libpq option works, but
> I removed the option. I know you wanted to keep it as a feature flag, but as
> discussed earlier, I don't think that'd be useful.

Sorry for the noise, I missed that there is still the test "Basic SCRAM
authentication with SSL" so that would be fine.  I would have preferred
keeping around the negative test so as we don't break SSL connections
when the client enforced cbind_flag to 'n' as that would be useful when
adding new SSL implementations as that would avoid manual tests which
people will most likely forget, but well...

You can remove $supports_tls_server_end_point in 002_scram.pl by the
way.  Should I remove it or perhaps you would prefer to do it?
--
Michael

  19. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-05T14:11:59Z

    On 05/08/18 15:45, Michael Paquier wrote:
> On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote:
>> That test just tested that the scram_channel_binding libpq option works, but
>> I removed the option. I know you wanted to keep it as a feature flag, but as
>> discussed earlier, I don't think that'd be useful.
> 
> Sorry for the noise, I missed that there is still the test "Basic SCRAM
> authentication with SSL" so that would be fine.  I would have preferred
> keeping around the negative test so as we don't break SSL connections
> when the client enforced cbind_flag to 'n' as that would be useful when
> adding new SSL implementations as that would avoid manual tests which
> people will most likely forget, but well...

The only negative test there was, was to check for bogus 
scram_channel_binding option, "scram_channel_binding=not-exists". Yeah, 
it would be nice to have some, but this commit didn't really change that 
situation.

I'm hoping that we add a libpq option to actually force channel binding 
soon. That'll make channel binding actually useful to users, but it will 
also make it easier to write tests to check that channel binding is 
actually used or not used, in the right situations.

> You can remove $supports_tls_server_end_point in 002_scram.pl by the
> way.  Should I remove it or perhaps you would prefer to do it?

Ah, good catch. I'll go and remove it, thanks!

- Heikki

  20. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-05T14:15:19Z

    On 05/08/18 17:11, I wrote:
> The only negative test there was, was to check for bogus
> scram_channel_binding option, "scram_channel_binding=not-exists". Yeah,
> it would be nice to have some, but this commit didn't really change that
> situation.

Sorry, I see now that there was indeed a test for 
scram_channel_binding='', which meant "no channel binding". That was 
confusing, I assumed '' was the default.

> I'm hoping that we add a libpq option to actually force channel binding
> soon. That'll make channel binding actually useful to users, but it will
> also make it easier to write tests to check that channel binding is
> actually used or not used, in the right situations.

Nevertheless, this we should do.

- Heikki

  21. Re: Negotiating the SCRAM channel binding type

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-05T16:01:04Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Sorry, I see now that there was indeed a test for 
> scram_channel_binding='', which meant "no channel binding". That was 
> confusing, I assumed '' was the default.

Ugh, it isn't?  There's a general principle in libpq that setting a
parameter to an empty string is the same as leaving it unset.  I think
violating that pattern is a bad idea.

			regards, tom lane

  22. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-05T16:38:58Z

    On 5 August 2018 19:01:04 EEST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Sorry, I see now that there was indeed a test for 
>> scram_channel_binding='', which meant "no channel binding". That was 
>> confusing, I assumed '' was the default.
>
>Ugh, it isn't?  There's a general principle in libpq that setting a
>parameter to an empty string is the same as leaving it unset.  I think
>violating that pattern is a bad idea.

Yeah. In any case, the whole option is gone now, so we're good.

- Heikki

  23. Re: Negotiating the SCRAM channel binding type

    Robert Haas <robertmhaas@gmail.com> — 2018-08-07T09:02:27Z

    On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Well, it'd be useless for users, there is no reason to switch off channel
> binding if both the client and server support it. It might not add any
> security you care about, but it won't do any harm either. The
> non-channel-binding codepath is still exercised with non-SSL connections.

Is that true?  What if it makes a connection fail that you wanted to
succeed?  Suppose we discover a bug that makes connections using
channel binding fail on Thursdays.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  24. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-08-07T11:40:43Z

    On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote:
> On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Well, it'd be useless for users, there is no reason to switch off channel
>> binding if both the client and server support it. It might not add any
>> security you care about, but it won't do any harm either. The
>> non-channel-binding codepath is still exercised with non-SSL connections.
> 
> Is that true?  What if it makes a connection fail that you wanted to
> succeed?  Suppose we discover a bug that makes connections using
> channel binding fail on Thursdays.

Well, as things stand today on HEAD, if channel binding has a bug, this
makes the SCRAM authentication not able to work over SSL, hence you need
to either drop SSL, SCRAM or patch libpq so as the client tells the
server that it does not want to use channel binding.  None of those are
appealing.  Before 7729113, the client still had the choice to enforce
channel binding to not be used over SSL, which I think is a win to
bypass any potential future bugs.  On top of that, we can test
automatically for *any* future SSL implementations that (SSL + SCRAM +
no channel binding) actually works properly, which is, it seems at least
to me, a good thing to get more confidence when a new SSL implementation
is added.
--
Michael

  25. Re: Negotiating the SCRAM channel binding type

    Stephen Frost <sfrost@snowman.net> — 2018-08-07T14:34:31Z

    Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote:
> > On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> Well, it'd be useless for users, there is no reason to switch off channel
> >> binding if both the client and server support it. It might not add any
> >> security you care about, but it won't do any harm either. The
> >> non-channel-binding codepath is still exercised with non-SSL connections.
> > 
> > Is that true?  What if it makes a connection fail that you wanted to
> > succeed?  Suppose we discover a bug that makes connections using
> > channel binding fail on Thursdays.
> 
> Well, as things stand today on HEAD, if channel binding has a bug, this
> makes the SCRAM authentication not able to work over SSL, hence you need
> to either drop SSL, SCRAM or patch libpq so as the client tells the
> server that it does not want to use channel binding.  None of those are
> appealing.  Before 7729113, the client still had the choice to enforce
> channel binding to not be used over SSL, which I think is a win to
> bypass any potential future bugs.  On top of that, we can test
> automatically for *any* future SSL implementations that (SSL + SCRAM +
> no channel binding) actually works properly, which is, it seems at least
> to me, a good thing to get more confidence when a new SSL implementation
> is added.

Uh, really?  We can come up with all kinds of potential bugs that might
exist in the world but I don't think we should be writing in options for
everything that might fail due to some bug existing that we don't know
about.  Also, we aren't going to release support for a new SSL library
in a minor release, so if we end up needing this option due to some SSL
library that we really want to support not having channel binding
support then we can add the option then (or realize that maybe we
shouldn't be bothering with adding support for an SSL implementation
that doesn't support channel binding....  it's not exactly a new thing
these days and there's very good reasons for having it).

Now- if we thought that maybe there was some connection pooling solution
that could be made to work with SSL+SCRAM if channel binding is turned
off, then that's a use-case that maybe we should try and support, but
this notion that we need to be able to turn it off because there might
be a bug is hogwash, imv.  Now, I haven't seen a pooling solution
actually figure out a way to do SSL+SCRAM even without channel binding,
and there might not even be a way, so I'm currently a -1 on adding an
option to disable it, but if someone turned up tomorrow with an credible
approach to doing that, then I'd +1 adding the option.

Thanks!

Stephen

  26. Re: Negotiating the SCRAM channel binding type

    Bruce Momjian <bruce@momjian.us> — 2018-08-07T19:34:49Z

    On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:
> On 12/07/18 07:14, Michael Paquier wrote:
> >On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:
> >>I started digging into this more closely, and ran into a little problem. If
> >>channel binding is not used, the client sends a flag to the server to
> >>indicate if it's because the client doesn't support channel binding, or
> >>because it thought that the server doesn't support it. This is the
> >>gs2-cbind-flag. How should the flag be set, if the server supports channel
> >>binding type A, while client supports only type B? The purpose of the flag
> >>is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
> >>variants from the list of supported mechanisms. If we treat incompatible
> >>channel binding types as "client doesn't support channel binding", then the
> >>downgrade attack becomes possible (the attacker can replace the legit PLUS
> >>variants with bogus channel binding types that the client surely doesn't
> >>support). If we treat it as "server doesn't support channel binding", then
> >>we won't downgrade to the non-channel-binding variant, in the legitimate
> >>case that the client and server both support channel binding, but with
> >>incompatible types.
> >>
> >>What we'd really want, is to include the full list of server's supported
> >>mechanisms as part of the exchange, not just a boolean "y/n" flag. But
> >>that's not what the spec says :-(.
> >
> >Let's not break the spec :)  I understand from it that the client is in
> >charge of the choice, so we are rather free to choose the reaction the
> >client should have.  In the second phase of the exchange, the client
> >communicates back to the server the channel binding it has decided to
> >choose, it is not up to the server to pick up one if the client thinks
> >that it can use multiple ones.
> 
> The case where the client and the server both support the same channel
> binding type, we're OK. The problematic case is when e.g. the server only
> supports tls-unique and the client only supports tls-server-end-point. What
> we would (usually) like to happen, is to fall back to not using channel
> binding. But it's not clear how to make that work, and still protect from
> downgrade attacks. If the client responds "y", meaning "the client supports
> channel binding, but it looks like the server doesn't", then the server will
> reject the authentication, because it did actually support channel binding.
> Just not the same one that the client did. If the client could reply "y",
> and also echo back the server's list of supported channel bindings in the
> same message, that would solve the problem. But the spec doesn't do that.
                ----------------------------

I know this is an academic question now, but I am not sure this is true.
A man-in-the-middle attacker could say they don't support channel
binding to the real client and real server and pretend to be the real
server.  What would work is to hash the secret in with the supported
channel binding list.  This is how TLS works --- all previous messages
are combined with the secret into a transmitted hash to prevent a MITM
from changing it.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  27. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-07T20:08:12Z

    On 07/08/18 22:34, Bruce Momjian wrote:
> On Thu, Jul 12, 2018 at 11:26:30AM +0300, Heikki Linnakangas wrote:
>> On 12/07/18 07:14, Michael Paquier wrote:
>>> On Wed, Jul 11, 2018 at 03:01:03PM +0300, Heikki Linnakangas wrote:
>>>> I started digging into this more closely, and ran into a little problem. If
>>>> channel binding is not used, the client sends a flag to the server to
>>>> indicate if it's because the client doesn't support channel binding, or
>>>> because it thought that the server doesn't support it. This is the
>>>> gs2-cbind-flag. How should the flag be set, if the server supports channel
>>>> binding type A, while client supports only type B? The purpose of the flag
>>>> is to detect downgrade attacks, where a man-in-the-middle removes the PLUS
>>>> variants from the list of supported mechanisms. If we treat incompatible
>>>> channel binding types as "client doesn't support channel binding", then the
>>>> downgrade attack becomes possible (the attacker can replace the legit PLUS
>>>> variants with bogus channel binding types that the client surely doesn't
>>>> support). If we treat it as "server doesn't support channel binding", then
>>>> we won't downgrade to the non-channel-binding variant, in the legitimate
>>>> case that the client and server both support channel binding, but with
>>>> incompatible types.
>>>>
>>>> What we'd really want, is to include the full list of server's supported
>>>> mechanisms as part of the exchange, not just a boolean "y/n" flag. But
>>>> that's not what the spec says :-(.
>>>
>>> Let's not break the spec :)  I understand from it that the client is in
>>> charge of the choice, so we are rather free to choose the reaction the
>>> client should have.  In the second phase of the exchange, the client
>>> communicates back to the server the channel binding it has decided to
>>> choose, it is not up to the server to pick up one if the client thinks
>>> that it can use multiple ones.
>>
>> The case where the client and the server both support the same channel
>> binding type, we're OK. The problematic case is when e.g. the server only
>> supports tls-unique and the client only supports tls-server-end-point. What
>> we would (usually) like to happen, is to fall back to not using channel
>> binding. But it's not clear how to make that work, and still protect from
>> downgrade attacks. If the client responds "y", meaning "the client supports
>> channel binding, but it looks like the server doesn't", then the server will
>> reject the authentication, because it did actually support channel binding.
>> Just not the same one that the client did. If the client could reply "y",
>> and also echo back the server's list of supported channel bindings in the
>> same message, that would solve the problem. But the spec doesn't do that.
>                  ----------------------------
> 
> I know this is an academic question now, but I am not sure this is true.
> A man-in-the-middle attacker could say they don't support channel
> binding to the real client and real server and pretend to be the real
> server.  What would work is to hash the secret in with the supported
> channel binding list.  This is how TLS works --- all previous messages
> are combined with the secret into a transmitted hash to prevent a MITM
> from changing it.

Yeah, that is what I meant. Currently, when client chooses to not use 
channel binding, it the sends a single flag, y/n, to indicate whether it 
thinks the server supports channel binding or not. That flag is included 
in the hashes used in the authentication, so if a MITM tries to change 
it, the authentication will fail. If instead of a single flag, it 
included a list of channel binding types supported by the server, that 
would solve the problem with supporting multiple channel binding types.

- Heikki

  28. Re: Negotiating the SCRAM channel binding type

    Bruce Momjian <bruce@momjian.us> — 2018-08-07T21:13:50Z

    On Tue, Aug  7, 2018 at 11:08:12PM +0300, Heikki Linnakangas wrote:
> >I know this is an academic question now, but I am not sure this is true.
> >A man-in-the-middle attacker could say they don't support channel
> >binding to the real client and real server and pretend to be the real
> >server.  What would work is to hash the secret in with the supported
> >channel binding list.  This is how TLS works --- all previous messages
> >are combined with the secret into a transmitted hash to prevent a MITM
> >from changing it.
> 
> Yeah, that is what I meant. Currently, when client chooses to not use
> channel binding, it the sends a single flag, y/n, to indicate whether it
> thinks the server supports channel binding or not. That flag is included in
> the hashes used in the authentication, so if a MITM tries to change it, the
> authentication will fail. If instead of a single flag, it included a list of
> channel binding types supported by the server, that would solve the problem
> with supporting multiple channel binding types.

Yes, agreed.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

  29. Re: Negotiating the SCRAM channel binding type

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-08-08T06:05:13Z

    On 07/08/18 17:34, Stephen Frost wrote:
> * Michael Paquier (michael@paquier.xyz) wrote:
>> On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote:
>>> On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>>> Well, it'd be useless for users, there is no reason to switch off channel
>>>> binding if both the client and server support it. It might not add any
>>>> security you care about, but it won't do any harm either. The
>>>> non-channel-binding codepath is still exercised with non-SSL connections.
>>>
>>> Is that true?  What if it makes a connection fail that you wanted to
>>> succeed?  Suppose we discover a bug that makes connections using
>>> channel binding fail on Thursdays.
>>
>> Well, as things stand today on HEAD, if channel binding has a bug, this
>> makes the SCRAM authentication not able to work over SSL, hence you need
>> to either drop SSL, SCRAM or patch libpq so as the client tells the
>> server that it does not want to use channel binding.  None of those are
>> appealing.  Before 7729113, the client still had the choice to enforce
>> channel binding to not be used over SSL, which I think is a win to
>> bypass any potential future bugs.  On top of that, we can test
>> automatically for *any* future SSL implementations that (SSL + SCRAM +
>> no channel binding) actually works properly, which is, it seems at least
>> to me, a good thing to get more confidence when a new SSL implementation
>> is added.
> 
> Uh, really?  We can come up with all kinds of potential bugs that might
> exist in the world but I don't think we should be writing in options for
> everything that might fail due to some bug existing that we don't know
> about.

Yeah, if there's a bug, we'll fix it and move on, like with any other 
feature.

> Now- if we thought that maybe there was some connection pooling solution
> that could be made to work with SSL+SCRAM if channel binding is turned
> off, then that's a use-case that maybe we should try and support, but
> this notion that we need to be able to turn it off because there might
> be a bug is hogwash, imv.  Now, I haven't seen a pooling solution
> actually figure out a way to do SSL+SCRAM even without channel binding,
> and there might not even be a way, so I'm currently a -1 on adding an
> option to disable it, but if someone turned up tomorrow with an credible
> approach to doing that, then I'd +1 adding the option.

Now that's a lot more compelling argument for having an option. 
Essentially, you might have a legitimate proxy or connection pooler that 
acts like a Man-In-The-Middle.

The removed "channel_binding" libpq option wasn't very user-friendly, 
and wasn't very good for dealing with that scenario anyway; wouldn't you 
want to disable channel binding in the server rather than the client in 
that scenario? So I have no regrets in removing it. But going forward, 
we do need to put some thought in configuring this. We've talked a lot 
about a libpq option to require channel binding, but we should also have 
a server-side option to disable it.

- Heikki

  30. Re: Negotiating the SCRAM channel binding type

    Stephen Frost <sfrost@snowman.net> — 2018-08-08T13:36:08Z

    Greetings,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 07/08/18 17:34, Stephen Frost wrote:
> >Now- if we thought that maybe there was some connection pooling solution
> >that could be made to work with SSL+SCRAM if channel binding is turned
> >off, then that's a use-case that maybe we should try and support, but
> >this notion that we need to be able to turn it off because there might
> >be a bug is hogwash, imv.  Now, I haven't seen a pooling solution
> >actually figure out a way to do SSL+SCRAM even without channel binding,
> >and there might not even be a way, so I'm currently a -1 on adding an
> >option to disable it, but if someone turned up tomorrow with an credible
> >approach to doing that, then I'd +1 adding the option.
> 
> Now that's a lot more compelling argument for having an option. Essentially,
> you might have a legitimate proxy or connection pooler that acts like a
> Man-In-The-Middle.
> 
> The removed "channel_binding" libpq option wasn't very user-friendly, and
> wasn't very good for dealing with that scenario anyway; wouldn't you want to
> disable channel binding in the server rather than the client in that
> scenario? So I have no regrets in removing it. But going forward, we do need
> to put some thought in configuring this. We've talked a lot about a libpq
> option to require channel binding, but we should also have a server-side
> option to disable it.

Yeah, I'm pretty sure we'd need it on both sides.  If we had it only on
one side or the other then you run into the risk of downgrade attacks
where the MITM is able to say "I don't support channel binding!" to both
sides, even when the actual libpq client and PG server do.

Thanks!

Stephen

  31. Re: Negotiating the SCRAM channel binding type

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-31T10:18:52Z

    On 05/08/2018 14:45, Michael Paquier wrote:
> On Sun, Aug 05, 2018 at 03:30:43PM +0300, Heikki Linnakangas wrote:
>> That test just tested that the scram_channel_binding libpq option works, but
>> I removed the option. I know you wanted to keep it as a feature flag, but as
>> discussed earlier, I don't think that'd be useful.
> 
> Sorry for the noise, I missed that there is still the test "Basic SCRAM
> authentication with SSL" so that would be fine.  I would have preferred
> keeping around the negative test so as we don't break SSL connections
> when the client enforced cbind_flag to 'n' as that would be useful when
> adding new SSL implementations as that would avoid manual tests which
> people will most likely forget, but well...

I was updating the gnutls patch for the changed channel binding setup,
and I noticed that the 002_scram.pl test now passes even though the
gnutls patch currently does not support channel binding.  So AFAICT,
we're not testing the channel binding functionality there at all.  Is
that as intended?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  32. Re: Negotiating the SCRAM channel binding type

    Michael Paquier <michael@paquier.xyz> — 2018-08-31T17:27:16Z

    On Fri, Aug 31, 2018 at 12:18:52PM +0200, Peter Eisentraut wrote:
> I was updating the gnutls patch for the changed channel binding setup,
> and I noticed that the 002_scram.pl test now passes even though the
> gnutls patch currently does not support channel binding.  So AFAICT,
> we're not testing the channel binding functionality there at all.  Is
> that as intended?

As far as I understood that's the intention.  One can still test easily
channel binding if you implement it so you can make sure that the
default SSL connection still works.  And you can also make sure that if
you don't implement channel binding then an SSL connection still works.
But you cannot make sure that if you have channel binding implemented
then the disabled path works.

I'd still like to think that having a way to enforce the disabled code
path over SSL has value, but you know, votes...
--
Michael