[Tom Lane <tgl@sss.pgh.pa.us>]

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Sep 07, 2018 at 04:18:55PM -0400, Tom Lane wrote:
>> I had been studying that when the report first arrived.  I'm not really
>> happy with the coding in pg_saslprep(): it leaves garbage in its output
>> parameter in some code paths, and I think it leaks memory in others.

> Agreed.  Do you want to give it a go?  I can see some of those code
> paths?  Likely you spotted more.

Looking closer, there are no leaks, though I think we could use a comment
about that.  And it still makes me itch that pg_saslprep (mostly) doesn't
worry about clearing *output on failure while only three of its four
callers bother to initialize their variables to null.  That's a recipe
for future bugs.  So I propose the attached cleanup.

We're still no closer to an explanation of Jeremy's failure, though
I'm now pretty sure that pg_saslprep itself isn't the issue.

			regards, tom lane

Attachments:

make-pg-saslprep-more-bulletproof.patch (text/x-diff, 2.3K) download
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8fbad53..e997c94 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -453,7 +453,7 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
 char *
 pg_be_scram_build_verifier(const char *password)
 {
-	char	   *prep_password = NULL;
+	char	   *prep_password;
 	pg_saslprep_rc rc;
 	char		saltbuf[SCRAM_DEFAULT_SALT_LEN];
 	char	   *result;
@@ -499,7 +499,7 @@ scram_verify_plain_password(const char *username, const char *password,
 	uint8		stored_key[SCRAM_KEY_LEN];
 	uint8		server_key[SCRAM_KEY_LEN];
 	uint8		computed_key[SCRAM_KEY_LEN];
-	char	   *prep_password = NULL;
+	char	   *prep_password;
 	pg_saslprep_rc rc;
 
 	if (!parse_scram_verifier(verifier, &iterations, &encoded_salt,
diff --git a/src/common/saslprep.c b/src/common/saslprep.c
index 2710215..4cf574f 100644
--- a/src/common/saslprep.c
+++ b/src/common/saslprep.c
@@ -1081,6 +1081,9 @@ pg_saslprep(const char *input, char **output)
 	unsigned char *p;
 	pg_wchar   *wp;
 
+	/* Ensure we return *output as NULL on failure */
+	*output = NULL;
+
 	/* Check that the password isn't stupendously long */
 	if (strlen(input) > MAX_PASSWORD_LENGTH)
 	{
@@ -1112,10 +1115,7 @@ pg_saslprep(const char *input, char **output)
 	 */
 	input_size = pg_utf8_string_len(input);
 	if (input_size < 0)
-	{
-		*output = NULL;
 		return SASLPREP_INVALID_UTF8;
-	}
 
 	input_chars = ALLOC((input_size + 1) * sizeof(pg_wchar));
 	if (!input_chars)
@@ -1246,6 +1246,11 @@ pg_saslprep(const char *input, char **output)
 	result = ALLOC(result_size + 1);
 	if (!result)
 		goto oom;
+
+	/*
+	 * There are no error exits below here, so the error exit paths don't need
+	 * to worry about possibly freeing "result".
+	 */
 	p = (unsigned char *) result;
 	for (wp = output_chars; *wp; wp++)
 	{
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 1d9c937..81531f3 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -747,7 +747,7 @@ verify_server_signature(fe_scram_state *state)
 char *
 pg_fe_scram_build_verifier(const char *password)
 {
-	char	   *prep_password = NULL;
+	char	   *prep_password;
 	pg_saslprep_rc rc;
 	char		saltbuf[SCRAM_DEFAULT_SALT_LEN];
 	char	   *result;