On Fri, Dec 1, 2017 at 11:55 PM, Peter Eisentraut
<[email protected]> wrote:
> On 11/30/17 00:36, Michael Paquier wrote:
>> On Wed, Nov 29, 2017 at 1:04 AM, Peter Eisentraut
>> <[email protected]> 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
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"))));
