pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedsome SCRAM read_any_attr() confusion
3+ messages / 2 participants
[nested] [flat]
* some SCRAM read_any_attr() confusion
@ 2019-08-17 08:11 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-08-17 12:57 ` Re: some SCRAM read_any_attr() confusion Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 3+ messages in thread
From: Peter Eisentraut @ 2019-08-17 08:11 UTC (permalink / raw)
To: pgsql-hackers
I was a bit confused by some of the comments around the SCRAM function
read_any_attr(), used to skip over extensions.
The comment "Returns NULL if there is attribute.", besides being
strangely worded, appears to be wrong anyway, because the function never
returns NULL.
This lead me to wonder how this loop would terminate if there is no "p"
attribute in the message:
/* ignore optional extensions */
do
{
proof = p - 1;
value = read_any_attr(&p, &attr);
} while (attr != 'p');
What actually happens is
ERROR: malformed SCRAM message
DETAIL: Attribute expected, but found invalid character "0x00".
which serves the purpose but was probably not quite intended that way.
I propose the attached patch to clean this up a bit, with better
comments and a better error message.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 48c38857cb5dbccbe512bb489a4a69214d03d2e4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 17 Aug 2019 10:04:11 +0200
Subject: [PATCH] Clean up some SCRAM attribute processing
Correct the comment for read_any_attr(). Give a clearer error message
when parsing at the end of the string, when the client-final-message
does not contain a "p" attribute (for some reason).
---
src/backend/libpq/auth-scram.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index aa918839fb..68792cb45e 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -790,7 +790,8 @@ sanitize_str(const char *s)
/*
* Read the next attribute and value in a SCRAM exchange message.
*
- * Returns NULL if there is attribute.
+ * The attribute character is set in *attr_p, the attribute value is the
+ * return value.
*/
static char *
read_any_attr(char **input, char *attr_p)
@@ -799,6 +800,12 @@ read_any_attr(char **input, char *attr_p)
char *end;
char attr = *begin;
+ if (attr == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("malformed SCRAM message"),
+ errdetail("Attribute expected, but found end of string.")));
+
/*------
* attr-val = ALPHA "=" value
* ;; Generic syntax of any attribute sent
@@ -1298,7 +1305,7 @@ read_client_final_message(scram_state *state, const char *input)
state->client_final_nonce = read_attr_value(&p, 'r');
- /* ignore optional extensions */
+ /* ignore optional extensions, read until we find "p" attribute */
do
{
proof = p - 1;
--
2.22.0
Attachments:
[text/plain] 0001-Clean-up-some-SCRAM-attribute-processing.patch (1.7K, ../../2fb8a15b-de35-682d-a77b-edcc9c52fa12@2ndquadrant.com/2-0001-Clean-up-some-SCRAM-attribute-processing.patch)
download | inline diff:
From 48c38857cb5dbccbe512bb489a4a69214d03d2e4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 17 Aug 2019 10:04:11 +0200
Subject: [PATCH] Clean up some SCRAM attribute processing
Correct the comment for read_any_attr(). Give a clearer error message
when parsing at the end of the string, when the client-final-message
does not contain a "p" attribute (for some reason).
---
src/backend/libpq/auth-scram.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index aa918839fb..68792cb45e 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -790,7 +790,8 @@ sanitize_str(const char *s)
/*
* Read the next attribute and value in a SCRAM exchange message.
*
- * Returns NULL if there is attribute.
+ * The attribute character is set in *attr_p, the attribute value is the
+ * return value.
*/
static char *
read_any_attr(char **input, char *attr_p)
@@ -799,6 +800,12 @@ read_any_attr(char **input, char *attr_p)
char *end;
char attr = *begin;
+ if (attr == '\0')
+ ereport(ERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("malformed SCRAM message"),
+ errdetail("Attribute expected, but found end of string.")));
+
/*------
* attr-val = ALPHA "=" value
* ;; Generic syntax of any attribute sent
@@ -1298,7 +1305,7 @@ read_client_final_message(scram_state *state, const char *input)
state->client_final_nonce = read_attr_value(&p, 'r');
- /* ignore optional extensions */
+ /* ignore optional extensions, read until we find "p" attribute */
do
{
proof = p - 1;
--
2.22.0
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: some SCRAM read_any_attr() confusion
2019-08-17 08:11 some SCRAM read_any_attr() confusion Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
@ 2019-08-17 12:57 ` Michael Paquier <michael@paquier.xyz>
2019-08-21 07:18 ` Re: some SCRAM read_any_attr() confusion Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 3+ messages in thread
From: Michael Paquier @ 2019-08-17 12:57 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers
On Sat, Aug 17, 2019 at 10:11:27AM +0200, Peter Eisentraut wrote:
> I was a bit confused by some of the comments around the SCRAM function
> read_any_attr(), used to skip over extensions.
>
> The comment "Returns NULL if there is attribute.", besides being
> strangely worded, appears to be wrong anyway, because the function never
> returns NULL.
This may have come from an incorrect merge with a past version when
this code has been developed. +1 for me for your suggestions and your
patch.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190817125750.GB6079@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: some SCRAM read_any_attr() confusion
2019-08-17 08:11 some SCRAM read_any_attr() confusion Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-08-17 12:57 ` Re: some SCRAM read_any_attr() confusion Michael Paquier <michael@paquier.xyz>
@ 2019-08-21 07:18 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 0 replies; 3+ messages in thread
From: Peter Eisentraut @ 2019-08-21 07:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 2019-08-17 14:57, Michael Paquier wrote:
> On Sat, Aug 17, 2019 at 10:11:27AM +0200, Peter Eisentraut wrote:
>> I was a bit confused by some of the comments around the SCRAM function
>> read_any_attr(), used to skip over extensions.
>>
>> The comment "Returns NULL if there is attribute.", besides being
>> strangely worded, appears to be wrong anyway, because the function never
>> returns NULL.
>
> This may have come from an incorrect merge with a past version when
> this code has been developed. +1 for me for your suggestions and your
> patch.
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2019-08-21 07:18 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-08-17 08:11 some SCRAM read_any_attr() confusion Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-08-17 12:57 ` Michael Paquier <michael@paquier.xyz>
2019-08-21 07:18 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox