Minor cleanup of backend SCRAM code.

hlinnaka · hlinnaka · commit 00707fa58275 · 2017-04-13T17:44:15.000+03:00
Free each SASL message after sending it. It's not a lot of wasted memory, and it's short-lived, but the authentication code in general tries to pfree() stuff, so let's follow the example. Adding the pfree() revealed a little bug in build_server_first_message(). It attempts to keeps a copy of the sent message, but it was missing a pstrdup(), so the pointer started to dangle, after adding the pfree() into CheckSCRAMAuth(). Reword comments and debug messages slightly, while we're at it. Reviewed by Michael Paquier. Discussion: https://www.postgresql.org/message-id/6490b975-5ee1-6280-ac1d-af975b19fb9a@iki.fi
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
@@ -161,10 +161,10 @@ static char *scram_MockSalt(const char *username);
  * needs to be called before doing any exchange.  It will be filled later
  * after the beginning of the exchange with verifier data.
  *
- * 'username' is the provided by the client.  'shadow_pass' is the role's
- * password verifier, from pg_authid.rolpassword.  If 'shadow_pass' is NULL, we
- * still perform an authentication exchange, but it will fail, as if an
- * incorrect password was given.
+ * 'username' is the username provided by the client in the startup message.
+ * 'shadow_pass' is the role's password verifier, from pg_authid.rolpassword.
+ * If 'shadow_pass' is NULL, we still perform an authentication exchange, but
+ * it will fail, as if an incorrect password was given.
  */
 void *
 pg_be_scram_init(const char *username, const char *shadow_pass)
@@ -984,7 +984,7 @@ build_server_first_message(scram_state *state)
 				 state->client_nonce, state->server_nonce,
 				 state->salt, state->iterations);
 
-	return state->server_first_message;
+	return pstrdup(state->server_first_message);
 }
 
 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
@@ -872,6 +872,8 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 					strlen(SCRAM_SHA256_NAME) + 1);
 
 	/*
+	 * Initialize the status tracker for message exchanges.
+	 *
 	 * If the user doesn't exist, or doesn't have a valid password, or it's
 	 * expired, we still go through the motions of SASL authentication, but
 	 * tell the authentication method that the authentication is "doomed".
@@ -880,8 +882,6 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 	 * This is because we don't want to reveal to an attacker what usernames
 	 * are valid, nor which users have a valid password.
 	 */
-
-	/* Initialize the status tracker for message exchanges */
 	scram_opaq = pg_be_scram_init(port->user_name, shadow_pass);
 
 	/*
@@ -918,7 +918,7 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			return STATUS_ERROR;
 		}
 
-		elog(DEBUG4, "Processing received SASL token of length %d", buf.len);
+		elog(DEBUG4, "Processing received SASL response of length %d", buf.len);
 
 		/*
 		 * we pass 'logdetail' as NULL when doing a mock authentication,
@@ -931,14 +931,16 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 		/* input buffer no longer used */
 		pfree(buf.data);
 
-		if (outputlen > 0)
+		if (output)
 		{
 			/*
 			 * Negotiation generated data to be sent to the client.
 			 */
-			elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+			elog(DEBUG4, "sending SASL challenge of length %u", outputlen);
 
 			sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
+
+			pfree(output);
 		}
 	} while (result == SASL_EXCHANGE_CONTINUE);
 
