[PATCH] Fixed malformed error message on malformed SCRAM message.

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

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

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