[Peter Eisentraut <peter.eisentraut@2ndquadrant.com>]

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