Skip to content

Commit e3df8f8

Browse files
hlinnakahlinnaka
committed
Improve authentication error messages.
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
1 parent 7ff9812 commit e3df8f8

File tree

2 files changed

+44
-37
lines changed

2 files changed

+44
-37
lines changed

‎src/backend/libpq/auth-scram.c

+38-28
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,8 @@ pg_be_scram_init(const char *username, const char *shadow_pass)
195195
* The password looked like a SCRAM verifier, but could not be
196196
* parsed.
197197
*/
198-
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
198+
ereport(LOG,
199+
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
199200
got_verifier = false;
200201
}
201202
}
@@ -283,11 +284,13 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
283284
if (inputlen == 0)
284285
ereport(ERROR,
285286
(errcode(ERRCODE_PROTOCOL_VIOLATION),
286-
(errmsg("malformed SCRAM message (empty message)"))));
287+
errmsg("malformed SCRAM message"),
288+
errdetail("The message is empty.")));
287289
if (inputlen != strlen(input))
288290
ereport(ERROR,
289291
(errcode(ERRCODE_PROTOCOL_VIOLATION),
290-
(errmsg("malformed SCRAM message (length mismatch)"))));
292+
errmsg("malformed SCRAM message"),
293+
errdetail("Message length does not match input length.")));
291294

292295
switch (state->state)
293296
{
@@ -319,7 +322,8 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
319322
if (!verify_final_nonce(state))
320323
ereport(ERROR,
321324
(errcode(ERRCODE_PROTOCOL_VIOLATION),
322-
(errmsg("invalid SCRAM response (nonce mismatch)"))));
325+
errmsg("invalid SCRAM response"),
326+
errdetail("Nonce does not match.")));
323327

324328
/*
325329
* Now check the final nonce and the client proof.
@@ -391,14 +395,9 @@ pg_be_scram_build_verifier(const char *password)
391395

392396
/* Generate random salt */
393397
if (!pg_backend_random(saltbuf, SCRAM_DEFAULT_SALT_LEN))
394-
{
395-
ereport(LOG,
398+
ereport(ERROR,
396399
(errcode(ERRCODE_INTERNAL_ERROR),
397400
errmsg("could not generate random salt")));
398-
if (prep_password)
399-
pfree(prep_password);
400-
return NULL;
401-
}
402401

403402
result = scram_build_verifier(saltbuf, SCRAM_DEFAULT_SALT_LEN,
404403
SCRAM_DEFAULT_ITERATIONS, password);
@@ -435,15 +434,17 @@ scram_verify_plain_password(const char *username, const char *password,
435434
/*
436435
* The password looked like a SCRAM verifier, but could not be parsed.
437436
*/
438-
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
437+
ereport(LOG,
438+
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
439439
return false;
440440
}
441441

442442
salt = palloc(pg_b64_dec_len(strlen(encoded_salt)));
443443
saltlen = pg_b64_decode(encoded_salt, strlen(encoded_salt), salt);
444444
if (saltlen == -1)
445445
{
446-
elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
446+
ereport(LOG,
447+
(errmsg("invalid SCRAM verifier for user \"%s\"", username)));
447448
return false;
448449
}
449450

@@ -582,14 +583,16 @@ read_attr_value(char **input, char attr)
582583
if (*begin != attr)
583584
ereport(ERROR,
584585
(errcode(ERRCODE_PROTOCOL_VIOLATION),
585-
(errmsg("malformed SCRAM message (attribute '%c' expected, %s found)",
586-
attr, sanitize_char(*begin)))));
586+
errmsg("malformed SCRAM message"),
587+
errdetail("Expected attribute '%c' but found %s.",
588+
attr, sanitize_char(*begin))));
587589
begin++;
588590

589591
if (*begin != '=')
590592
ereport(ERROR,
591593
(errcode(ERRCODE_PROTOCOL_VIOLATION),
592-
(errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
594+
errmsg("malformed SCRAM message"),
595+
errdetail("Expected character = for attribute %c.", attr)));
593596
begin++;
594597

595598
end = begin;
@@ -669,16 +672,18 @@ read_any_attr(char **input, char *attr_p)
669672
(attr >= 'a' && attr <= 'z')))
670673
ereport(ERROR,
671674
(errcode(ERRCODE_PROTOCOL_VIOLATION),
672-
(errmsg("malformed SCRAM message (attribute expected, invalid char %s found)",
673-
sanitize_char(attr)))));
675+
errmsg("malformed SCRAM message"),
676+
errdetail("Attribute expected, but found invalid character %s.",
677+
sanitize_char(attr))));
674678
if (attr_p)
675679
*attr_p = attr;
676680
begin++;
677681

678682
if (*begin != '=')
679683
ereport(ERROR,
680684
(errcode(ERRCODE_PROTOCOL_VIOLATION),
681-
(errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
685+
errmsg("malformed SCRAM message"),
686+
errdetail("Expected character = for attribute %c.", attr)));
682687
begin++;
683688

684689
end = begin;
@@ -795,14 +800,16 @@ read_client_first_message(scram_state *state, char *input)
795800
default:
796801
ereport(ERROR,
797802
(errcode(ERRCODE_PROTOCOL_VIOLATION),
798-
(errmsg("malformed SCRAM message (unexpected channel-binding flag %s)",
799-
sanitize_char(*input)))));
803+
errmsg("malformed SCRAM message"),
804+
errdetail("Unexpected channel-binding flag %s.",
805+
sanitize_char(*input))));
800806
}
801807
if (*input != ',')
802808
ereport(ERROR,
803809
(errcode(ERRCODE_PROTOCOL_VIOLATION),
804-
errmsg("malformed SCRAM message (comma expected, got %s)",
805-
sanitize_char(*input))));
810+
errmsg("malformed SCRAM message"),
811+
errdetail("Comma expected, but found character %s.",
812+
sanitize_char(*input))));
806813
input++;
807814

808815
/*
@@ -815,8 +822,9 @@ read_client_first_message(scram_state *state, char *input)
815822
if (*input != ',')
816823
ereport(ERROR,
817824
(errcode(ERRCODE_PROTOCOL_VIOLATION),
818-
errmsg("malformed SCRAM message (unexpected attribute %s in client-first-message)",
819-
sanitize_char(*input))));
825+
errmsg("malformed SCRAM message"),
826+
errdetail("Unexpected attribute %s in client-first-message.",
827+
sanitize_char(*input))));
820828
input++;
821829

822830
state->client_first_message_bare = pstrdup(input);
@@ -831,7 +839,7 @@ read_client_first_message(scram_state *state, char *input)
831839
if (*input == 'm')
832840
ereport(ERROR,
833841
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
834-
errmsg("client requires mandatory SCRAM extension")));
842+
errmsg("client requires an unsupported SCRAM extension")));
835843

836844
/*
837845
* Read username. Note: this is ignored. We use the username from the
@@ -960,7 +968,7 @@ build_server_first_message(scram_state *state)
960968
int encoded_len;
961969

962970
if (!pg_backend_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
963-
ereport(COMMERROR,
971+
ereport(ERROR,
964972
(errcode(ERRCODE_INTERNAL_ERROR),
965973
errmsg("could not generate random nonce")));
966974

@@ -1044,14 +1052,16 @@ read_client_final_message(scram_state *state, char *input)
10441052
if (pg_b64_decode(value, strlen(value), client_proof) != SCRAM_KEY_LEN)
10451053
ereport(ERROR,
10461054
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1047-
(errmsg("malformed SCRAM message (malformed proof in client-final-message"))));
1055+
errmsg("malformed SCRAM message"),
1056+
errdetail("Malformed proof in client-final-message.")));
10481057
memcpy(state->ClientProof, client_proof, SCRAM_KEY_LEN);
10491058
pfree(client_proof);
10501059

10511060
if (*p != '\0')
10521061
ereport(ERROR,
10531062
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1054-
(errmsg("malformed SCRAM message (garbage at end of client-final-message)"))));
1063+
errmsg("malformed SCRAM message"),
1064+
errdetail("Garbage found at the end of client-final-message.")));
10551065

10561066
state->client_final_message_without_proof = palloc(proof - begin + 1);
10571067
memcpy(state->client_final_message_without_proof, input, proof - begin);

‎src/backend/libpq/auth.c

+6-9
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,7 @@ recv_password_packet(Port *port)
656656
* log.
657657
*/
658658
if (mtype != EOF)
659-
ereport(COMMERROR,
659+
ereport(ERROR,
660660
(errcode(ERRCODE_PROTOCOL_VIOLATION),
661661
errmsg("expected password response, got message type %d",
662662
mtype)));
@@ -684,7 +684,7 @@ recv_password_packet(Port *port)
684684
* StringInfo is guaranteed to have an appended '\0'.
685685
*/
686686
if (strlen(buf.data) + 1 != buf.len)
687-
ereport(COMMERROR,
687+
ereport(ERROR,
688688
(errcode(ERRCODE_PROTOCOL_VIOLATION),
689689
errmsg("invalid password packet size")));
690690

@@ -897,11 +897,10 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
897897
/* Only log error if client didn't disconnect. */
898898
if (mtype != EOF)
899899
{
900-
ereport(COMMERROR,
900+
ereport(ERROR,
901901
(errcode(ERRCODE_PROTOCOL_VIOLATION),
902902
errmsg("expected SASL response, got message type %d",
903903
mtype)));
904-
return STATUS_ERROR;
905904
}
906905
else
907906
return STATUS_EOF;
@@ -935,11 +934,9 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
935934
selected_mech = pq_getmsgrawstring(&buf);
936935
if (strcmp(selected_mech, SCRAM_SHA256_NAME) != 0)
937936
{
938-
ereport(COMMERROR,
937+
ereport(ERROR,
939938
(errcode(ERRCODE_PROTOCOL_VIOLATION),
940939
errmsg("client selected an invalid SASL authentication mechanism")));
941-
pfree(buf.data);
942-
return STATUS_ERROR;
943940
}
944941

945942
inputlen = pq_getmsgint(&buf, 4);
@@ -1144,7 +1141,7 @@ pg_GSS_recvauth(Port *port)
11441141
{
11451142
/* Only log error if client didn't disconnect. */
11461143
if (mtype != EOF)
1147-
ereport(COMMERROR,
1144+
ereport(ERROR,
11481145
(errcode(ERRCODE_PROTOCOL_VIOLATION),
11491146
errmsg("expected GSS response, got message type %d",
11501147
mtype)));
@@ -1384,7 +1381,7 @@ pg_SSPI_recvauth(Port *port)
13841381
{
13851382
/* Only log error if client didn't disconnect. */
13861383
if (mtype != EOF)
1387-
ereport(COMMERROR,
1384+
ereport(ERROR,
13881385
(errcode(ERRCODE_PROTOCOL_VIOLATION),
13891386
errmsg("expected SSPI response, got message type %d",
13901387
mtype)));

0 commit comments

Comments
 (0)