[PATCH] Fixed malformed error message on malformed SCRAM message.
Patch attached
Attachments:
0001-Fixed-malformed-error-message-on-malformed-SCRAM-mes.patchtext/x-patch; charset=US-ASCII; name=0001-Fixed-malformed-error-message-on-malformed-SCRAM-mes.patchDownload
From 8f52fc3d0efd27c4d0cc4f0034563482ce2b2854 Mon Sep 17 00:00:00 2001
From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Mon, 29 May 2017 21:34:26 +0100
Subject: [PATCH] Fixed malformed error message on malformed SCRAM message.
Yep.
---
src/backend/libpq/auth-scram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 99feb0c..f91ba7c 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1044,7 +1044,7 @@ 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 (malformed proof in client-final-message)"))));
memcpy(state->ClientProof, client_proof, SCRAM_KEY_LEN);
pfree(client_proof);
--
2.7.4
On Mon, May 29, 2017 at 1:38 PM, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:
Patch attached
Right. I am adding that to the list of open items, and Heikki in CC
will likely take care of it.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Daniele Varrazzo wrote:
Patch attached
Uh, shouldn't the parenthical comment be errdetail()?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, May 29, 2017 at 1:43 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Daniele Varrazzo wrote:
Patch attached
Uh, shouldn't the parenthical comment be errdetail()?
FWIW, this did not strike me as necessary for this one as well all the
other error messages in auth-scram.c as the main error strings are
short, and the extra information looked adapted as part of the main
error message.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Michael Paquier wrote:
On Mon, May 29, 2017 at 1:43 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Daniele Varrazzo wrote:
Patch attached
Uh, shouldn't the parenthical comment be errdetail()?
FWIW, this did not strike me as necessary for this one as well all the
other error messages in auth-scram.c as the main error strings are
short, and the extra information looked adapted as part of the main
error message.
To me this sounds like you're uttering "beetlejuice" thrice to summon
the message-style police.
These messages look all wrong to me.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On May 29, 2017 2:06:49 PM PDT, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
To me this sounds like you're uttering "beetlejuice" thrice to summon
the message-style police.
Hey, my poor coffee... ;)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, May 29, 2017 at 2:06 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
These messages look all wrong to me.
So your complain would be to do the following for each error message
that uses parenthesis to include details? Like that I suppose:
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -283,11 +283,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("Empty message.")));
if (inputlen != strlen(input))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
- (errmsg("malformed SCRAM message (length mismatch)"))));
+ errmsg("malformed SCRAM message"),
+ errdetail("Length mismatch.")));
--
Michael--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Michael Paquier wrote:
On Mon, May 29, 2017 at 2:06 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:These messages look all wrong to me.
So your complain would be to do the following for each error message that uses parenthesis to include details? Like that I suppose: --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -283,11 +283,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("Empty message.")));
Yeah, but along the lines of errdetail("The message is empty.")
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, May 29, 2017 at 2:40 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Michael Paquier wrote:
On Mon, May 29, 2017 at 2:06 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:These messages look all wrong to me.
So your complain would be to do the following for each error message that uses parenthesis to include details? Like that I suppose: --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -283,11 +283,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("Empty message.")));Yeah, but along the lines of errdetail("The message is empty.")
Okay. What do you think about the attached patch then? Does it address
your concerns about the format of those error messages?
--
Michael
Attachments:
scram-errdetail.patchapplication/octet-stream; name=scram-errdetail.patchDownload
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 99feb0ce94..366a11feb8 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -283,11 +283,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("Input length does not match.")));
switch (state->state)
{
@@ -319,7 +321,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.
@@ -582,14 +585,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 +674,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 +684,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 +802,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 +824,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);
@@ -1044,14 +1054,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);
On Tue, May 30, 2017 at 04:39:55PM -0700, Michael Paquier wrote:
On Mon, May 29, 2017 at 2:40 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Michael Paquier wrote:
On Mon, May 29, 2017 at 2:06 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:These messages look all wrong to me.
So your complain would be to do the following for each error message that uses parenthesis to include details? Like that I suppose: --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -283,11 +283,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("Empty message.")));Yeah, but along the lines of errdetail("The message is empty.")
Okay. What do you think about the attached patch then? Does it address
your concerns about the format of those error messages?
--
Michael
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 99feb0ce94..366a11feb8 100644 --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -283,11 +283,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("Input length does not match.")));
auth.c uses COMMERROR for this sort of thing; why does auth-scram.c use ERROR?
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Mon, May 29, 2017 at 01:43:26PM -0700, Michael Paquier wrote:
On Mon, May 29, 2017 at 1:38 PM, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:Patch attached
Right. I am adding that to the list of open items, and Heikki in CC
will likely take care of it.
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Noah Misch <noah@leadboat.com> writes:
auth.c uses COMMERROR for this sort of thing; why does auth-scram.c use ERROR?
"COMMERROR" is more or less "LOG"; we'd have to throw a different error
afterwards if we reported these messages with COMMERROR.
The main case where COMMERROR is appropriate, I think, is where we think
that communication with the client has already failed and it's unlikely
that what we say will reach the client; but we'd like to be sure it
reaches the postmaster log.
There's a somewhat separate question of whether we'd be exposing useful
information to an attacker if we returned such details to the client.
Not entirely sure if any of these messages fall into that category, but
offhand I think that an attacker would already know if he's violating
protocol.
BTW, since you mention COMMERROR uses in auth.c, isn't the usage at
line 687 wrong? It sure looks like the author supposed that that
ereport call wouldn't return, but it will. Adjacent similar calls
clean up and return NULL.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 06/02/2017 08:57 AM, Noah Misch wrote:
On Tue, May 30, 2017 at 04:39:55PM -0700, Michael Paquier wrote:
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 99feb0ce94..366a11feb8 100644 --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -283,11 +283,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("Input length does not match.")));auth.c uses COMMERROR for this sort of thing; why does auth-scram.c use ERROR?
Yeah, COMMERROR would be more consistent. But I wonder why auth.c uses
COMMERROR rather than ERROR for these. It would seem more user-friendly.
Or developer-friendly; the most likely time you see these messages is
when you're developing a driver that speaks the protocol.
And pqformat.c uses ERROR for protocol violation errors. So we're not
very consistent. I think it would be best to convert all or most of the
ERRCODE_PROTOCOL_VIOLATION messages to ERROR. COMMERROR is appropriate
when you you think that the client has already disconnected, or is so
thoroughly confused that sending an error would just make things worse.
Or if you the message contains information that you don't want to reveal
to a non-authenticated client. I don't think that applies to any of the
COMMERRORs in auth.c.
- Heikki
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Fri, Jun 02, 2017 at 02:20:00AM -0400, Tom Lane wrote:
BTW, since you mention COMMERROR uses in auth.c, isn't the usage at
line 687 wrong? It sure looks like the author supposed that that
ereport call wouldn't return, but it will. Adjacent similar calls
clean up and return NULL.
Probably, though one could argue for proceeding with the short password.
Deserves a comment if log-only is intentional.
The lack of an exit after COMMERROR "client selected an invalid SASL
authentication mechanism" looks like a bug.
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Fri, Jun 02, 2017 at 05:58:59AM +0000, Noah Misch wrote:
On Mon, May 29, 2017 at 01:43:26PM -0700, Michael Paquier wrote:
On Mon, May 29, 2017 at 1:38 PM, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:Patch attached
Right. I am adding that to the list of open items, and Heikki in CC
will likely take care of it.[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.[1] /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
This PostgreSQL 10 open item is past due for your status update. Kindly send
a status update within 24 hours, and include a date for your subsequent status
update. Refer to the policy on open item ownership:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 06/02/2017 09:32 AM, Noah Misch wrote:
On Fri, Jun 02, 2017 at 02:20:00AM -0400, Tom Lane wrote:
BTW, since you mention COMMERROR uses in auth.c, isn't the usage at
line 687 wrong? It sure looks like the author supposed that that
ereport call wouldn't return, but it will. Adjacent similar calls
clean up and return NULL.Probably, though one could argue for proceeding with the short password.
Deserves a comment if log-only is intentional.
Let's turn it into an ERROR.
The lack of an exit after COMMERROR "client selected an invalid SASL
authentication mechanism" looks like a bug.
Yes. That was fixed in commit 505b5d2f86 already.
Taking all the comments in this thread into account, and a few more
things that I spotted while looking at the error messages, I came up
with the attached patch. It includes the changes from Michael's patch
upthread to use errdetail() in the SCRAM errors, and it turns the
protocol violation errors in auth.c from COMMERROR into ERROR. See
commit message for more details. Barring objections, I'll push this
tomorrow.
- Heikki
Attachments:
0001-Improve-authentication-error-messages.patchapplication/x-download; name=0001-Improve-authentication-error-messages.patchDownload
From d3bf2b93336830fe8381998caa1716809ca1ad4c Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Wed, 7 Jun 2017 17:41:45 +0300
Subject: [PATCH 1/1] 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
---
src/backend/libpq/auth-scram.c | 66 ++++++++++++++++++++++++------------------
src/backend/libpq/auth.c | 15 ++++------
2 files changed, 44 insertions(+), 37 deletions(-)
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 99feb0ce94..e4a535f4a1 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -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);
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 5b68e3b7a1..081c06a1e6 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -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)));
--
2.11.0
On Wed, Jun 7, 2017 at 11:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 06/02/2017 09:32 AM, Noah Misch wrote:
BTW, since you mention COMMERROR uses in auth.c, isn't the usage at
line 687 wrong? It sure looks like the author supposed that that
ereport call wouldn't return, but it will. Adjacent similar calls
clean up and return NULL.Probably, though one could argue for proceeding with the short password.
Deserves a comment if log-only is intentional.Let's turn it into an ERROR.
Shouldn't that portion be back-patched?
The lack of an exit after COMMERROR "client selected an invalid SASL
authentication mechanism" looks like a bug.Yes. That was fixed in commit 505b5d2f86 already.
Taking all the comments in this thread into account, and a few more things
that I spotted while looking at the error messages, I came up with the
attached patch. It includes the changes from Michael's patch upthread to use
errdetail() in the SCRAM errors, and it turns the protocol violation errors
in auth.c from COMMERROR into ERROR. See commit message for more details.
Barring objections, I'll push this tomorrow.
Thanks for the new version. No additional comments from me.
--
Michael
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 06/08/2017 03:07 AM, Michael Paquier wrote:
On Wed, Jun 7, 2017 at 11:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 06/02/2017 09:32 AM, Noah Misch wrote:
BTW, since you mention COMMERROR uses in auth.c, isn't the usage at
line 687 wrong? It sure looks like the author supposed that that
ereport call wouldn't return, but it will. Adjacent similar calls
clean up and return NULL.Probably, though one could argue for proceeding with the short password.
Deserves a comment if log-only is intentional.Let's turn it into an ERROR.
Shouldn't that portion be back-patched?
Yeah, perhaps. But since it's not actively broken, nothing particularly
bad happens with the code as it is, I think I'm not going to bother.
The lack of an exit after COMMERROR "client selected an invalid SASL
authentication mechanism" looks like a bug.Yes. That was fixed in commit 505b5d2f86 already.
Taking all the comments in this thread into account, and a few more things
that I spotted while looking at the error messages, I came up with the
attached patch. It includes the changes from Michael's patch upthread to use
errdetail() in the SCRAM errors, and it turns the protocol violation errors
in auth.c from COMMERROR into ERROR. See commit message for more details.
Barring objections, I'll push this tomorrow.Thanks for the new version. No additional comments from me.
Ok, committed, thanks!
- Heikki
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs