Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
On 07/08/18 17:34, Stephen Frost wrote: * Michael Paquier (mich...@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 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
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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 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 +
Re: Negotiating the SCRAM channel binding type
Greetings, * Michael Paquier (mich...@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 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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
On Tue, Aug 07, 2018 at 02:32:27PM +0530, Robert Haas wrote: > On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas 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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
On Sun, Aug 5, 2018 at 4:30 PM, Heikki Linnakangas 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
Re: Negotiating the SCRAM channel binding type
On 5 August 2018 19:01:04 EEST, Tom Lane wrote: >Heikki Linnakangas 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
Re: Negotiating the SCRAM channel binding type
Heikki Linnakangas 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
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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 tls-server-end-point. 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
Re: Negotiating the SCRAM channel binding type
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 >From 4e2f010692aaef97d4e16822359e86073a53e96b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Fri, 20 Jul 2018 20:02:17 +0300 Subject: [PATCH 1/2] Remove support for tls-unique channel binding. There are some problems with the tls-unique channel binding type. It's not supported by all SSL libraries, and strictly speaking it's not defined for TLS 1.3 at all, even though at least in OpenSSL, the functions used for it still seem to work with TLS 1.3 connections. And since we had no mechanism to negotiate what channel binding type to use, there would be awkward interoperability issues if a server only supported some channel binding types. tls-server-end-point seems feasible to support with any SSL library, so let's just stick to that. This removes the scram_channel_binding libpq option altogether, since there is now only one supported channel binding type. This also removes all the channel binding tests from the SSL test suite. It would be good to actually have some tests for channel binding, but these tests were really just testing the scram_channel_binding option, which is now gone. I also removed the SCRAM_CHANNEL_BINDING_TLS_END_POINT constant. This is a matter of taste, but IMO it's more readable to just use the "tls-server-end-point" string. Refactor the checks on whether the SSL library supports the functions needed for tls-server-end-point channel binding. Now the server won't advertise, and the client won't choose, the SCRAM-SHA-256-PLUS variant, if compiled with an OpenSSL version too old to support it. In the passing, add some sanity checks to check that the chosen SASL mechanism, SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, matches whether the SCRAM exchange used channel binding or not. For example, if the client selects the non-channel-binding variant SCRAM-SHA-256, but in the SCRAM message uses channel binding anyway. It's harmless from a security point of view, I believe, and I'm not sure if there are some other conditions that would cause the connection to fail, but it seems better to be strict about these things and check explicitly. Discussion: https://www.postgresql.org/message-id/ec787074-2305-c6f4-86aa-6902f98485a4%40iki.fi --- doc/src/sgml/libpq.sgml | 28 doc/src/sgml/protocol.sgml | 28 ++-- doc/src/sgml/release-11.sgml | 5 +- src/backend/libpq/auth-scram.c | 218 +-- src/backend/libpq/auth.c | 61 +++-- src/backend/libpq/be-secure-openssl.c| 27 +--- src/include/common/scram-common.h| 4 - src/include/libpq/libpq-be.h | 12 +- src/include/libpq/scram.h| 4 +-
Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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 signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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 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 +
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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
Re: Negotiating the SCRAM channel binding type
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.gb1...@paquier.xyz We need to allow a given SSL implementation to specify what the list of channel bindings it supports is. -- Michael signature.asc Description: PGP signature
Re: Negotiating the SCRAM channel binding type
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 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 +
Negotiating the SCRAM channel binding type
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
