On Tue, Feb 21, 2023 at 12:35 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
> I don't think the "overlong" or "truncated" bit is helpful. For example,
> if the pre-v3.0 error message seems to be "overlong", it's not clear
> that's really what happened. More likely, it's just garbage.
I think this is maybe a distinction without a difference, at least at
the protocol level -- in the event of a missed terminator, any message
could be garbage independently of whether it's too long. But I also
don't mind the "invalid" wording you've proposed, so done that way in
v2. (You're probably going to break out Wireshark for this either
way.)
> It's useful to have a unique error message for every different error, so
> that if you see that error, you can point to the exact place in the code
> where it was generated. If we care about that, we could add some detail
> to the messages, like "received invalid error message; null-terminator
> not found before end-of-message". I don't think that's necessary,
> though, and we've re-used the "expected authentication request from
> server, but received %c" for two different checks already.
(Note that I've reworded the duplicate message in patch v2, if that
changes the calculus.)
> > @@ -3370,6 +3389,7 @@ keep_going:
> > /* We will come back to here until there is
> > /* Get the type of request. */
> > if (pqGetInt((int *) &areq, 4, conn))
> > {
> > + libpq_append_conn_error(conn,
> > "server sent truncated authentication request");
> > goto error_return;
> > }
> > msgLength -= 4;
>
> This is unreachable, because we already checked the length. Better safe
> than sorry I guess, but let's avoid the translation overhead of this at
> least.
Should we just Assert() instead of an error message?
> This isn't from your patch, but a pre-existing message in the vicinity
> that caught my eye:
>
> > if ((beresp == 'R' || beresp == 'v') &&
> > (msgLength < 8 || msgLength > 2000))
> > {
> > libpq_append_conn_error(conn,
> > "expected authentication request from server, but received %c",
> >
> > beresp);
> > goto error_return;
> > }
>
> If we receive a 'R' or 'v' message that's too long or too short, the
> message is confusing because the 'beresp' that it prints is actually
> expected, but the length is unexpected.
Updated. I think there's room for additional improvement here, since
as of the protocol negotiation improvements, we don't just expect an
authentication request anymore.
> (Wow, that was a long message for such a simple patch. I may have fallen
> into the trap of bikeshedding, sorry :-) )
No worries :D This code is overdue for a tuneup, I think, and message
tweaks are cheap.
Thanks!
--Jacob
1: 04942e8e64 ! 1: d7ff5c8f64 PQconnectPoll: fix unbounded authentication
exchanges
@@ Commit message
long, and a v3 error should be bounded by its original message length.
For the existing error_return cases, I added some additional error
- messages for symmetry with the new ones.
+ messages for symmetry with the new ones, and cleaned up some message
+ rot.
## src/interfaces/libpq/fe-connect.c ##
@@ src/interfaces/libpq/fe-connect.c: keep_going:
/* We will come back to here until there is
+ */
+ if ((beresp == 'R' || beresp == 'v') &&
(msgLength < 8 || msgLength > 2000))
+ {
+- libpq_append_conn_error(conn, "expected
authentication request from server, but received %c",
+-
beresp);
++ libpq_append_conn_error(conn, "received
invalid authentication request");
goto error_return;
}
@@ src/interfaces/libpq/fe-connect.c: keep_going:
/* We will come back to here
+ avail = conn->inEnd -
conn->inCursor;
+ if (avail > MAX_ERRLEN)
+ {
-+
libpq_append_conn_error(conn, "server sent overlong v2 error message");
++
libpq_append_conn_error(conn, "received invalid error message");
+ goto error_return;
+ }
+
@@ src/interfaces/libpq/fe-connect.c: keep_going:
/* We will come back to here
{
- /* We'll come back when there
is more data */
- return PGRES_POLLING_READING;
-+ libpq_append_conn_error(conn,
"server sent truncated error message");
++ libpq_append_conn_error(conn,
"received invalid error message");
+ goto error_return;
}
/* OK, we read the message; mark data
consumed */
@@ src/interfaces/libpq/fe-connect.c: keep_going:
/* We will come back to here
{
if
(pqGetNegotiateProtocolVersion3(conn))
{
-+ libpq_append_conn_error(conn,
"server sent truncated protocol negotiation message");
++ libpq_append_conn_error(conn,
"received invalid protocol negotiation message");
goto error_return;
}
/* OK, we read the message; mark data
consumed */
@@ src/interfaces/libpq/fe-connect.c: keep_going:
/* We will come back to here
/* Get the type of request. */
if (pqGetInt((int *) &areq, 4, conn))
{
-+ libpq_append_conn_error(conn, "server
sent truncated authentication request");
++ libpq_append_conn_error(conn, "received
invalid authentication request");
goto error_return;
}
msgLength -= 4;
From d7ff5c8f6412345d59d9c5c66bc79cb6dc98802b Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Fri, 18 Nov 2022 13:45:34 -0800
Subject: [PATCH v2] PQconnectPoll: fix unbounded authentication exchanges
A couple of code paths in CONNECTION_AWAITING_RESPONSE will eagerly read
bytes off a connection that should be closed. Don't let a misbehaving
server chew up client resources here; a v2 error can't be infinitely
long, and a v3 error should be bounded by its original message length.
For the existing error_return cases, I added some additional error
messages for symmetry with the new ones, and cleaned up some message
rot.
---
src/interfaces/libpq/fe-connect.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50b5df3490..762ba51d2e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3225,17 +3225,28 @@ keep_going: /* We will come back to here until there is
*/
if ((beresp == 'R' || beresp == 'v') && (msgLength < 8 || msgLength > 2000))
{
- libpq_append_conn_error(conn, "expected authentication request from server, but received %c",
- beresp);
+ libpq_append_conn_error(conn, "received invalid authentication request");
goto error_return;
}
- if (beresp == 'E' && (msgLength < 8 || msgLength > 30000))
+#define MAX_ERRLEN 30000
+ if (beresp == 'E' && (msgLength < 8 || msgLength > MAX_ERRLEN))
{
/* Handle error from a pre-3.0 server */
conn->inCursor = conn->inStart + 1; /* reread data */
if (pqGets_append(&conn->errorMessage, conn))
{
+ /*
+ * We may not have authenticated the server yet, so
+ * don't let the buffer grow forever.
+ */
+ avail = conn->inEnd - conn->inCursor;
+ if (avail > MAX_ERRLEN)
+ {
+ libpq_append_conn_error(conn, "received invalid error message");
+ goto error_return;
+ }
+
/* We'll come back when there is more data */
return PGRES_POLLING_READING;
}
@@ -3255,9 +3266,15 @@ keep_going: /* We will come back to here until there is
goto error_return;
}
+#undef MAX_ERRLEN
/*
* Can't process if message body isn't all here yet.
+ *
+ * After this check passes, any further EOF during parsing
+ * implies that the server sent a bad/truncated message. Reading
+ * more bytes won't help in that case, so don't return
+ * PGRES_POLLING_READING after this point.
*/
msgLength -= 4;
avail = conn->inEnd - conn->inCursor;
@@ -3280,8 +3297,8 @@ keep_going: /* We will come back to here until there is
{
if (pqGetErrorNotice3(conn, true))
{
- /* We'll come back when there is more data */
- return PGRES_POLLING_READING;
+ libpq_append_conn_error(conn, "received invalid error message");
+ goto error_return;
}
/* OK, we read the message; mark data consumed */
conn->inStart = conn->inCursor;
@@ -3357,6 +3374,7 @@ keep_going: /* We will come back to here until there is
{
if (pqGetNegotiateProtocolVersion3(conn))
{
+ libpq_append_conn_error(conn, "received invalid protocol negotiation message");
goto error_return;
}
/* OK, we read the message; mark data consumed */
@@ -3370,6 +3388,7 @@ keep_going: /* We will come back to here until there is
/* Get the type of request. */
if (pqGetInt((int *) &areq, 4, conn))
{
+ libpq_append_conn_error(conn, "received invalid authentication request");
goto error_return;
}
msgLength -= 4;
--
2.25.1
