Thread: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
[BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Daniele Varrazzo
Date:
Patch attached -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Michael Paquier
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Alvaro Herrera
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Michael Paquier
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Alvaro Herrera
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Andres Freund
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Michael Paquier
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Alvaro Herrera
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Michael Paquier
Date:
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
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
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] 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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Heikki Linnakangas
Date:
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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Heikki Linnakangas
Date:
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 -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
Attachment
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAM message.
From
Michael Paquier
Date:
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
Re: [BUGS] [PATCH] Fixed malformed error message on malformed SCRAMmessage.
From
Heikki Linnakangas
Date:
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