Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
HI all,
When a client connects during a SCRAM exchange, it has multiple ways
to let the server know what the client supports or not when using
channel binding:
- "n" -> client doesn't support channel binding.
- "y" -> client does support channel binding but thinks the server does not.
- "p" -> client requires channel binding.
On a v10 client, we just need to use the "n" flag because the client
does not support channel binding. This way, a v10 client can connect
to a v10 or v11 server with or without SSL, and this even if the
server has published the SASL mechanism SCRAM-SHA-256-PLUS, which is
used to define channel binding use during SCRAM authentication.
With a v11 client though, things are more fancy:
- If the server publishes the SCRAM-PLUS mechanism, then the client
replies with a "p" message. We are here in the context of an SSL
connection. This is the case of a v11 client, v11 server.
- If using an SSL connection, and the server did not publish
SCRAM-PLUS, then the client uses "y". This is the case of a v11 client
and v10 server.
- For a non-SSL connection, "n" is used. (The server would not have
sent the -PLUS mechanism anyway). This happens for all combinations
without SSL.
When using "n" or "y", the data sent by the client to the server about
the use of channel binding is a base64-encoded string of respectively
"n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
v10 server is able to allow connections with "n,,", but not with
"y,,":
/messages/by-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.com
When trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail. The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.
Thoughts?
--
Michael
Attachments:
scram-channel-binding-pg10.patchtext/x-patch; charset=US-ASCII; name=scram-channel-binding-pg10.patchDownload
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 9161c885e1..e7423cba51 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1033,10 +1033,12 @@ read_client_final_message(scram_state *state, char *input)
/*
* Read channel-binding. We don't support channel binding, so it's
- * expected to always be "biws", which is "n,,", base64-encoded.
+ * expected to always be "biws", which is "n,,", or 'eSws', which is
+ * "y,,", base64-encoded.
*/
channel_binding = read_attr_value(&p, 'c');
- if (strcmp(channel_binding, "biws") != 0)
+ if (strcmp(channel_binding, "biws") != 0 &&
+ strcmp(channel_binding, "eSws") != 0)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("unexpected SCRAM channel-binding attribute in client-final-message"))));
Michael Paquier <michael.paquier@gmail.com> writes:
When trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail.
That's bad ...
The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.
This is not an acceptable fix. We have to maintain the ability to connect
to unpatched older servers. If features added to HEAD have broken that,
either we fix those features to be backwards compatible, or we revert
them.
regards, tom lane
(Adding Heikki here because that concerns him as well)
On Mon, Nov 20, 2017 at 2:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.This is not an acceptable fix. We have to maintain the ability to connect
to unpatched older servers. If features added to HEAD have broken that,
either we fix those features to be backwards compatible, or we revert
them.
Well, when doing the SASL exchange the client does not know what is
the backend version. If we'd know that it would then be possible to
enforce a "n" flag conditionally from the client. So in order to be
backward-compatible we could send unconditionally a "n" flag from a
v11 client even in an SSL context. But that would not actually be true
because the client is able to support channel binding in this case, so
we would make libpq not RFC-compliant.
v10 has not been out for a long time, so this can plead in favor of a
change, and SCRAM is not spread yet. It is really unfortunate that we
did not notice that during at the moment SCRAM has been implemented.
That's clearly a bug of v10.
Honestly I am of the opinion to do a proper fix now and have things in
a clean state which is RFC-compliant instead of maintaining for 10
years a backward-compatible change in libpq that would be only valid
for 10.0 and 10.1 (assuming that the server-side fix is added in
10.2). Note that we need to fix the v10 server anyway with something
like the patch upthread. The enforcement of this "n" flag can just
happen in a v11-client.
--
Michael
On 11/19/17 23:08, Michael Paquier wrote:
When using "n" or "y", the data sent by the client to the server about
the use of channel binding is a base64-encoded string of respectively
"n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
v10 server is able to allow connections with "n,,", but not with
"y,,":
/messages/by-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.comWhen trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail. The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.
I noticed what I think is an omission in the current v11 code. We also
need to check whether the channel binding flag (n/y/p) encoded in the
client-final-message is the same one used in the client-first-message.
See attached patch. This would also affect what we might end up
backpatching.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Check-channel-binding-flag-at-end-of-SCRAM-exchange.patchtext/plain; charset=UTF-8; name=0001-Check-channel-binding-flag-at-end-of-SCRAM-exchange.patch; x-mac-creator=0; x-mac-type=0Download
From 7db5626b0dd082ec782188759316fb345df37999 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Nov 2017 14:02:57 -0500
Subject: [PATCH] Check channel binding flag at end of SCRAM exchange
We need to check whether the channel-binding flag encoded in the
client-final-message is the same one sent in the client-first-message.
---
src/backend/libpq/auth-scram.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 22103ce479..bfde6b746d 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -110,6 +110,7 @@ typedef struct
const char *username; /* username from startup packet */
+ char cbind_flag;
bool ssl_in_use;
const char *tls_finished_message;
size_t tls_finished_len;
@@ -788,6 +789,7 @@ read_client_first_message(scram_state *state, char *input)
* Read gs2-cbind-flag. (For details see also RFC 5802 Section 6 "Channel
* Binding".)
*/
+ state->cbind_flag = *input;
switch (*input)
{
case 'n':
@@ -1108,6 +1110,8 @@ read_client_final_message(scram_state *state, char *input)
char *b64_message;
int b64_message_len;
+ Assert(state->cbind_flag == 'p');
+
/*
* Fetch data appropriate for channel binding type
*/
@@ -1152,10 +1156,11 @@ read_client_final_message(scram_state *state, char *input)
/*
* If we are not using channel binding, the binding data is expected
* to always be "biws", which is "n,," base64-encoded, or "eSws",
- * which is "y,,".
+ * which is "y,,". We also have to check whether the flag is the same
+ * one that the client originally sent.
*/
- if (strcmp(channel_binding, "biws") != 0 &&
- strcmp(channel_binding, "eSws") != 0)
+ if (!(strcmp(channel_binding, "biws") == 0 && state->cbind_flag == 'n') &&
+ !(strcmp(channel_binding, "eSws") == 0 && state->cbind_flag == 'y'))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("unexpected SCRAM channel-binding attribute in client-final-message"))));
--
2.15.0
On Thu, Nov 23, 2017 at 4:08 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 11/19/17 23:08, Michael Paquier wrote:
When using "n" or "y", the data sent by the client to the server about
the use of channel binding is a base64-encoded string of respectively
"n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
v10 server is able to allow connections with "n,,", but not with
"y,,":
/messages/by-id/887b6fb7-15fe-239e-2aad-5911d2b0945b@2ndquadrant.comWhen trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail. The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.I noticed what I think is an omission in the current v11 code. We also
need to check whether the channel binding flag (n/y/p) encoded in the
client-final-message is the same one used in the client-first-message.
See attached patch. This would also affect what we might end up
backpatching.
Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value. I don't see much need for an assertion or
such.
--
Michael
On 11/22/17 21:08, Michael Paquier wrote:
Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value.
Could you clarify what comment you would like to have added or changed?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 11/22/17 21:08, Michael Paquier wrote:
Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value.Could you clarify what comment you would like to have added or changed?
Sure. Here is with the attached patch what I have in mind. The way
cbind-flag is assigned in the client-first message should be kept
in-sync with the way the client-final message builds the binding data
in c=. It could be possible to add more sanity-checks based on
assertions by keeping track of the cbind-flag assigned in the
client-first message as your upthread patch is doing in the backend
code, but I see a simple comment as a sufficient reminder.
--
Michael
Attachments:
fe-auth-scram-comment.patchtext/x-patch; charset=US-ASCII; name=fe-auth-scram-comment.patchDownload
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 97db0b1faa..7ef5cc437e 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -437,6 +437,8 @@ build_client_final_message(fe_scram_state *state, PQExpBuffer errormessage)
/*
* Construct client-final-message-without-proof. We need to remember it
* for verifying the server proof in the final step of authentication.
+ * This needs to be kept consistent with the cbind_flag handling when
+ * building the first client message in build_client_first_message().
*/
if (strcmp(state->sasl_mechanism, SCRAM_SHA256_PLUS_NAME) == 0)
{
On 11/30/17 00:36, Michael Paquier wrote:
On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:On 11/22/17 21:08, Michael Paquier wrote:
Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value.Could you clarify what comment you would like to have added or changed?
Sure. Here is with the attached patch what I have in mind. The way
cbind-flag is assigned in the client-first message should be kept
in-sync with the way the client-final message builds the binding data
in c=. It could be possible to add more sanity-checks based on
assertions by keeping track of the cbind-flag assigned in the
client-first message as your upthread patch is doing in the backend
code, but I see a simple comment as a sufficient reminder.
Committed with that comment, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 1, 2017 at 11:55 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 11/30/17 00:36, Michael Paquier wrote:
On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:On 11/22/17 21:08, Michael Paquier wrote:
Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value.Could you clarify what comment you would like to have added or changed?
Sure. Here is with the attached patch what I have in mind. The way
cbind-flag is assigned in the client-first message should be kept
in-sync with the way the client-final message builds the binding data
in c=. It could be possible to add more sanity-checks based on
assertions by keeping track of the cbind-flag assigned in the
client-first message as your upthread patch is doing in the backend
code, but I see a simple comment as a sufficient reminder.Committed with that comment, thanks.
Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch
then. This ensures that eSws is checked in the final message and that
the cbind-flag sent in the first message maps with the data of the
final message in the backend. I have checked with the following
configurations with a v10 backend:
- v11 libpq with SSL
- v11 libpq without SSL
- v10 libpq with SSL
- v10 libpq without SSL
And in all cases the connection is accepted as it should.
--
Michael
Attachments:
scram-channel-binding-pg10-v2.patchtext/x-patch; charset=US-ASCII; name=scram-channel-binding-pg10-v2.patchDownload
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 9161c885e1..7b87d1c14b 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -112,6 +112,8 @@ typedef struct
const char *username; /* username from startup packet */
+ char cbind_flag;
+
int iterations;
char *salt; /* base64-encoded */
uint8 StoredKey[SCRAM_KEY_LEN];
@@ -774,6 +776,7 @@ read_client_first_message(scram_state *state, char *input)
*/
/* read gs2-cbind-flag */
+ state->cbind_flag = *input;
switch (*input)
{
case 'n':
@@ -1033,10 +1036,13 @@ read_client_final_message(scram_state *state, char *input)
/*
* Read channel-binding. We don't support channel binding, so it's
- * expected to always be "biws", which is "n,,", base64-encoded.
+ * expected to always be "biws", which is "n,,", base64-encoded or
+ * "eSws", which is "y,,". We also have to check whether the flag is
+ * the same one that the client originally sent.
*/
channel_binding = read_attr_value(&p, 'c');
- if (strcmp(channel_binding, "biws") != 0)
+ if (!(strcmp(channel_binding, "biws") == 0 && state->cbind_flag == 'n') &&
+ !(strcmp(channel_binding, "eSws") == 0 && state->cbind_flag == 'y'))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("unexpected SCRAM channel-binding attribute in client-final-message"))));
On 12/1/17 18:11, Michael Paquier wrote:
Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch
then. This ensures that eSws is checked in the final message and that
the cbind-flag sent in the first message maps with the data of the
final message in the backend. I have checked with the following
configurations with a v10 backend:
- v11 libpq with SSL
- v11 libpq without SSL
- v10 libpq with SSL
- v10 libpq without SSL
And in all cases the connection is accepted as it should.
Committed.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Dec 9, 2017 at 12:23 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 12/1/17 18:11, Michael Paquier wrote:
Cool. Thanks. For REL_10_STABLE, I would suggest the attached patch
then. This ensures that eSws is checked in the final message and that
the cbind-flag sent in the first message maps with the data of the
final message in the backend. I have checked with the following
configurations with a v10 backend:
- v11 libpq with SSL
- v11 libpq without SSL
- v10 libpq with SSL
- v10 libpq without SSL
And in all cases the connection is accepted as it should.Committed.
Thanks.
--
Michael