public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: [PATCH] Fix unbounded authentication exchanges during PQconnectPoll()
Date: Mon, 13 Feb 2023 15:22:11 -0800
Message-ID: <[email protected]> (raw)
Hello,
This is closely related to the prior conversation at [1]. There are a
couple places in CONNECTION_AWAITING_RESPONSE where libpq will read a
huge number of bytes from a server that we really should have hung up on.
The attached patch adds a length check for the v2 error compatibility
case, and updates the v3 error handling to jump to error_return rather
than asking for more data. The existing error_return paths have been
updated for consistency.
Thanks,
--Jacob
[1]
https://www.postgresql.org/message-id/a5c5783d-73f3-acbc-997f-1649a7406029%40timescale.com
Attachments:
[text/x-patch] PQconnectPoll-fix-unbounded-authentication-exchanges.patch (3.3K, ../[email protected]/2-PQconnectPoll-fix-unbounded-authentication-exchanges.patch)
download | inline diff:
From 949a5e994235a60731ff827dcfc5157007d937ca Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 18 Nov 2022 13:45:34 -0800
Subject: [PATCH] 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.
---
src/interfaces/libpq/fe-connect.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50b5df3490..5212ae1a52 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3230,12 +3230,24 @@ keep_going: /* We will come back to here until there is
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, "server sent overlong v2 error message");
+ goto error_return;
+ }
+
/* We'll come back when there is more data */
return PGRES_POLLING_READING;
}
@@ -3255,9 +3267,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 +3298,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, "server sent truncated error message");
+ goto error_return;
}
/* OK, we read the message; mark data consumed */
conn->inStart = conn->inCursor;
@@ -3357,6 +3375,7 @@ keep_going: /* We will come back to here until there is
{
if (pqGetNegotiateProtocolVersion3(conn))
{
+ libpq_append_conn_error(conn, "server sent truncated protocol negotiation message");
goto error_return;
}
/* OK, we read the message; mark data consumed */
@@ -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;
--
2.25.1
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Fix unbounded authentication exchanges during PQconnectPoll()
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox