From 00707fa58275e370dc445fa7e1130085aa04f37b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 13 Apr 2017 17:44:15 +0300 Subject: [PATCH] Minor cleanup of backend SCRAM code. 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 --- src/backend/libpq/auth-scram.c | 10 +++++----- src/backend/libpq/auth.c | 12 +++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 5077ff33b1..a47c48d980 100644 --- 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 index 66ead9381d..b4c98c45c9 100644 --- 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);