Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Mon, Oct  8, 2018 at 12:42:39PM +0200, Peter Eisentraut wrote:
> On 05/10/2018 19:01, Bruce Momjian wrote:
> > On Fri, Oct  5, 2018 at 04:53:34PM +0200, Peter Eisentraut wrote:
> >> On 23/05/2018 08:46, Heikki Linnakangas wrote:
> >>> "tls-unique" and "tls-server-end-point" are overly technical to users. 
> >>> They don't care which one is used, there's no difference in security. 
> >>
> >> A question was raised about this in a recent user group meeting.
> >>
> >> When someone steals the server certificate from the real database server
> >> and sets up a MITM with that certificate, this would pass
> >> tls-server-end-point channel binding, because both the MITM and the real
> >> server have the same certificate.  But with tls-unique they would have
> >> different channel binding data, so the channel binding would detect this.
> >>
> >> Is that not correct?
> > 
> > Not correct.  First, they need to steal the server certificate and
> > _private_ key that goes with the certificate to impersonate the owner of
> > the certificate.
> 
> Right, I meant to imply that.

Right --- that is often confused so I wanted to clarify.

> > If that happens, with tls-server-end-point, a MITM
> > could replay what the real server sends to the MITM.  You are right that
> > tls-unique makes it harder for a MITM to reproduce the TLS shared key
> > which is mixed with the password hash to prove the server knows the
> > password hash.
> 
> So you appear to be saying the above *is* correct?

Yep, I am afraid so.  tls-unique seems technically superior, but I guess
not operationally superior, e.g., Java can't access the TLS shared
secret.  Supporting both causes the kind of channel binding mode
confusion that we have been dealing with, and the new security trend is
not to support too many options because their interaction is often
surprising, and insecure.

-- 
  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: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 05/10/2018 19:01, Bruce Momjian wrote:
> On Fri, Oct  5, 2018 at 04:53:34PM +0200, Peter Eisentraut wrote:
>> On 23/05/2018 08:46, Heikki Linnakangas wrote:
>>> "tls-unique" and "tls-server-end-point" are overly technical to users. 
>>> They don't care which one is used, there's no difference in security. 
>>
>> A question was raised about this in a recent user group meeting.
>>
>> When someone steals the server certificate from the real database server
>> and sets up a MITM with that certificate, this would pass
>> tls-server-end-point channel binding, because both the MITM and the real
>> server have the same certificate.  But with tls-unique they would have
>> different channel binding data, so the channel binding would detect this.
>>
>> Is that not correct?
> 
> Not correct.  First, they need to steal the server certificate and
> _private_ key that goes with the certificate to impersonate the owner of
> the certificate.

Right, I meant to imply that.

> If that happens, with tls-server-end-point, a MITM
> could replay what the real server sends to the MITM.  You are right that
> tls-unique makes it harder for a MITM to reproduce the TLS shared key
> which is mixed with the password hash to prove the server knows the
> password hash.

So you appear to be saying the above *is* correct?

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

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Fri, Oct  5, 2018 at 04:53:34PM +0200, Peter Eisentraut wrote:
> On 23/05/2018 08:46, Heikki Linnakangas wrote:
> > "tls-unique" and "tls-server-end-point" are overly technical to users. 
> > They don't care which one is used, there's no difference in security. 
> 
> A question was raised about this in a recent user group meeting.
> 
> When someone steals the server certificate from the real database server
> and sets up a MITM with that certificate, this would pass
> tls-server-end-point channel binding, because both the MITM and the real
> server have the same certificate.  But with tls-unique they would have
> different channel binding data, so the channel binding would detect this.
> 
> Is that not correct?

Not correct.  First, they need to steal the server certificate and
_private_ key that goes with the certificate to impersonate the owner of
the certificate.  If that happens, with tls-server-end-point, a MITM
could replay what the real server sends to the MITM.  You are right that
tls-unique makes it harder for a MITM to reproduce the TLS shared key
which is mixed with the password hash to prove the server knows the
password hash.

I think the standard is now focusing on tls-server-end-point because
most APIs make the certificate more accessible than the TLS shared key. 
There also might be exploits with TLS shared keys being cached to
improve SSL performance, particularly for https access:

https://tools.ietf.org/html/draft-badra-tls-key-exchange-00

Of course, that is just a guess.

-- 
  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: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 23/05/2018 08:46, Heikki Linnakangas wrote:
> "tls-unique" and "tls-server-end-point" are overly technical to users. 
> They don't care which one is used, there's no difference in security. 

A question was raised about this in a recent user group meeting.

When someone steals the server certificate from the real database server
and sets up a MITM with that certificate, this would pass
tls-server-end-point channel binding, because both the MITM and the real
server have the same certificate.  But with tls-unique they would have
different channel binding data, so the channel binding would detect this.

Is that not correct?

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

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 29.06.18 04:16, Michael Paquier wrote:
> I am able to find easily drafts of TLS 1.3, but I am not seeing an RFC
> associated to it, which would be the base document to rely on I
> guess...  So that's really hard to make any decision in this area
> without the real deal.  As far as I can see tls-unique could be
> deprecated and replaced, but from OpenSSL point of view it technically
> works.

At this point, I think we can leave it as is until they came out with a
new channel binding type.

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

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, Jun 29, 2018 at 10:37:55AM +0900, Michael Paquier wrote:
> The set of APIs that we use to the SSL abstraction layer is very
> internal, so it would not be an issue if we add some in stable branches,
> no?  My point is that from OpenSSL point of view, TLS 1.3 stuff has been
> added in 1.1.1 which is now in beta 6 stage, so we could consider as
> well all this part once OpenSSL is released.  That's compatibility work
> I wanted to work on anyway.  Impossible to say down to which versions of
> Postgres things could be applied easily though without a deep
> investigation of the new compatibility breakages that upstream OpenSSL
> has very-likely introduced in upstream.
> 
> Still it does not sound completely strange either to me to wait for
> OpenSSL to release as we won't be able to have a full solution designed
> before that.

Actually, I got curious about that part and just compiled Postgres with
OpenSSL 1.1.1 beta 6 that I compiled manually, and channel binding is
generating consistent data for both tls-unique and tls-server-end-point
even if TLS v1.3 is used, while tests in src/test/ssl/ are all able to
pass.  So that's less dramatic than what I thought after the melodrama
of upgrading the code to 1.1.0.

The thread where this is discussed is also kind of interesting as the
last email points to having tls-unique deprecated for all the TLS
versions:
https://www.ietf.org/mail-archive/web/tls/current/msg18265.html

I am able to find easily drafts of TLS 1.3, but I am not seeing an RFC
associated to it, which would be the base document to rely on I
guess...  So that's really hard to make any decision in this area
without the real deal.  As far as I can see tls-unique could be
deprecated and replaced, but from OpenSSL point of view it technically
works.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Thu, Jun 28, 2018 at 10:05:23AM +0200, Peter Eisentraut wrote:
> But before we drop the SCRAM business completely off the open items, I
> think we need to consider how TLS 1.3 affects this.

The set of APIs that we use to the SSL abstraction layer is very
internal, so it would not be an issue if we add some in stable branches,
no?  My point is that from OpenSSL point of view, TLS 1.3 stuff has been
added in 1.1.1 which is now in beta 6 stage, so we could consider as
well all this part once OpenSSL is released.  That's compatibility work
I wanted to work on anyway.  Impossible to say down to which versions of
Postgres things could be applied easily though without a deep
investigation of the new compatibility breakages that upstream OpenSSL
has very-likely introduced in upstream.

Still it does not sound completely strange either to me to wait for
OpenSSL to release as we won't be able to have a full solution designed
before that.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Thu, Jun 28, 2018 at 10:04:05AM +0200, Peter Eisentraut wrote:
> On 6/28/18 09:35, Magnus Hagander wrote:
> > No, we absolutely still have SCRAM channel binding.
> > 
> > *libpq* has no way to *enforce* it, meaning it always acts like our
> > default SSL config which is "use it if available but if it's not then
> > silently accept the downgrade". From a security perspective, it's just
> > as bad as our default ssl config, but unlike ssl you can't configure a
> > requirement in 11.
> 
> Isn't this similar to what happened whenever we added a new or better
> password method?  A MITM that didn't want to bother cracking MD5 could
> just alter the stream and request "password" authentication.  Same with
> MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
> the SCRAM hashing method.  Clearly, we need a more comprehensive
> solution for this.

The issue is that our different password methods were designed to do a
best-effort at preventing _passive_ snoopers from decrypting or reusing
passwords.  With channel binding, we are trying to prevent _active_
network attacks, and our fallback behavior eliminates the protection
that channel binding provides.

-- 
  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: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Thu, Jun 28, 2018 at 09:35:57AM +0200, Magnus Hagander wrote:
> No, we absolutely still have SCRAM channel binding.
> 
> *libpq* has no way to *enforce* it, meaning it always acts like our default 
> SSL
> config which is "use it if available but if it's not then silently accept the
> downgrade". From a security perspective, it's just as bad as our default ssl
> config, but unlike ssl you can't configure a requirement in 11.

I think we are much more likely to be able to force channel binding by
default since there is no need to configure a certificate authority.

-- 
  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: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Thu, Jun 28, 2018 at 10:04 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/28/18 09:35, Magnus Hagander wrote:
> > No, we absolutely still have SCRAM channel binding.
> >
> > *libpq* has no way to *enforce* it, meaning it always acts like our
> > default SSL config which is "use it if available but if it's not then
> > silently accept the downgrade". From a security perspective, it's just
> > as bad as our default ssl config, but unlike ssl you can't configure a
> > requirement in 11.
>
> Isn't this similar to what happened whenever we added a new or better
> password method?  A MITM that didn't want to bother cracking MD5 could
> just alter the stream and request "password" authentication.  Same with
> MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
> the SCRAM hashing method.  Clearly, we need a more comprehensive
> solution for this.
>

That is sort of the gist of the discussion, yes. It is.

So if you just enabled scram channel binding, an attacker could just turn
off scram completely.

That's why we need a solution that covers the full problem, which is why it
needs to be thought of as one problem so we don't end up with a fragmented
solution.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/28/18 09:33, Magnus Hagander wrote:
> Should there be one or more parameters? How should they interact? At
> which level should they be controlled? Limited to SCRAM or other channel
> bindings? Are the different levels of SCRAM to be considered different
> protocols or the same protocol with a tweak? etc.

OK, I'm fine with postponing this.

But before we drop the SCRAM business completely off the open items, I
think we need to consider how TLS 1.3 affects this.

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

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/28/18 09:35, Magnus Hagander wrote:
> No, we absolutely still have SCRAM channel binding.
> 
> *libpq* has no way to *enforce* it, meaning it always acts like our
> default SSL config which is "use it if available but if it's not then
> silently accept the downgrade". From a security perspective, it's just
> as bad as our default ssl config, but unlike ssl you can't configure a
> requirement in 11.

Isn't this similar to what happened whenever we added a new or better
password method?  A MITM that didn't want to bother cracking MD5 could
just alter the stream and request "password" authentication.  Same with
MD5->SCRAM, SCRAM->SCRAM+CB, and even a hypothetical future change in
the SCRAM hashing method.  Clearly, we need a more comprehensive
solution for this.

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

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Wed, Jun 27, 2018 at 7:24 PM, Alvaro Herrera 
wrote:

> Going over this thread a little bit I'm confused about what is being
> proposed.  I think I understand that we no longer think we have have
> SCRAM channel binding.  I hope that doesn't mean we don't have SCRAM
> itself.  However, in terms of the Postgres release proper, what do we
> need to do?  There is still an open item about this, and I had the
> impression that if we simply demoted channel binding from a pg11 major
> feature to barely a footnote that somebody can implement it with some
> hypothetical future JDBC driver that supports the option, then we're
> done.
>
> Am I mistaken?
>

No, we absolutely still have SCRAM channel binding.

*libpq* has no way to *enforce* it, meaning it always acts like our default
SSL config which is "use it if available but if it's not then silently
accept the downgrade". From a security perspective, it's just as bad as our
default ssl config, but unlike ssl you can't configure a requirement in 11.

There is nothing preventing a third party driver like jdbc or npgsql to
implement a way to enforce it. I would generally recommend they wait for
the outcome of the discussion about parameters and names in order to
implement the same semantics, but they don't have to wait for the next
postgres release.

It doesn't affect the having of SCRAM at all. That one is still there, and
has been since 10.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Alvaro Herrera]

Going over this thread a little bit I'm confused about what is being
proposed.  I think I understand that we no longer think we have have
SCRAM channel binding.  I hope that doesn't mean we don't have SCRAM
itself.  However, in terms of the Postgres release proper, what do we
need to do?  There is still an open item about this, and I had the
impression that if we simply demoted channel binding from a pg11 major
feature to barely a footnote that somebody can implement it with some
hypothetical future JDBC driver that supports the option, then we're
done.

Am I mistaken?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/14/18 13:43, Magnus Hagander wrote:
> I still think that the fact that we are still discussing what is
> basically the *basic concepts* of how this would be set up after we have
> released beta1 is a clear sign that this should not go into 11.

Other than some naming and handling of some nonsensical combinations,
what is unclear?

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

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sat, Jun 23, 2018 at 10:30:19PM +0900, Michael Paquier wrote:
> On Fri, Jun 22, 2018 at 11:01:53PM -0400, Bruce Momjian wrote:
> > Uh, as I am understanding it, if we don't allow clients to force channel
> > binding, then channel binding is useless because it cannot prevent
> > man-in-the-middle attacks.  I am sure some users will try to use it, and
> > not understand that it serves no purpose.  If we then allow clients to
> > force channel binding in PG 12, they will then need to fix their
> > clients.
> > 
> > I suggest that if we don't allow users to use channel binding
> > effectively that we should remove all documentation about this
> > feature.
> 
> Well, I don't agree with this position as the protocol put in place for
> SCRAM with or without channel binding perfectly allows a client to
> enforce the use channel binding.  While that's missing for libpq, other
> clients like JDBC or npgsql could perfectly implement that before this
> gets in Postgres core in the shape they want.  So I think that the docs
> should be kept.

Yes, the code is useful, but the _feature_ is not useful until some
interface allows the forcing of channel binding.  People are worried
about users having to change their API in PG 12, but the point is that
to use this feature people will have to change their API in PG 12
anyway, and it doesn't do anything useful without an interface we don't
ship, and hasn't been written, so why confuse people that it is a
feature in PG 11?

Channel binding is listed as a _major_ feature in PG 11 in the release
notes, and you can bet people are going to look at how to use it:

  Channel binding for SCRAM authentication, to prevent potential
  man-in-the-middle attacks on database connections

It should perhaps be marked in the source code section, and listed as
not useful by PG 11's libpq or any of the interfaces built on it.  We
are also going to need to communicate to people who have already looked
at the release notes that this features is not useful in PG 11 using
libpq.

-- 
  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: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, Jun 22, 2018 at 11:01:53PM -0400, Bruce Momjian wrote:
> Uh, as I am understanding it, if we don't allow clients to force channel
> binding, then channel binding is useless because it cannot prevent
> man-in-the-middle attacks.  I am sure some users will try to use it, and
> not understand that it serves no purpose.  If we then allow clients to
> force channel binding in PG 12, they will then need to fix their
> clients.
> 
> I suggest that if we don't allow users to use channel binding
> effectively that we should remove all documentation about this
> feature.

Well, I don't agree with this position as the protocol put in place for
SCRAM with or without channel binding perfectly allows a client to
enforce the use channel binding.  While that's missing for libpq, other
clients like JDBC or npgsql could perfectly implement that before this
gets in Postgres core in the shape they want.  So I think that the docs
should be kept.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sun, Jun 17, 2018 at 10:21:27PM +0900, Michael Paquier wrote:
> On Fri, Jun 15, 2018 at 05:23:27PM -0400, Robert Haas wrote:
> > On Thu, Jun 14, 2018 at 7:43 AM, Magnus Hagander  
> > wrote:
> >> I still think that the fact that we are still discussing what is basically
> >> the *basic concepts* of how this would be set up after we have released
> >> beta1 is a clear sign that this should not go into 11.
> > 
> > +1.
> 
> Yes, that sounds right.

Uh, as I am understanding it, if we don't allow clients to force channel
binding, then channel binding is useless because it cannot prevent
man-in-the-middle attacks.  I am sure some users will try to use it, and
not understand that it serves no purpose.  If we then allow clients to
force channel binding in PG 12, they will then need to fix their
clients.

I suggest that if we don't allow users to use channel binding
effectively that we should remove all documentation about this feature.

This is different from downgrade attacks like SCRAM to MD5 or MD5 to
'password' because the way the password is transmitted is not integral
to preventing man-in-the-middle attacks.  Channel binding's sole value
is to prevent such attacks, so if it cannot prevent them, it has no use
and will just confuse people until we make it useful in a later release.

-- 
  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: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, Jun 15, 2018 at 05:23:27PM -0400, Robert Haas wrote:
> On Thu, Jun 14, 2018 at 7:43 AM, Magnus Hagander  wrote:
>> I still think that the fact that we are still discussing what is basically
>> the *basic concepts* of how this would be set up after we have released
>> beta1 is a clear sign that this should not go into 11.
> 
> +1.

Yes, that sounds right.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Robert Haas]

On Thu, Jun 14, 2018 at 7:43 AM, Magnus Hagander  wrote:
> I still think that the fact that we are still discussing what is basically
> the *basic concepts* of how this would be set up after we have released
> beta1 is a clear sign that this should not go into 11.

+1.

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

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Tue, Jun 12, 2018 at 6:49 AM, Michael Paquier 
wrote:

> On Mon, Jun 11, 2018 at 04:54:45PM +0200, Magnus Hagander wrote:
> > I'm wondering if that means we should then also not do it specifically
> for
> > scram in this version. Otherwise we're likely to end up with a parameter
> > that only has a "lifetime" of one version, and that seems like a bad
> idea.
> > If nothing else we should clearly think out what the path is to make sure
> > that doesn't happen. (e.g. we don't want a
> > scram_channel_binding_mode=require in this version, if the next one is
> > going to replace it with something like heikkis suggested
> > allowed_authentication_methods=SCRAM-SHA-256-PLUS or whatever we end up
> > coming up with there).
>
> Conceptually, it depends on if we consider SCRAM and
> SCRAM+channel_binding as two separate authentication protocols.  However
> it seems to me that as both are the same thing as they use the same
> protocol so it would be confusing for the user to be able to define both
> SCRAM-SHA-256 and SCRAM-SHA-256-PLUS within the same list, so I would
> tend to think that we should have in this future
> "allowed_authentication_methods" only scram-sha-256 listed as an option,
> which counts for both SCRAM with and without channel binding.
>

One could argue they are equally the same protocol if we have
SCRAM-SHA-512 or whatever in the future. So how would those be handled?


Thinking harder about this thread, it could be as well possible in the
> future that we add support for channel binding for some other SASL
> mechanism, in which case I would tend to rename
> scram_channel_binding_type and scram_channel_binding_mode to simply
> channel_binding_type and channel_binding_mode, without any concepts of
> SCRAM attached to it.  So in short, I'd like to keep both enforcement
> mechanisms as separate concepts.  One future compatibility issue is how
> to deal with for example cases like allowed_authentication_methods="md5"
> and channel_binding_mode=require where an allowed authentication does
> not have channel binding?  And it seems to me that this should result in
> an error.
>

Yeah, not embedding scram in the name seems smarter. But you might still
need to define which one, so channel_binding_mode=require wouldn't be
enough in that case -- you'd need to have
channel_binding_mode=require-scram-sha-256-plus, wouldn't you?

And yes, in your suggested example, it should absolutely fail early, as
there is no way to actually connect with that setting. Arguably we should
also fail on e.g. sslmode=require over a unix socket, though, which we
don't. But that is probably not somethign to copy :)

I still think that the fact that we are still discussing what is basically
the *basic concepts* of how this would be set up after we have released
beta1 is a clear sign that this should not go into 11.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Mon, Jun 11, 2018 at 04:54:45PM +0200, Magnus Hagander wrote:
> I'm wondering if that means we should then also not do it specifically for
> scram in this version. Otherwise we're likely to end up with a parameter
> that only has a "lifetime" of one version, and that seems like a bad idea.
> If nothing else we should clearly think out what the path is to make sure
> that doesn't happen. (e.g. we don't want a
> scram_channel_binding_mode=require in this version, if the next one is
> going to replace it with something like heikkis suggested
> allowed_authentication_methods=SCRAM-SHA-256-PLUS or whatever we end up
> coming up with there).

Conceptually, it depends on if we consider SCRAM and
SCRAM+channel_binding as two separate authentication protocols.  However
it seems to me that as both are the same thing as they use the same
protocol so it would be confusing for the user to be able to define both
SCRAM-SHA-256 and SCRAM-SHA-256-PLUS within the same list, so I would
tend to think that we should have in this future
"allowed_authentication_methods" only scram-sha-256 listed as an option,
which counts for both SCRAM with and without channel binding.

Thinking harder about this thread, it could be as well possible in the
future that we add support for channel binding for some other SASL
mechanism, in which case I would tend to rename
scram_channel_binding_type and scram_channel_binding_mode to simply
channel_binding_type and channel_binding_mode, without any concepts of
SCRAM attached to it.  So in short, I'd like to keep both enforcement
mechanisms as separate concepts.  One future compatibility issue is how
to deal with for example cases like allowed_authentication_methods="md5"
and channel_binding_mode=require where an allowed authentication does
not have channel binding?  And it seems to me that this should result in
an error.

Opinions of others are welcome.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Mon, Jun 11, 2018 at 4:49 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/6/18 18:04, Michael Paquier wrote:
> > On Wed, Jun 06, 2018 at 11:53:06PM +0300, Heikki Linnakangas wrote:
> >> That would certainly be good. We've always had that problem, even with
> md5
> >> -> plaintext password downgrade, and it would be nice to fix it. It's
> quite
> >> late in the release cycle already, do you think we should address that
> now?
> >> I could go either way..
> >
> > I would be inclined to treat that as new development as this is no new
> > problem.
>
> I agree.
>
>
Agreed as well.

I'm wondering if that means we should then also not do it specifically for
scram in this version. Otherwise we're likely to end up with a parameter
that only has a "lifetime" of one version, and that seems like a bad idea.
If nothing else we should clearly think out what the path is to make sure
that doesn't happen. (e.g. we don't want a
scram_channel_binding_mode=require in this version, if the next one is
going to replace it with something like heikkis suggested
allowed_authentication_methods=SCRAM-SHA-256-PLUS or whatever we end up
coming up with there).

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/6/18 18:04, Michael Paquier wrote:
> On Wed, Jun 06, 2018 at 11:53:06PM +0300, Heikki Linnakangas wrote:
>> That would certainly be good. We've always had that problem, even with md5
>> -> plaintext password downgrade, and it would be nice to fix it. It's quite
>> late in the release cycle already, do you think we should address that now?
>> I could go either way..
> 
> I would be inclined to treat that as new development as this is no new
> problem.

I agree.

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

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Wed, Jun 06, 2018 at 11:46:11PM +0300, Heikki Linnakangas wrote:
> Ok. Perhaps add a comment pointing out that as the code stands,
> get_auth_request_str() is never called with AUTH_REQ_OK. So that if someone
> starts calling it with that, maybe they'll know to revisit this.

That makes sense.  Done.

>> + 
>> +  prefer (default)
>> +  
>> +   
>> +Use channel binding if available.  If the cluster does not
>> +support it, then it is not used.  This is the default.
>> +   
>> +  
>> + 
> 
> I'd suggest s/cluster/server/. Here, and elsewhere in the docs changes.

Done.

> There are some funny behaviors with different combinations of
> "scram_channel_binding_mode=require" and different sslmode settings. For
> example, with sslmode=disable, the client will attempt to connect, but it's
> guaranteed to fail at authentication, because channel binding is only
> possible with SSL. Perhaps it should throw an error without even attempting
> to connect? Or at least, the "server does not support channel binding" is
> misleading, as it was the client that insisted on an impossible combination;
> the server might well support channel binding, if SSL was used.
> 
> And with sslmode=allow, if the server allows a non-SSL connection, then the
> client won't use SSL, and will fail, as with sslmode=disable. But if the
> server requires SSL, then it will work. Perhaps sslmode=allow should be
> "promoted" to "sslmode=require", if scram_channel_binding_mode=require? Or
> don't let the user select a silly combination like that at all, and throw an
> error.

I was not really planning to make the code more complicated with
cross-option checks (that's complicated enough), but isn't what you are
looking for here to make sslmode=require a mandatory thing if
channel_binding_mode=require is set?  It is possible to add checks like
that in connectOptions2 for example after setting all the values.
Promoting automatically sslmode="allow" to "require" if
scram_channel_binding_mode=require feels like a trap for users as well.

> If scram_channel_binding_mode=require, but the server doesn't support SSL at
> all, you get:
> 
> psql: server does not support channel binding, and
> scram_channel_binding_mode=require was used
> 
> Perhaps it would be more clear to say "server does not support SSL" or
> something like that. I could imagine an admin spending a long time looking
> for an "enable channel binding" option in server settings, not realizing
> that it's actually "ssl=off" that's the problem.

It really depends on the connection context as a v10 server allows SSL
without channel binding, so just saying that server does not support SSL
is not correct either.  It seems to me that if you want an operator to
take correct actions then we want to see if the backend connected to is
a v10 server or something newer, or actually uses SSL in the connection
context.  If that's a v10 or older, then libpq should still complain
about channel binding not supported.  If it is a v11 or newer, then
libpq should complain about SSL not being supported.  By adding the
cross-option check in connectOptions2, the error message about SSL not
being supported is moot if requiring sslmode=require for
channel_binding_mode=require.  I have added a more complicated logic in
pg_SASL_init() for that (grep for "server does not support channel
binding") as it looks safer to me to have that for future sanity checks,
so please let me know your thoughts.  And it could be perfectly possible
that an attacking server tries to make the client think that it is a v11
server but tries to downgrade to SCRAM without channel binding.

Still, the tests reach their limit here, as you would normally bump into
HBA entries which don't match normally, still it is easy to add a test
which checks that sslmode!=require fails with scram_channel_binding=require.

> As the patch stands, if the server is configured for "trust" authentication,
> and the client uses "scram_channel_binding_mode=require", you get this
> error:
> 
> psql: channel binding required for authentication but SASL exchange is not
> finished
> 
> "SASL exchange is not finished" is quite technical. Can we have something
> like "... but server was configured for \"trust\" authentication"?

OK, here is a suggestion then as in the attached:
"channel binding required, but the server requested authentication \"trust\""

> So, in general, would be good to go through the different combinations of
> scram_channel_binding_mode, sslmode, server configured for SSL or not,
> server configured for different authentication methods, one more time. To
> make sure the error message in each case makes sense, and points to what the
> admin needs to change to make it work.

In the updated patch attached, you get the following failures with all
those configurations (making sslmode=require a requirement with
channel_binding_mode=require reduces the set of combinations a 

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Wed, Jun 06, 2018 at 11:53:06PM +0300, Heikki Linnakangas wrote:
> That would certainly be good. We've always had that problem, even with md5
> -> plaintext password downgrade, and it would be nice to fix it. It's quite
> late in the release cycle already, do you think we should address that now?
> I could go either way..

I would be inclined to treat that as new development as this is no new
problem.  Still that's linked with what is discussed on this thread with
scram_channel_bindin_mode.

> What should the option look like? Perhaps something like:
> 
> allowed_authentication_methods=md5,SCRAM-SHA-256,SCRAM-SHA-256-PLUS

That's actually a discussion I had with somebody after my talk at
PGCon, and I suggested a comma-separate list of authorized protocols as
well, except that those could just map to the hba entries, and that each
entry could just be lower-case :)
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 06/06/18 23:31, Peter Eisentraut wrote:

On 6/6/18 16:26, Heikki Linnakangas wrote:

On 06/06/18 23:20, Peter Eisentraut wrote:

Aren't we attacking this on the wrong level?  We are here attempting to
prevent a SCRAM-SHA-256-PLUS -> SCRAM-SHA-256 downgrade, but we are not
preventing a SCRAM-SHA-256-PLUS -> anything-else downgrade.


The latest patch does prevent that, too. That was my complaint at
https://www.postgresql.org/message-id/030284cc-d1d6-ce88-b677-a814f61c1880%40iki.fi,
but it's been fixed now. (Or if you see a case where it still isn't,
that's a bug.)


OK, that would do, but we don't do anything about a SCRAM-SHA-256 ->
anything-else downgrade.  Instead of tying this to the channel binding,
should we tie it to the authentication type?


That would certainly be good. We've always had that problem, even with 
md5 -> plaintext password downgrade, and it would be nice to fix it. 
It's quite late in the release cycle already, do you think we should 
address that now? I could go either way..


What should the option look like? Perhaps something like:

allowed_authentication_methods=md5,SCRAM-SHA-256,SCRAM-SHA-256-PLUS

That would not be very user-friendly, though.

- Heikki

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 05/06/18 09:41, Michael Paquier wrote:

On Sat, Jun 02, 2018 at 01:08:56PM -0400, Heikki Linnakangas wrote:

On 28/05/18 15:08, Michael Paquier wrote:

On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote:
+  printfPQExpBuffer(>errorMessage,
+libpq_gettext("channel binding required for authentication but invalid protocol 
used\n"));


If I understand correctly, you get this error, e.g. if you have "password"
or "sspi" in pg_hba.conf, and the client uses
"channel_binding_mode=require". "Invalid protocol" doesn't sound right for
that. Perhaps "channel binding required, but the server requested %s
authentication".


Right, even "md5" enters in this category if the user has a MD5
verifier, but not if it has a SCRAM verifier.  I have done that with
get_auth_request_str(), which maps authentication requests to the
probable hba entry.  It feels like cheating to map "trust" to
AUTH_REQ_OK as all methods use it as final message though, even if it is
not triggered by this patch.  So I have used no mapping name for it.


Ok. Perhaps add a comment pointing out that as the code stands, 
get_auth_request_str() is never called with AUTH_REQ_OK. So that if 
someone starts calling it with that, maybe they'll know to revisit this.




+ 
+  prefer (default)
+  
+   
+Use channel binding if available.  If the cluster does not
+support it, then it is not used.  This is the default.
+   
+  
+ 


I'd suggest s/cluster/server/. Here, and elsewhere in the docs changes.


There are some funny behaviors with different compinations of 
"scram_channel_binding_mode=require" and different sslmode settings. For 
example, with sslmode=disable, the client will attempt to connect, but 
it's guaranteed to fail at authentication, because channel binding is 
only possible with SSL. Perhaps it should throw an error without even 
attempting to connect? Or at least, the "server does not support channel 
binding" is misleading, as it was the client that insisted on an 
impossible combination; the server might well support channel binding, 
if SSL was used.


And with sslmode=allow, if the server allows a non-SSL connection, then 
the client won't use SSL, and will fail, as with sslmode=disable. But if 
the server requires SSL, then it will work. Perhaps sslmode=allow should 
be "promoted" to "sslmode=require", if 
scram_channel_binding_mode=require? Or don't let the user select a silly 
combination like that at all, and throw an error.


If scram_channel_binding_mode=require, but the server doesn't support 
SSL at all, you get:


psql: server does not support channel binding, and 
scram_channel_binding_mode=require was used


Perhaps it would be more clear to say "server does not support SSL" or 
something like that. I could imagine an admin spending a long time 
looking for an "enable channel binding" option in server settings, not 
realizing that it's actually "ssl=off" that's the problem.


As the patch stands, if the server is configured for "trust" 
authentication, and the client uses 
"scram_channel_binding_mode=require", you get this error:


psql: channel binding required for authentication but SASL exchange is 
not finished


"SASL exchange is not finished" is quite technical. Can we have 
something like "... but server was configured for \"trust\" authentication"?



So, in general, would be good to go through the different combinations 
of scram_channel_binding_mode, sslmode, server configured for SSL or 
not, server configured for different authentication methods, one more 
time. To make sure the error message in each case makes sense, and 
points to what the admin needs to change to make it work.


- Heikki

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

On 6/6/18 16:26, Heikki Linnakangas wrote:
> On 06/06/18 23:20, Peter Eisentraut wrote:
>> Aren't we attacking this on the wrong level?  We are here attempting to
>> prevent a SCRAM-SHA-256-PLUS -> SCRAM-SHA-256 downgrade, but we are not
>> preventing a SCRAM-SHA-256-PLUS -> anything-else downgrade.
> 
> The latest patch does prevent that, too. That was my complaint at 
> https://www.postgresql.org/message-id/030284cc-d1d6-ce88-b677-a814f61c1880%40iki.fi,
>  
> but it's been fixed now. (Or if you see a case where it still isn't, 
> that's a bug.)

OK, that would do, but we don't do anything about a SCRAM-SHA-256 ->
anything-else downgrade.  Instead of tying this to the channel binding,
should we tie it to the authentication type?

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

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 06/06/18 23:20, Peter Eisentraut wrote:

Aren't we attacking this on the wrong level?  We are here attempting to
prevent a SCRAM-SHA-256-PLUS -> SCRAM-SHA-256 downgrade, but we are not
preventing a SCRAM-SHA-256-PLUS -> anything-else downgrade.


The latest patch does prevent that, too. That was my complaint at 
https://www.postgresql.org/message-id/030284cc-d1d6-ce88-b677-a814f61c1880%40iki.fi, 
but it's been fixed now. (Or if you see a case where it still isn't, 
that's a bug.)


- Heikki

Re: SCRAM with channel binding downgrade attack

[Peter Eisentraut]

Aren't we attacking this on the wrong level?  We are here attempting to
prevent a SCRAM-SHA-256-PLUS -> SCRAM-SHA-256 downgrade, but we are not
preventing a SCRAM-SHA-256-PLUS -> anything-else downgrade.

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

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, Jun 02, 2018 at 01:08:56PM -0400, Heikki Linnakangas wrote:
> On 28/05/18 15:08, Michael Paquier wrote:
>> On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote:
>> > Sounds good.
>> 
>> Okay.  Done this way as attached.  If the backend forces anything else
>> than SCRAM then the client gets an immediate error if channel binding is
>> required, without waiting for the password prompt.
> 
> Thanks! The comments and error messages need some copy-editing:

Thanks Heikki for the input.  I have reworked all the points you raised
in the attached.

>>  /*
>>   * Select the mechanism to use.  Pick SCRAM-SHA-256-PLUS over 
>> anything
>>   * else if a channel binding mode is not 'disable'.  Pick 
>> SCRAM-SHA-256
>>   * if nothing else has already been picked.  If we add more 
>> mechanisms, a
>>   * more refined priority mechanism might become necessary.
>>   */
> 
> "else if a channel binding mode is not 'disable'" sounds a bit awkward. A
> double negative. (Also, "a" -> "the")

Okay.  I have completely rewritten this block.  Hopefully that's clearer.

>> +/*
>> + * If client is willing to enforce the use the channel binding but
>> + * it has not been previously selected, because it was either not
>> + * published by the server or could not be selected, then complain
>> + * back.  If SSL is not used for this connection, still complain
>> + * similarly, as the client may want channel binding but forgot
>> + * to set it up to do so which could be the case with sslmode=prefer
>> + * for example.  This protects from any kind of downgrade attacks
>> + * from rogue servers attempting to bypass channel binding.
>> + */
> 
> Instead of "is willing to enforce the use the channel binding", perhaps
> simply "requires channel binding".

Check.

>> +  printfPQExpBuffer(>errorMessage,
>> +libpq_gettext("channel binding required for SASL authentication but no 
>> valid mechanism could be selected\n"));
> 
> Channel binding is a property of SCRAM authentication specifically, it's not
> applicable to other SASL mechanisms. So I'd suggest something like:
> 
> "server does not support channel binding, and channel_binding_mode=require
> was used"

Changed as such, except that this is using scram_channel_binding_mode in
the error message.

>> +/*
>> + * Complain if channel binding mechanism is chosen but no channel
>> + * binding type is defined.
>> + */
>> +if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0 &&
>> +conn->scram_channel_binding_type == NULL)
>> +{
>> +printfPQExpBuffer(>errorMessage,
>> + libpq_gettext("SASL authentication mechanism using channel 
>> binding supported but no channel binding type defined\n"));
>> +goto error;
>> +}
> 
> It's not immediately clear to me from the error message or the comment, when
> this error is thrown. Can this even happen?

No, let's not make this happen then.  I have added NULL handling in
connectOptions2 related to the initialization of the options so both
scram_channel_binding_mode and scram_channel_binding_type will never be
NULL like sslmode.

>> +/*
>> + * Before processing any message, perform security-related sanity
>> + * checks.  If the client is willing to perform channel binding with
>> + * SCRAM authentication, only a subset of messages can be used so
>> + * as there is no transmission of any password data or such.
>> + */
> 
> I'd suggest something like:
> 
> "If SCRAM with channel binding is required, refuse all other authentication
> methods. Requiring channel binding implies that the client doesn't trust the
> server, until the SCRAM authentication is completed, so it's important that
> we don't divulge the plaintext password, or perform some weaker
> authentication, even if the server asks for it. In all other authentication
> methods, it's currently assumed that the client trusts the server, either
> implicitly or because the SSL certificate was already verified during the
> SSL handshake."

Check.  Thanks for the suggestion.

>> +  printfPQExpBuffer(>errorMessage,
>> +libpq_gettext("channel binding required for authentication but invalid 
>> protocol used\n"));
> 
> If I understand correctly, you get this error, e.g. if you have "password"
> or "sspi" in pg_hba.conf, and the client uses
> "channel_binding_mode=require". "Invalid protocol" doesn't sound right for
> that. Perhaps "channel binding required, but the server requested %s
> authentication".

Right, even "md5" enters in this category if the user has a MD5
verifier, but not if it has a SCRAM verifier.  I have done that with 
get_auth_request_str(), which maps authentication requests to the
probable hba entry.  It feels like cheating to map "trust" to
AUTH_REQ_OK as all methods use it as final message though, even if it is  
not triggered by this patch.  So I 

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 28/05/18 15:08, Michael Paquier wrote:

On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote:

Sounds good.


Okay.  Done this way as attached.  If the backend forces anything else
than SCRAM then the client gets an immediate error if channel binding is
required, without waiting for the password prompt.


Thanks! The comments and error messages need some copy-editing:


/*
 * Select the mechanism to use.  Pick SCRAM-SHA-256-PLUS over 
anything
 * else if a channel binding mode is not 'disable'.  Pick 
SCRAM-SHA-256
 * if nothing else has already been picked.  If we add more 
mechanisms, a
 * more refined priority mechanism might become necessary.
 */


"else if a channel binding mode is not 'disable'" sounds a bit awkward. 
A double negative. (Also, "a" -> "the")



+   /*
+* If client is willing to enforce the use the channel binding but
+* it has not been previously selected, because it was either not
+* published by the server or could not be selected, then complain
+* back.  If SSL is not used for this connection, still complain
+* similarly, as the client may want channel binding but forgot
+* to set it up to do so which could be the case with sslmode=prefer
+* for example.  This protects from any kind of downgrade attacks
+* from rogue servers attempting to bypass channel binding.
+*/


Instead of "is willing to enforce the use the channel binding", perhaps 
simply "requires channel binding".



+  printfPQExpBuffer(>errorMessage,
+libpq_gettext("channel binding required for SASL authentication but no valid 
mechanism could be selected\n"));


Channel binding is a property of SCRAM authentication specifically, it's 
not applicable to other SASL mechanisms. So I'd suggest something like:


"server does not support channel binding, and 
channel_binding_mode=require was used"



+   /*
+* Complain if channel binding mechanism is chosen but no channel
+* binding type is defined.
+*/
+   if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0 &&
+   conn->scram_channel_binding_type == NULL)
+   {
+   printfPQExpBuffer(>errorMessage,
+ libpq_gettext("SASL authentication 
mechanism using channel binding supported but no channel binding type defined\n"));
+   goto error;
+   }


It's not immediately clear to me from the error message or the comment, 
when this error is thrown. Can this even happen?



+   /*
+* Before processing any message, perform security-related sanity
+* checks.  If the client is willing to perform channel binding with
+* SCRAM authentication, only a subset of messages can be used so
+* as there is no transmission of any password data or such.
+*/


I'd suggest something like:

"If SCRAM with channel binding is required, refuse all other 
authentication methods. Requiring channel binding implies that the 
client doesn't trust the server, until the SCRAM authentication is 
completed, so it's important that we don't divulge the plaintext 
password, or perform some weaker authentication, even if the server asks 
for it. In all other authentication methods, it's currently assumed that 
the client trusts the server, either implicitly or because the SSL 
certificate was already verified during the SSL handshake."



+  printfPQExpBuffer(>errorMessage,
+libpq_gettext("channel binding required for authentication but invalid protocol 
used\n"));


If I understand correctly, you get this error, e.g. if you have 
"password" or "sspi" in pg_hba.conf, and the client uses 
"channel_binding_mode=require". "Invalid protocol" doesn't sound right 
for that. Perhaps "channel binding required, but the server requested %s 
authentication". Is it possible to have an error hint here? Perhaps add 
"HINT: change the authentication method to scram-sha-256 in the server's 
pg_hba.conf file".


- Heikki

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 28/05/18 12:20, Michael Paquier wrote:

On Mon, May 28, 2018 at 12:00:33PM +0300, Heikki Linnakangas wrote:

That's not a new problem, but it makes the MITM protection fairly pointless,
if a fake server can acquire the user's password by simply asking for it.
The client will report a failed connection, but with the user's password,
Mallory won't need to act as a MITM anymore.


Yeah, I know..  Do you think that it would be better to add an extra
switch/case at the beginning of pg_fe_sendauth which filters and checks
per message types then?


Sounds good.

- Heikki

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Mon, May 28, 2018 at 12:00:33PM +0300, Heikki Linnakangas wrote:
> That's not a new problem, but it makes the MITM protection fairly pointless,
> if a fake server can acquire the user's password by simply asking for it.
> The client will report a failed connection, but with the user's password,
> Mallory won't need to act as a MITM anymore.

Yeah, I know..  Do you think that it would be better to add an extra
switch/case at the beginning of pg_fe_sendauth which filters and checks
per message types then?
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 28/05/18 04:23, Michael Paquier wrote:

On Sat, May 26, 2018 at 11:42:38PM +0900, Michael Paquier wrote:

On Sat, May 26, 2018 at 09:08:50AM -0400, Bruce Momjian wrote:

On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:


OK, I can live with that as well.  So we'll go in the direction of two
parameters then:
- scram_channel_binding, which can use "prefer" (default), "require" or
"disable".
- scram_channel_binding_name, developer option to choose the type of
channel binding, with "tls-unique" (default) and "tls-server-end-point".
We could also remove the prefix "scram_".  Ideas of names are welcome.


scram_channel_binding_method?


Or scram_channel_binding_type.  The first sentence of RFC 5929 uses this
term.


I just went with scram_channel_binding_mode (require, disable and
prefer) and scram_channel_binding_type as parameter names, in the shape
of the attached patch.


Thanks! Getting better.

There's one pretty fatal bug in this patch: If you use 
"scram_channel_binding=require", we only fail the connection after going 
through the motions of authenticating with whatever authentication the 
server asked for. That's bad, because it means that the client will 
merrily respond to a AUTH_REQ_PASSWORD request from the server, by 
sending the password in cleartext.


That's not a new problem, but it makes the MITM protection fairly 
pointless, if a fake server can acquire the user's password by simply 
asking for it. The client will report a failed connection, but with the 
user's password, Mallory won't need to act as a MITM anymore.


- Heikki

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, May 26, 2018 at 11:42:38PM +0900, Michael Paquier wrote:
> On Sat, May 26, 2018 at 09:08:50AM -0400, Bruce Momjian wrote:
>> On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
>>>
>>> OK, I can live with that as well.  So we'll go in the direction of two
>>> parameters then:
>>> - scram_channel_binding, which can use "prefer" (default), "require" or
>>> "disable".
>>> - scram_channel_binding_name, developer option to choose the type of
>>> channel binding, with "tls-unique" (default) and "tls-server-end-point".
>>> We could also remove the prefix "scram_".  Ideas of names are welcome.
>> 
>> scram_channel_binding_method?
> 
> Or scram_channel_binding_type.  The first sentence of RFC 5929 uses this
> term.

I just went with scram_channel_binding_mode (require, disable and
prefer) and scram_channel_binding_type as parameter names, in the shape
of the attached patch.

>> Do we really know someone is going to want to actually specify the
>> channel binding type?  If it is only testing, maybe we don't need to
>> document this parameter.
> 
> Keeping everything documented is useful as well for new developers, as
> they need to guess less from the code.  So I would prefer seeing both
> connection parameters documented, and mentioning directly in the docs if
> a parameter is for developers or not.

So done this way.  Feel free to pick me up at PGcon this week if you
wish to discuss this issue.  Docs, tests and a commit message are
added.
--
Michael
From 34b68db28172458f69c59488a613d848939838a3 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 22 May 2018 17:03:48 +0900
Subject: [PATCH] Rework scram_channel_binding to protect from downgrade
 attacks

When a client attempts to connect to a PostgreSQL cluster, it may be
possible that it requested channel binding with SCRAM authentication,
but that the server tricks the cluster and forcibly downgrades the
authentication request.  For example, a v10 cluster supports SCRAM but
not channel binding so authentication could be achieved without channel
binding used.

In order to protect from such attacks, rework libpq handling of the
connection parameter scram_channel_binding as follows by splitting it
into two pieces:
- scram_channel_binding_mode, which can use as values "require",
"disable" or "prefer".
- scram_channel_binding_type, which is similar to the previous
scram_channel_binding, except that it does not accept anymore an empty
value to mean that channel binding is disabled.

"prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used. This
does not protect from downgrade attacks.
"disable", which is the equivalent of the empty value previously,
disables channel binding for all exchanges.
"require" makes channel binding a hard requirement for the
authentication.

For "require", if the cluster does not support channel binding with
SCRAM (v10 server) or if SSL is not used, then the connection is refused
by libpq, which is something that can happen if SSL is not used
(prevention also for users forgetting to use sslmode=require at least in
connection strings).  This also allows users to enforce the use of SCRAM
with channel binding even if the targeted cluster downgrades to "trust"
or similar.

In order to achieve that, when receiving AUTH_REQ_OK from the server, we
check if a SASL exchange has happened and has finished, where the client
makes sure that the server knows the connection proof.  If the cluster
does not publish the -PLUS mechanism, then connection is also refused.

Discussion: https://postgr.es/m/20180519123557.gb2...@paquier.xyz
---
 doc/src/sgml/libpq.sgml  | 72 
 doc/src/sgml/release-11.sgml |  2 +-
 src/interfaces/libpq/fe-auth-scram.c | 45 +
 src/interfaces/libpq/fe-auth.c   | 54 +++--
 src/interfaces/libpq/fe-auth.h   |  1 +
 src/interfaces/libpq/fe-connect.c| 51 +---
 src/interfaces/libpq/libpq-int.h |  3 +-
 src/test/ssl/ServerSetup.pm  | 27 ++-
 src/test/ssl/t/002_scram.pl  | 53 
 9 files changed, 257 insertions(+), 51 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 498b8df988..90e522b2af 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1238,24 +1238,76 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
- 
-  scram_channel_binding
+ 
+  scram_channel_binding_mode
+  
+   
+This option determines whether channel binding should be used
+with SCRAM authentication.  While
+SCRAM alone prevents the replay of transmitted
+hashed passwords, channel binding also prevents man-in-the-middle
+attacks.  There are three modes:
+
+
+ 
+  disable
+  
+   
+Disable the use of channel binding 

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, May 26, 2018 at 09:08:50AM -0400, Bruce Momjian wrote:
> On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
>>
>> OK, I can live with that as well.  So we'll go in the direction of two
>> parameters then:
>> - scram_channel_binding, which can use "prefer" (default), "require" or
>> "disable".
>> - scram_channel_binding_name, developer option to choose the type of
>> channel binding, with "tls-unique" (default) and "tls-server-end-point".
>> We could also remove the prefix "scram_".  Ideas of names are welcome.
> 
> scram_channel_binding_method?

Or scram_channel_binding_type.  The first sentence of RFC 5929 uses this
term.

> Do we really know someone is going to want to actually specify the
> channel binding type?  If it is only testing, maybe we don't need to
> document this parameter.

Keeping everything documented is useful as well for new developers, as
they need to guess less from the code.  So I would prefer seeing both
connection parameters documented, and mentioning directly in the docs if
a parameter is for developers or not.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
> On Fri, May 25, 2018 at 06:24:07PM +0300, Heikki Linnakangas wrote:
> > On 25 May 2018 17:44:16 EEST, Robert Haas  wrote:
> >> It seems to me that this is really another sort of thing altogether.
> >> Whether or not you want to insist on channel binding is a completely
> >> separate thing from which channel binding methods you're willing to
> >> use.  It seems to me like the most logical thing would be to make
> >> these two separate connection options. 
> > 
> > Works for me.
> 
> OK, I can live with that as well.  So we'll go in the direction of two
> parameters then:
> - scram_channel_binding, which can use "prefer" (default), "require" or
> "disable".
> - scram_channel_binding_name, developer option to choose the type of
> channel binding, with "tls-unique" (default) and "tls-server-end-point".
> We could also remove the prefix "scram_".  Ideas of names are welcome.

scram_channel_binding_method?

> At the end, the core of the proposed patch relies on the fact that it
> checks when receiving AUTH_REQ_OK that a full set of SCRAM messages has
> happened with the server, up to the point where the client has checked
> the server proof that both ends know the same password.  So do people
> here think that this is a sane apprach?  Are there other ideas?

Do we really know someone is going to want to actually specify the
channel binding type?  If it is only testing, maybe we don't need to
document this parameter.

-- 
  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: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, May 25, 2018 at 06:24:07PM +0300, Heikki Linnakangas wrote:
> On 25 May 2018 17:44:16 EEST, Robert Haas  wrote:
>> It seems to me that this is really another sort of thing altogether.
>> Whether or not you want to insist on channel binding is a completely
>> separate thing from which channel binding methods you're willing to
>> use.  It seems to me like the most logical thing would be to make
>> these two separate connection options. 
> 
> Works for me.

OK, I can live with that as well.  So we'll go in the direction of two
parameters then:
- scram_channel_binding, which can use "prefer" (default), "require" or
"disable".
- scram_channel_binding_name, developer option to choose the type of
channel binding, with "tls-unique" (default) and "tls-server-end-point".
We could also remove the prefix "scram_".  Ideas of names are welcome.

At the end, the core of the proposed patch relies on the fact that it
checks when receiving AUTH_REQ_OK that a full set of SCRAM messages has
happened with the server, up to the point where the client has checked
the server proof that both ends know the same password.  So do people
here think that this is a sane apprach?  Are there other ideas?
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 25 May 2018 17:44:16 EEST, Robert Haas  wrote:
>On Wed, May 23, 2018 at 2:46 AM, Heikki Linnakangas 
>wrote:
>> We could provide "tls-unique" and "tls-server-end-point" in addition
>to
>> those, but I'd consider those to be developer only settings, useful
>only for
>> testing the protocol.
>
>It seems to me that this is really another sort of thing altogether.
>Whether or not you want to insist on channel binding is a completely
>separate thing from which channel binding methods you're willing to
>use.  It seems to me like the most logical thing would be to make
>these two separate connection options. 

Works for me.

- Heikki

Re: SCRAM with channel binding downgrade attack

[Robert Haas]

On Wed, May 23, 2018 at 2:46 AM, Heikki Linnakangas  wrote:
> "tls-unique" and "tls-server-end-point" are overly technical to users. They
> don't care which one is used, there's no difference in security.
> Furthermore, if we add another channel binding type in the future, perhaps
> because someone finds a vulnerability in those types and a new one is added
> to address it, users would surely like to be automatically switched over to
> the new binding type. If they have "tls-unique" hardcoded in connection
> strings, that won't happen.

+1.

> So I think the options should be "prefer", "disable", and "require". That's
> also symmetrical with sslmode, which is nice.

+1.

> We could provide "tls-unique" and "tls-server-end-point" in addition to
> those, but I'd consider those to be developer only settings, useful only for
> testing the protocol.

It seems to me that this is really another sort of thing altogether.
Whether or not you want to insist on channel binding is a completely
separate thing from which channel binding methods you're willing to
use.  It seems to me like the most logical thing would be to make
these two separate connection options.  If it's discovered that
tls-unique sucks bigtime for some reason, people are going to want to
turn it off whether they are preferring channel binding or requiring
it.

> It would be nice to have a libpq setting like "authenticate_server=require",
> which would mean "I want man-in-the-middle protection". With that, a
> connection would be allowed, if either the server's SSL certificate is
> verified as with "sslmode=verify-full", *or* SCRAM authentication with
> channel binding was used. Or perhaps cram it into sslmode,
> "sslmode=verify-full-or-scram-channel-binding", just with a nicer name. (We
> can do that after v11 though, I think.)

IMHO we could do all of this after v11.  This seems like late
development being jammed through after beta1 to me.  But I just work
here.

Semantically, I see the value of an option that means basically
mitm_protection=true, but in practice I'm not sure there is any real
advantage over having the user just specify either ssmode=verify-full
or channelbinding=require depending on the technology they wish to
use.  They probably have a specific technology in mind; it's hard to
see how they're going to get an environment configured otherwise.

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

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Wed, May 23, 2018 at 06:41:16PM -0400, Bruce Momjian wrote:
> I can imagine someone wanting both checks so merging them into a single
> options seems unwise, as Magnus mentioned.

FWIW, definitely agreed.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Wed, May 23, 2018 at 11:15:28AM +0200, Magnus Hagander wrote:
> On Wed, May 23, 2018 at 11:08 AM, Heikki Linnakangas  wrote:
> SCRAM, even without channel binding, does prove that you're talking to the
> correct server. Or to be precise, it proves to the client, that the server
> also knows the password, so assuming that you're using strong passwords 
> and
> not sharing them across servers, you know that you're talking to the
> correct server.
> 
> Right. It provides a very different guarantee from what ssl certs provide. 
> They
> are not replaceable, or mutually exclusive. Trying to force those into a 
> single
> configuration parameter doesn't make a lot of sense IMO.

True.  sslmode is checking the the SSL endpoint with which you have a
shared secret has access to the private key of a server certificate that
is signed by a trusted CA, and perhaps the certificate's subject name
also matches the hostname.

With channel binding, you are proving that the SSL endpoint with which
you have a shared secret has access to the user password hash.

I can imagine someone wanting both checks so merging them into a single
options seems unwise, as Magnus mentioned.

-- 
  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: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Wed, May 23, 2018 at 11:08 AM, Heikki Linnakangas 
wrote:

> On 23/05/18 09:59, Magnus Hagander wrote:
>
>> With that, a connection would be allowed, if either the server's SSL
>>> certificate is verified as with "sslmode=verify-full", *or* SCRAM
>>> authentication with channel binding was used. Or perhaps cram it into
>>> sslmode, "sslmode=verify-full-or-scram-channel-binding", just with a
>>> nicer name. (We can do that after v11 though, I think.)
>>>
>>
>> sslmode=verify-full is very different from SCRAM with channel binding,
>> isn't it? As in, SCRAM with channel binding at no point proves which
>> server
>> you're talking to -- only that you are talking to the SSL endpoint? It
>> could be a rogue SSL endpoint unless you do certificate validation.
>>
>
> SCRAM, even without channel binding, does prove that you're talking to the
> correct server. Or to be precise, it proves to the client, that the server
> also knows the password, so assuming that you're using strong passwords and
> not sharing them across servers, you know that you're talking to the
> correct server.
>

Right. It provides a very different guarantee from what ssl certs provide.
They are not replaceable, or mutually exclusive. Trying to force those into
a single configuration parameter doesn't make a lot of sense IMO.


Channel binding adds the guarantee that the SSL endpoint belongs to the
> same server you're authenticating with, i.e. there is no man in the middle.


 Yeah, it does protect you against things like pgbouncer (a real one or a
rogue one- the rogue one being the mitm attacker). But again, only if you
never share a password, which would be a nice world to live in :)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Wed, May 23, 2018 at 05:56:19PM +0900, Michael Paquier wrote:
> OK, being able to introduce a new default if necessary is a good point.
> Let's introduce a "require" mode then, which uses tls-unique
> underground, while "tls-unique" and "tls-server-end-point" are
> documented as developer-oriented.

By the way, if somebody could review the latest version of the patch
before I write a new version and agrees with the concept introduced
would be nice..  Adding one option is simple enough, making sure that we
agree that the patch is on good tracks is harder.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 23/05/18 09:59, Magnus Hagander wrote:

With that, a connection would be allowed, if either the server's SSL
certificate is verified as with "sslmode=verify-full", *or* SCRAM
authentication with channel binding was used. Or perhaps cram it into
sslmode, "sslmode=verify-full-or-scram-channel-binding", just with a
nicer name. (We can do that after v11 though, I think.)


sslmode=verify-full is very different from SCRAM with channel binding,
isn't it? As in, SCRAM with channel binding at no point proves which server
you're talking to -- only that you are talking to the SSL endpoint? It
could be a rogue SSL endpoint unless you do certificate validation.


SCRAM, even without channel binding, does prove that you're talking to 
the correct server. Or to be precise, it proves to the client, that the 
server also knows the password, so assuming that you're using strong 
passwords and not sharing them across servers, you know that you're 
talking to the correct server.


Channel binding adds the guarantee that the SSL endpoint belongs to the 
same server you're authenticating with, i.e. there is no man in the middle.


- Heikki

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Wed, May 23, 2018 at 10:56 AM, Michael Paquier 
wrote:

> On Wed, May 23, 2018 at 08:59:43AM +0200, Magnus Hagander wrote:
> > On Wed, May 23, 2018 at 8:46 AM, Heikki Linnakangas 
> wrote:
> >> "tls-unique" and "tls-server-end-point" are overly technical to users.
> >> They don't care which one is used, there's no difference in security.
> >> Furthermore, if we add another channel binding type in the future,
> perhaps
> >> because someone finds a vulnerability in those types and a new one is
> added
> >> to address it, users would surely like to be automatically switched
> over to
> >> the new binding type. If they have "tls-unique" hardcoded in connection
> >> strings, that won't happen.
> >>
> >> So I think the options should be "prefer", "disable", and "require".
> >> That's also symmetrical with sslmode, which is nice.
>
> OK, being able to introduce a new default if necessary is a good point.
> Let's introduce a "require" mode then, which uses tls-unique
> underground, while "tls-unique" and "tls-server-end-point" are
> documented as developer-oriented.


> > As a general point, I still don't like being symmetrical with
> > sslmode=prefer, because that's a silly setting for sslmode :) It
> basically
> > says "I don't care about the security guarantees, I just want the
> overhead
> > please". Now, granted, the over head for SCRAM channel-binding is
> certainly
> > a lot less than it is for TLS. But if we just want to go with symmetry,
> it
> > would perhaps make more sense to implement the "allow" mode which makes
> > more sense on the TLS side as well.
>
> Something that you may be forgetting here is that we still want to be
> able to connect to a v10 backend with default options even with a
> post-v11 libpq.  Or we will get complaints.
>

Right. Having a mode called "allow" and having that be default would work
fine in this case, wouldn't it?

(That is, my suggestion is to implement "allow", "disable" and "require",
and to make "allow" the default)


>> It would be nice to have a libpq setting like
> >> "authenticate_server=require", which would mean "I want
> man-in-the-middle
> >> protection".
> >
> > That seems like a bad name for such a thing. Shouldn't it be
> > "authenticate_server=no_man_in_the_middle" (not those words but you get
> the
> > idea). Specifically, protecting against man in the middle attack does not
> > equal authenticating server -- you can still be connected to the wrong
> > server just with no second attacker between you and the first
> > attacker.
>
> Still that's not something we want for v11, right?
>

Agreed.


>> With that, a connection would be allowed, if either the server's SSL
> >> certificate is verified as with "sslmode=verify-full", *or* SCRAM
> >> authentication with channel binding was used. Or perhaps cram it into
> >> sslmode, "sslmode=verify-full-or-scram-channel-binding", just with a
> >> nicer name. (We can do that after v11 though, I think.)
> >
> > sslmode=verify-full is very different from SCRAM with channel binding,
> > isn't it? As in, SCRAM with channel binding at no point proves which
> server
> > you're talking to -- only that you are talking to the SSL endpoint? It
> > could be a rogue SSL endpoint unless you do certificate validation.
>
> And the reverse is true as well, say the CA is compromised..
>

Well, sure. But scram channel binding doesn't protect you there, so you're
screwed either way if that happens.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Wed, May 23, 2018 at 08:59:43AM +0200, Magnus Hagander wrote:
> On Wed, May 23, 2018 at 8:46 AM, Heikki Linnakangas  wrote:
>> "tls-unique" and "tls-server-end-point" are overly technical to users.
>> They don't care which one is used, there's no difference in security.
>> Furthermore, if we add another channel binding type in the future, perhaps
>> because someone finds a vulnerability in those types and a new one is added
>> to address it, users would surely like to be automatically switched over to
>> the new binding type. If they have "tls-unique" hardcoded in connection
>> strings, that won't happen.
>>
>> So I think the options should be "prefer", "disable", and "require".
>> That's also symmetrical with sslmode, which is nice.

OK, being able to introduce a new default if necessary is a good point.
Let's introduce a "require" mode then, which uses tls-unique
underground, while "tls-unique" and "tls-server-end-point" are
documented as developer-oriented.

> As a general point, I still don't like being symmetrical with
> sslmode=prefer, because that's a silly setting for sslmode :) It basically
> says "I don't care about the security guarantees, I just want the overhead
> please". Now, granted, the over head for SCRAM channel-binding is certainly
> a lot less than it is for TLS. But if we just want to go with symmetry, it
> would perhaps make more sense to implement the "allow" mode which makes
> more sense on the TLS side as well.

Something that you may be forgetting here is that we still want to be
able to connect to a v10 backend with default options even with a
post-v11 libpq.  Or we will get complaints.

>> It would be nice to have a libpq setting like
>> "authenticate_server=require", which would mean "I want man-in-the-middle
>> protection".
> 
> That seems like a bad name for such a thing. Shouldn't it be
> "authenticate_server=no_man_in_the_middle" (not those words but you get the
> idea). Specifically, protecting against man in the middle attack does not
> equal authenticating server -- you can still be connected to the wrong
> server just with no second attacker between you and the first
> attacker.

Still that's not something we want for v11, right?

>> With that, a connection would be allowed, if either the server's SSL
>> certificate is verified as with "sslmode=verify-full", *or* SCRAM
>> authentication with channel binding was used. Or perhaps cram it into
>> sslmode, "sslmode=verify-full-or-scram-channel-binding", just with a
>> nicer name. (We can do that after v11 though, I think.)
> 
> sslmode=verify-full is very different from SCRAM with channel binding,
> isn't it? As in, SCRAM with channel binding at no point proves which server
> you're talking to -- only that you are talking to the SSL endpoint? It
> could be a rogue SSL endpoint unless you do certificate validation.

And the reverse is true as well, say the CA is compromised..
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Magnus Hagander]

On Wed, May 23, 2018 at 8:46 AM, Heikki Linnakangas  wrote:

> On 22/05/18 11:22, Michael Paquier wrote:
>
>> On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
>>
>>> The previous patch has actually problems with servers using "trust",
>>> "password" and any protocol which can send directly AUTH_REQ_OK as those
>>> could still enforce a downgrade to not use channel binding, so we
>>> actually need to make sure that AUTH_REQ_SASL_FIN has been received when
>>> channel binding is required when looking at a AUTH_REQ_OK message.
>>>
>>
>> Okay, so I have digested the previous comments with the attached.
>> scram_channel_binding is modified as follows (from commit message):
>> - "prefer", is the default and behaves so as channel binding is used if
>> available.  If the cluster does not support it then it is not used. This
>> does not protect from downgrade attacks.
>> - "disable", which is the equivalent of the empty value previously,
>> disables channel binding.
>> - "tls-unique" and "tls-server-end-point" make channel binding a
>> requirement and use the channel binding of the same name for
>> connection.  Note that in this case, if the connection is attempted
>> without SSL or if server does not support channel binding then libpq
>> refuses the connection as well.
>>
>
> "tls-unique" and "tls-server-end-point" are overly technical to users.
> They don't care which one is used, there's no difference in security.
> Furthermore, if we add another channel binding type in the future, perhaps
> because someone finds a vulnerability in those types and a new one is added
> to address it, users would surely like to be automatically switched over to
> the new binding type. If they have "tls-unique" hardcoded in connection
> strings, that won't happen.
>
> So I think the options should be "prefer", "disable", and "require".
> That's also symmetrical with sslmode, which is nice.
>

As a general point, I still don't like being symmetrical with
sslmode=prefer, because that's a silly setting for sslmode :) It basically
says "I don't care about the security guarantees, I just want the overhead
please". Now, granted, the over head for SCRAM channel-binding is certainly
a lot less than it is for TLS. But if we just want to go with symmetry, it
would perhaps make more sense to implement the "allow" mode which makes
more sense on the TLS side as well.


We could provide "tls-unique" and "tls-server-end-point" in addition to
> those, but I'd consider those to be developer only settings, useful only
> for testing the protocol.
>
> It would be nice to have a libpq setting like
> "authenticate_server=require", which would mean "I want man-in-the-middle
> protection".


That seems like a bad name for such a thing. Shouldn't it be
"authenticate_server=no_man_in_the_middle" (not those words but you get the
idea). Specifically, protecting against man in the middle attack does not
equal authenticating server -- you can still be connected to the wrong
server just with no second attacker between you and the first attacker.



> With that, a connection would be allowed, if either the server's SSL
> certificate is verified as with "sslmode=verify-full", *or* SCRAM
> authentication with channel binding was used. Or perhaps cram it into
> sslmode, "sslmode=verify-full-or-scram-channel-binding", just with a
> nicer name. (We can do that after v11 though, I think.)


sslmode=verify-full is very different from SCRAM with channel binding,
isn't it? As in, SCRAM with channel binding at no point proves which server
you're talking to -- only that you are talking to the SSL endpoint? It
could be a rogue SSL endpoint unless you do certificate validation.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: SCRAM with channel binding downgrade attack

[Heikki Linnakangas]

On 22/05/18 11:22, Michael Paquier wrote:

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:

The previous patch has actually problems with servers using "trust",
"password" and any protocol which can send directly AUTH_REQ_OK as those
could still enforce a downgrade to not use channel binding, so we
actually need to make sure that AUTH_REQ_SASL_FIN has been received when
channel binding is required when looking at a AUTH_REQ_OK message.


Okay, so I have digested the previous comments with the attached.
scram_channel_binding is modified as follows (from commit message):
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used. This
does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.  Note that in this case, if the connection is attempted
without SSL or if server does not support channel binding then libpq
refuses the connection as well.


"tls-unique" and "tls-server-end-point" are overly technical to users. 
They don't care which one is used, there's no difference in security. 
Furthermore, if we add another channel binding type in the future, 
perhaps because someone finds a vulnerability in those types and a new 
one is added to address it, users would surely like to be automatically 
switched over to the new binding type. If they have "tls-unique" 
hardcoded in connection strings, that won't happen.


So I think the options should be "prefer", "disable", and "require". 
That's also symmetrical with sslmode, which is nice.


We could provide "tls-unique" and "tls-server-end-point" in addition to 
those, but I'd consider those to be developer only settings, useful only 
for testing the protocol.


It would be nice to have a libpq setting like 
"authenticate_server=require", which would mean "I want 
man-in-the-middle protection". With that, a connection would be allowed, 
if either the server's SSL certificate is verified as with 
"sslmode=verify-full", *or* SCRAM authentication with channel binding 
was used. Or perhaps cram it into sslmode, 
"sslmode=verify-full-or-scram-channel-binding", just with a nicer name. 
(We can do that after v11 though, I think.)



In order to make sure that a client is not tricked by a "trust"
connection downgrade which sends back directly AUTH_REQ_OK, one way I
have thought about is to check if the client has achieved with a server
a full SASL exchange when receiving this message type, which is
something that we know about as the exchange state is saved in
PGconn->sasl_state.


It'd be nice if the client could tell the server that it requires 
authentication, so that the server would go through the SCRAM 
authentication even with "trust". With channel binding, SCRAM not only 
authenticates the client to the server, but also the server to the 
client. But that's not urgent, I think we can live without it for v11.


- Heikki

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Tue, May 22, 2018 at 08:19:34PM -0400, Bruce Momjian wrote:
> Since tls-unique and tls-server-end-point provide the same level of
> security, I don't see any value in allowing prefer-tls-server-end-point,
> as you stated above.

Thanks.  We are on the same line for this portion then.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Tue, May 22, 2018 at 05:22:19PM +0900, Michael Paquier wrote:
> On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> > The previous patch has actually problems with servers using "trust",
> > "password" and any protocol which can send directly AUTH_REQ_OK as those
> > could still enforce a downgrade to not use channel binding, so we
> > actually need to make sure that AUTH_REQ_SASL_FIN has been received when 
> > channel binding is required when looking at a AUTH_REQ_OK message.
> 
> Okay, so I have digested the previous comments with the attached.
> scram_channel_binding is modified as follows (from commit message): 
> - "prefer", is the default and behaves so as channel binding is used if
> available.  If the cluster does not support it then it is not used. This
> does not protect from downgrade attacks.
> - "disable", which is the equivalent of the empty value previously,
> disables channel binding.

Yes, I never liked the 'empty value' idea since I don't know of any
other libpq settings that use that API.  "disable" matches "sslmode"
too, which is nice.

> In order to make sure that a client is not tricked by a "trust"
> connection downgrade which sends back directly AUTH_REQ_OK, one way I
> have thought about is to check if the client has achieved with a server
> a full SASL exchange when receiving this message type, which is
> something that we know about as the exchange state is saved in
> PGconn->sasl_state.

I had not thought of 'trust'.  I was more worried about the password
hash being downgraded in robustness or passed through a
man-in-the-middle, while the 'trust' does not require.  However, you are
right that channel binding, when required, does require the other end to
know the same password as the client knows.  Good point.

-- 
  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: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> On Fri, May 18, 2018 at 09:30:22AM -0400, Bruce Momjian wrote:
> > Good work, but I think the existance of both scram_channel_binding and
> > channel_binding_mode in libpq is confusing.  I am thinking we should
> > have one channel binding parameter that can take values "prefer",
> > "tls-unique", "tls-server-end-point", and "require".  I don't see the
> > point to having "none" and "allow" that sslmode supports.   "tls-unique"
> > and "tls-server-end-point" would _require_ those channel binding modes; 
> > the setting would never be ignored, e.g. if no SSL.
> 
> I can see the point you are coming at.  Do you think that we should
> worry about users willing to use specifically tls-server-end-point
> (which is something actually in the backend protocol only for JDBC) and
> manage a cluster of both v10 and v11 servers?  In this case we assume
> that "prefer" means always using tls-unique as channel binding, but
> there is no way to say "I would prefer channel binding if available,
> only with end-point".  It would not be that hard to let the application
> layer on top of libpq handle that complexity with channel binding
> handling, though it makes the life of libpq consumers a bit harder.

This is going to be hard enough so let's do whatever we can to simplify
it.

> Hence, I would actually eliminate "require", as that would be actually
> the same as "tls-unique", and the possibility to use an empty value from
> the list of options available but add "none" as that's actually the same
> as the current empty value.  This gives as list:
> - tls-unique
> - tls-server-end-point
> - none
> - prefer, which has the meaning of preferring tls-unique
> - And prefer-end-point?  But thinking why end-point has been added it
> would not be worth it, and for tests only the option requiring end-point
> is enough.

Since tls-unique and tls-server-end-point provide the same level of
security, I don't see any value in allowing prefer-tls-server-end-point,
as you stated above.

-- 
  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: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
> The previous patch has actually problems with servers using "trust",
> "password" and any protocol which can send directly AUTH_REQ_OK as those
> could still enforce a downgrade to not use channel binding, so we
> actually need to make sure that AUTH_REQ_SASL_FIN has been received when 
> channel binding is required when looking at a AUTH_REQ_OK message.

Okay, so I have digested the previous comments with the attached.
scram_channel_binding is modified as follows (from commit message): 
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used. This
does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.  Note that in this case, if the connection is attempted
without SSL or if server does not support channel binding then libpq
refuses the connection as well.

In order to make sure that a client is not tricked by a "trust"
connection downgrade which sends back directly AUTH_REQ_OK, one way I
have thought about is to check if the client has achieved with a server
a full SASL exchange when receiving this message type, which is
something that we know about as the exchange state is saved in
PGconn->sasl_state.

The patch includes documentation and tests, which check that connections
are refused without SSL and or if the server downgrades authentication
requests.

Thanks,
--
Michael
From 5bf51e7bdcfaf2d6e8af5132bb7884bc307f440b Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 22 May 2018 17:03:48 +0900
Subject: [PATCH] Rework scram_channel_binding to protect from downgrade
 attacks

When a client attempts to connect to a PostgreSQL cluster, it may be
possible that it requested channel binding with SCRAM authentication,
but that the server tricks the clister and forcibly downgrades the
authentication request.  For example, a v10 cluster supports SCRAM but
not channel binding so authentication could be achieved without channel
binding used.

In order to protect from such attacks, rework libpq handling of the
connection parameter scram_channel_binding as follows:
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used.
This does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.

For both "tls-unique" and "tsl-server-end-point, if the cluster does not
support channel binding with SCRAM (v10 server) or if SSL is not used,
then the connection is refused by libpq, which is something that can
happen if SSL is not used (prevention also for users forgetting to use
sslmode=require at least in connection strings).  This also allows users
to enforce the use of SCRAM with channel binding even if the targetted
cluster downgrades to "trust" or similar.

In order to achieve that, when receiving AUTH_REQ_OK from the server, we
check if a SASL exchange has happened and has finished, where the client
makes sure that the server knows the connection proof.  If the cluster
does not publish the -PLUS mechanism, then connection is also refused.

Discussion: https://postgr.es/m/20180519123557.gb2...@paquier.xyz
---
 doc/src/sgml/libpq.sgml  | 34 +++-
 src/interfaces/libpq/fe-auth-scram.c | 59 ++--
 src/interfaces/libpq/fe-auth.c   | 57 +--
 src/interfaces/libpq/fe-auth.h   |  4 +-
 src/interfaces/libpq/fe-connect.c| 20 +-
 src/test/ssl/ServerSetup.pm  | 22 +++
 src/test/ssl/t/002_scram.pl  | 38 --
 7 files changed, 198 insertions(+), 36 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 498b8df988..f1fa744a8b 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1250,18 +1250,34 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 

 The list of channel binding types supported by the server are
-listed in .  An empty value
-specifies that the client will not use channel binding.  If this
-parameter is not specified, tls-unique is used,
-if supported by both server and client.
-Channel binding is only supported on SSL connections.  If the
-connection is not using SSL, then this setting is ignored.
+listed in .  A value of
+disable disables channel binding completely,
+hence the client decides to not use it even if the server supports
+it.  A value of prefer, the default, means that
+  

Re: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, May 18, 2018 at 09:30:22AM -0400, Bruce Momjian wrote:
> Good work, but I think the existance of both scram_channel_binding and
> channel_binding_mode in libpq is confusing.  I am thinking we should
> have one channel binding parameter that can take values "prefer",
> "tls-unique", "tls-server-end-point", and "require".  I don't see the
> point to having "none" and "allow" that sslmode supports.   "tls-unique"
> and "tls-server-end-point" would _require_ those channel binding modes; 
> the setting would never be ignored, e.g. if no SSL.

I can see the point you are coming at.  Do you think that we should
worry about users willing to use specifically tls-server-end-point
(which is something actually in the backend protocol only for JDBC) and
manage a cluster of both v10 and v11 servers?  In this case we assume
that "prefer" means always using tls-unique as channel binding, but
there is no way to say "I would prefer channel binding if available,
only with end-point".  It would not be that hard to let the application
layer on top of libpq handle that complexity with channel binding
handling, though it makes the life of libpq consumers a bit harder.

Hence, I would actually eliminate "require", as that would be actually
the same as "tls-unique", and the possibility to use an empty value from
the list of options available but add "none" as that's actually the same
as the current empty value.  This gives as list:
- tls-unique
- tls-server-end-point
- none
- prefer, which has the meaning of preferring tls-unique
- And prefer-end-point?  But thinking why end-point has been added it
would not be worth it, and for tests only the option requiring end-point
is enough.

The previous patch has actually problems with servers using "trust",
"password" and any protocol which can send directly AUTH_REQ_OK as those
could still enforce a downgrade to not use channel binding, so we
actually need to make sure that AUTH_REQ_SASL_FIN has been received when 
channel binding is required when looking at a AUTH_REQ_OK message.
--
Michael


signature.asc
Description: PGP signature

Re: SCRAM with channel binding downgrade attack

[Bruce Momjian]

On Fri, May 18, 2018 at 12:03:49PM +0900, Michael Paquier wrote:
> On Fri, May 18, 2018 at 10:46:46AM +0900, Michael Paquier wrote:
> > From a security point of view, 1) is important for libpq, but I am not
> > much enthusiast about 2) as a whole.  The backend has proper support for
> > channel binding, hence other drivers speaking the protocol could have
> > their own restriction mechanisms.
> 
> So, I have been playing with libpq to address point 1), and added a new
> connection parameter called channel_binding_mode which can be set to
> 'prefer', which is what libpq uses now, and 'require'.  The patch has
> two important parts:

Good work, but I think the existance of both scram_channel_binding and
channel_binding_mode in libpq is confusing.  I am thinking we should
have one channel binding parameter that can take values "prefer",
"tls-unique", "tls-server-end-point", and "require".  I don't see the
point to having "none" and "allow" that sslmode supports.   "tls-unique"
and "tls-server-end-point" would _require_ those channel binding modes; 
the setting would never be ignored, e.g. if no SSL.

-- 
  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: SCRAM with channel binding downgrade attack

[Michael Paquier]

On Fri, May 18, 2018 at 10:46:46AM +0900, Michael Paquier wrote:
> From a security point of view, 1) is important for libpq, but I am not
> much enthusiast about 2) as a whole.  The backend has proper support for
> channel binding, hence other drivers speaking the protocol could have
> their own restriction mechanisms.

So, I have been playing with libpq to address point 1), and added a new
connection parameter called channel_binding_mode which can be set to
'prefer', which is what libpq uses now, and 'require'.  The patch has
two important parts:
1) If a server does not support channel binding, still it is sending
back a SCRAM authentication, but the client still wants to enforce the
use of channel binding, then libpq reacts as follows:
$ psql -d "dbname=foo channel_binding_mode=require"
psql: channel binding required for SASL authentication but no valid
mechanism could be selected
This requires pg_SASL_init() to be patched after the SASL mechanism has
been selected.  That error can be triggered with a v10 server with whom
a SCRAM authentication is done, as well as with a v11 server where SSL
is not used.  Some people may use sslmode=prefer in combination to
channel_binding_mode=require, in which case an error should be raised if
the SSL connection cannot be achieved first.  That addresses a bit of
the craziness of sslmode=prefer...
2) If client wants to use channel binding, but the server is trying to
enforce another protocol than SCRAM, like MD5, trust, gssapi or such,
then the following error happens:
$ psql -d "dbname=foo channel_binding_mode=require"
psql: channel binding required for authentication but no valid protocol are used
In this case, it seems to me that the best bet is to patch
pg_fe_sendauth() and filter by message types.

In the attached, I have added the parameter and some documentation.  I
have not added tests, but some things could be tested in the SSL suite:
- Check for incorrect values in the parameter.
- Test connection without SCRAM with "require"
- Test connection without SSL but SCRAM with "require"
I have not put much thoughts into the documentation, but the patch is
rather simple so hopefully that helps in making progress in the
discussion.
--
Michael
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 498b8df988..aedc65b63f 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1238,6 +1238,49 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  channel_binding_mode
+  
+   
+This option determines whether channel binding should be used for
+authentication or not.  As of now, channel binding is only supported
+with SCRAM authentication.  There are two modes:
+
+
+ 
+  prefer (default)
+  
+   
+Try to use channel binding if available but rely on the server
+depending on what types of SASL mechanisms are published during
+the message exchange.  This is the default, and it can be
+considered as insecure as this does not protect from servers
+attempting downgrade authentication attacks to a client.
+   
+  
+ 
+
+ 
+  require
+  
+   
+Reject any connection attempts to a server if channel binding
+is not supported by the protocol used.  Channel binding being
+supported only for SCRAM in the context of an SSL connection,
+a connection with this option enabled would fail except in this
+authentication context.  This protects from downgrade attacks
+and ensures that a SCRAM connection with channel binding is
+able to transparently achieve MITM protection with
+libpq.
+   
+  
+ 
+
+   
+
+  
+ 
+
  
   scram_channel_binding
   
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 3b2073a47f..e23ab83c1b 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -547,6 +547,25 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 		goto error;
 	}
 
+	/*
+	 * If client is willing to enforce the use the channel binding but
+	 * it has not been previously selected, because it was either not
+	 * published by the server or could not be selected, then complain
+	 * back.  If SSL is not used for this connection, still complain
+	 * similarly, as the client may want channel binding but forgot
+	 * to set it up to do so which could be the case with sslmode=prefer
+	 * for example.  This protects from any kind of downgrade attacks
+	 * from rogue servers attempting to bypass channel binding.
+	 */
+	if (conn->channel_binding_mode &&
+		strcmp(conn->channel_binding_mode, "require") == 0 &&
+		strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
+	{
+		printfPQExpBuffer(>errorMessage,
+		  libpq_gettext("channel binding required for SASL