rdelaage/postgresql
1
0
Fork 0
mirror of https://git.postgresql.org/git/postgresql.git synced 2024-09-28 03:51:50 +02:00
Code Issues Packages Projects Releases Wiki Activity

Improve authentication error messages.

Browse Source 
Most of the improvements were in the new SCRAM code:

* In SCRAM protocol violation messages, use errdetail to provide the
  details.

* If pg_backend_random() fails, throw an ERROR rather than just LOG. We
  shouldn't continue authentication if we can't generate a random nonce.

* Use ereport() rather than elog() for the "invalid SCRAM verifier"
  messages. They shouldn't happen, if everything works, but it's not
  inconceivable that someone would have invalid scram verifiers in
  pg_authid, e.g. if a broken client application was used to generate the
  verifier.

But this change applied to old code:

* Use ERROR rather than COMMERROR for protocol violation errors. There's
  no reason to not tell the client what they did wrong. The client might be
  confused already, so that it cannot read and display the error correctly,
  but let's at least try. In the "invalid password packet size" case, we
  used to actually continue with authentication anyway, but that is now a
  hard error.

Patch by Michael Paquier and me. Thanks to Daniel Varrazzo for spotting
the typo in one of the messages that spurred the discussion and these
larger changes.

Discussion: https://www.postgresql.org/message-id/CA%2Bmi_8aZYLhuyQi1Jo0hO19opNZ2OEATEOM5fKApH7P6zTOZGg%40mail.gmail.com
This commit is contained in:
Heikki Linnakangas 2017-06-08 19:54:22 +03:00
parent 7ff9812f9a
commit e3df8f8b93
2 changed files with 44 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
src/backend/libpq/auth-scram.c
View File

@ -195,7 +195,8 @@ pg_be_scram_init(const char *username, const char *shadow_pass)
* The password looked like a SCRAM verifier, but could not be
* parsed.
*/
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
ereport(LOG,
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
got_verifier = false;
}
}
@ -283,11 +284,13 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
if (inputlen == 0)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (empty message)"))));
errmsg("malformed SCRAM message"),
errdetail("The message is empty.")));
if (inputlen != strlen(input))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (length mismatch)"))));
errmsg("malformed SCRAM message"),
errdetail("Message length does not match input length.")));
switch (state->state)
{
@ -319,7 +322,8 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
if (!verify_final_nonce(state))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("invalid SCRAM response (nonce mismatch)"))));
errmsg("invalid SCRAM response"),
errdetail("Nonce does not match.")));
/*
* Now check the final nonce and the client proof.
@ -391,14 +395,9 @@ pg_be_scram_build_verifier(const char *password)
/* Generate random salt */
if (!pg_backend_random(saltbuf, SCRAM_DEFAULT_SALT_LEN))
{
ereport(LOG,
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("could not generate random salt")));
if (prep_password)
pfree(prep_password);
return NULL;
}
result = scram_build_verifier(saltbuf, SCRAM_DEFAULT_SALT_LEN,
SCRAM_DEFAULT_ITERATIONS, password);
@ -435,7 +434,8 @@ scram_verify_plain_password(const char *username, const char *password,
/*
* The password looked like a SCRAM verifier, but could not be parsed.
*/
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
ereport(LOG,
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
return false;
}
@ -443,7 +443,8 @@ scram_verify_plain_password(const char *username, const char *password,
saltlen = pg_b64_decode(encoded_salt, strlen(encoded_salt), salt);
if (saltlen == -1)
{
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
ereport(LOG,
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
return false;
}
@ -582,14 +583,16 @@ read_attr_value(char **input, char attr)
if (*begin != attr)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (attribute '%c' expected, %s found)",
attr, sanitize_char(*begin)))));
errmsg("malformed SCRAM message"),
errdetail("Expected attribute '%c' but found %s.",
attr, sanitize_char(*begin))));
begin++;
if (*begin != '=')
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
errmsg("malformed SCRAM message"),
errdetail("Expected character = for attribute %c.", attr)));
begin++;
end = begin;
@ -669,8 +672,9 @@ read_any_attr(char **input, char *attr_p)
(attr >= 'a' && attr <= 'z')))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (attribute expected, invalid char %s found)",
sanitize_char(attr)))));
errmsg("malformed SCRAM message"),
errdetail("Attribute expected, but found invalid character %s.",
sanitize_char(attr))));
if (attr_p)
*attr_p = attr;
begin++;
@ -678,7 +682,8 @@ read_any_attr(char **input, char *attr_p)
if (*begin != '=')
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
errmsg("malformed SCRAM message"),
errdetail("Expected character = for attribute %c.", attr)));
begin++;
end = begin;
@ -795,14 +800,16 @@ read_client_first_message(scram_state *state, char *input)
default:
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (unexpected channel-binding flag %s)",
sanitize_char(*input)))));
errmsg("malformed SCRAM message"),
errdetail("Unexpected channel-binding flag %s.",
sanitize_char(*input))));
}
if (*input != ',')
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("malformed SCRAM message (comma expected, got %s)",
sanitize_char(*input))));
errmsg("malformed SCRAM message"),
errdetail("Comma expected, but found character %s.",
sanitize_char(*input))));
input++;
/*
@ -815,8 +822,9 @@ read_client_first_message(scram_state *state, char *input)
if (*input != ',')
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("malformed SCRAM message (unexpected attribute %s in client-first-message)",
sanitize_char(*input))));
errmsg("malformed SCRAM message"),
errdetail("Unexpected attribute %s in client-first-message.",
sanitize_char(*input))));
input++;
state->client_first_message_bare = pstrdup(input);
@ -831,7 +839,7 @@ read_client_first_message(scram_state *state, char *input)
if (*input == 'm')
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("client requires mandatory SCRAM extension")));
errmsg("client requires an unsupported SCRAM extension")));
/*
* Read username. Note: this is ignored. We use the username from the
@ -960,7 +968,7 @@ build_server_first_message(scram_state *state)
int encoded_len;
if (!pg_backend_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("could not generate random nonce")));
@ -1044,14 +1052,16 @@ read_client_final_message(scram_state *state, char *input)
if (pg_b64_decode(value, strlen(value), client_proof) != SCRAM_KEY_LEN)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (malformed proof in client-final-message"))));
errmsg("malformed SCRAM message"),
errdetail("Malformed proof in client-final-message.")));
memcpy(state->ClientProof, client_proof, SCRAM_KEY_LEN);
pfree(client_proof);
if (*p != '\0')
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("malformed SCRAM message (garbage at end of client-final-message)"))));
errmsg("malformed SCRAM message"),
errdetail("Garbage found at the end of client-final-message.")));
state->client_final_message_without_proof = palloc(proof - begin + 1);
memcpy(state->client_final_message_without_proof, input, proof - begin);

15
src/backend/libpq/auth.c
View File

@ -656,7 +656,7 @@ recv_password_packet(Port *port)
* log.
*/
if (mtype != EOF)
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected password response, got message type %d",
mtype)));
@ -684,7 +684,7 @@ recv_password_packet(Port *port)
* StringInfo is guaranteed to have an appended '\0'.
*/
if (strlen(buf.data) + 1 != buf.len)
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid password packet size")));
@ -897,11 +897,10 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
{
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected SASL response, got message type %d",
mtype)));
return STATUS_ERROR;
}
else
return STATUS_EOF;
@ -935,11 +934,9 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
selected_mech = pq_getmsgrawstring(&buf);
if (strcmp(selected_mech, SCRAM_SHA256_NAME) != 0)
{
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("client selected an invalid SASL authentication mechanism")));
pfree(buf.data);
return STATUS_ERROR;
}
inputlen = pq_getmsgint(&buf, 4);
@ -1144,7 +1141,7 @@ pg_GSS_recvauth(Port *port)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected GSS response, got message type %d",
mtype)));
@ -1384,7 +1381,7 @@ pg_SSPI_recvauth(Port *port)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
ereport(COMMERROR,
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected SSPI response, got message type %d",
mtype)));