From 855048b6a8452973be2afdc2b9b0bbf9ca3d324b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 1 Apr 2025 16:02:27 +0300 Subject: [PATCH 2/7] libpq: Handle NegotiateProtocolVersion message differently Previously libpq would always error when the server returns a NegotiateProtocolVersion message. This was fine because libpq only supported a single protocol version and did not support any protocol parameters. But we now that we're discussing protocol changes we need to change this behaviour, and introduce a fallback mechanism when connecting to an older server. This patch modifies the client side checks to allow a range of supported protocol versions, instead of only allowing the exact version that was requested. Currently this "range" only contains the 3.0 version, but in a future commit we'll change this. In addition it changes the errors for protocol to say that they error because we did not request any parameters, not because the server did not support some of them. In a future commit more infrastructure for protocol parameters will be added, so that these checks can also succeed when receiving unsupported parameters back. Note that at the moment this change does not have any behavioural effect, because libpq will only request version 3.0 and will never send protocol parameters. Which means that the client never receives a NegotiateProtocolVersion message from the server. Author: Jelte Fennema-Nio Discussion: /message-id/CAGECzQTfc_O%2BHXqAo5_-xG4r3EFVsTefUeQzSvhEyyLDba-O9w@mail.gmail.com Discussion: /message-id/CAGECzQRbAGqJnnJJxTdKewTsNOovUt4bsx3NFfofz3m2j-t7tA@mail.gmail.com --- src/interfaces/libpq/fe-connect.c | 12 +++- src/interfaces/libpq/fe-protocol3.c | 91 ++++++++++++++++++----------- src/interfaces/libpq/libpq-int.h | 1 + 3 files changed, 70 insertions(+), 34 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index d5051f5e820..a1e462f2e2f 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -667,6 +667,7 @@ pqDropServerData(PGconn *conn) /* Reset assorted other per-connection state */ conn->last_sqlstate[0] = '\0'; + conn->negotiate_pversion_received = false; conn->auth_req_received = false; conn->client_finished_auth = false; conn->password_needed = false; @@ -4084,16 +4085,25 @@ keep_going: /* We will come back to here until there is CONNECTION_FAILED(); } + /* Handle NegotiateProtocolVersion */ else if (beresp == PqMsg_NegotiateProtocolVersion) { + if (conn->negotiate_pversion_received) + { + libpq_append_conn_error(conn, "received duplicate protocol negotiation message"); + goto error_return; + } + conn->negotiate_pversion_received = true; + if (pqGetNegotiateProtocolVersion3(conn)) { libpq_append_conn_error(conn, "received invalid protocol negotiation message"); goto error_return; } + /* OK, we read the message; mark data consumed */ pqParseDone(conn, conn->inCursor); - goto error_return; + goto keep_going; } /* It is an authentication request. */ diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index 95dd456f076..43e3519e4bd 100644 --- a/src/interfaces/libpq/fe-protocol3.c +++ b/src/interfaces/libpq/fe-protocol3.c @@ -870,6 +870,7 @@ advance_and_error: /* * Attempt to read an Error or Notice response message. * This is possible in several places, so we break it out as a subroutine. + * * Entry: 'E' or 'N' message type and length have already been consumed. * Exit: returns 0 if successfully consumed message. * returns EOF if not enough data. @@ -1399,64 +1400,87 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding) /* - * Attempt to read a NegotiateProtocolVersion message. + * Attempt to read a NegotiateProtocolVersion message. Sets conn->pversion + * to the version that's negotiated by the server. + * * Entry: 'v' message type and length have already been consumed. * Exit: returns 0 if successfully consumed message. - * returns EOF if not enough data. + * returns 1 on failure. The error message is filled in. */ int pqGetNegotiateProtocolVersion3(PGconn *conn) { - int tmp; - ProtocolVersion their_version; + int their_version; int num; - PQExpBufferData buf; - if (pqGetInt(&tmp, 4, conn) != 0) - return EOF; - their_version = tmp; + if (pqGetInt(&their_version, 4, conn) != 0) + goto eof; if (pqGetInt(&num, 4, conn) != 0) - return EOF; + goto eof; - initPQExpBuffer(&buf); - for (int i = 0; i < num; i++) + /* Check the protocol version */ + if (their_version > conn->pversion) { - if (pqGets(&conn->workBuffer, conn)) - { - termPQExpBuffer(&buf); - return EOF; - } - if (buf.len > 0) - appendPQExpBufferChar(&buf, ' '); - appendPQExpBufferStr(&buf, conn->workBuffer.data); + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requested downgrade to a higher-numbered version"); + goto failure; + } + + if (their_version < PG_PROTOCOL(3, 0)) + { + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requested downgrade to pre-3.0 protocol version"); + goto failure; } - if (their_version < conn->pversion) - libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u", - PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion), - PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version)); - if (num > 0) + if (num < 0) { - appendPQExpBuffer(&conn->errorMessage, - libpq_ngettext("protocol extension not supported by server: %s", - "protocol extensions not supported by server: %s", num), - buf.data); - appendPQExpBufferChar(&conn->errorMessage, '\n'); + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported negative number of unsupported parameters"); + goto failure; + } + + if (their_version == conn->pversion && num == 0) + { + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server negotiated but asks for no changes"); + goto failure; } - /* neither -- server shouldn't have sent it */ - if (!(their_version < conn->pversion) && !(num > 0)) - libpq_append_conn_error(conn, "invalid %s message", "NegotiateProtocolVersion"); + /* the version is acceptable */ + conn->pversion = their_version; + + /* + * We don't currently request any protocol extensions, so we don't expect + * the server to reply with any either. + */ + for (int i = 0; i < num; i++) + { + if (pqGets(&conn->workBuffer, conn)) + { + goto eof; + } + if (strncmp(conn->workBuffer.data, "_pq_.", 5) != 0) + { + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported unsupported parameter name without a _pq_. prefix (\"%s\")", conn->workBuffer.data); + goto failure; + } + libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported an unsupported parameter that was not requested (\"%s\")", conn->workBuffer.data); + goto failure; + } - termPQExpBuffer(&buf); return 0; + +eof: + libpq_append_conn_error(conn, "received invalid protocol negotation message: message too short"); +failure: + conn->asyncStatus = PGASYNC_READY; + pqSaveErrorResult(conn); + return 1; } /* * Attempt to read a ParameterStatus message. * This is possible in several places, so we break it out as a subroutine. + * * Entry: 'S' message type and length have already been consumed. * Exit: returns 0 if successfully consumed message. * returns EOF if not enough data. @@ -1486,6 +1510,7 @@ getParameterStatus(PGconn *conn) /* * Attempt to read a Notify response message. * This is possible in several places, so we break it out as a subroutine. + * * Entry: 'A' message type and length have already been consumed. * Exit: returns 0 if successfully consumed Notify message. * returns EOF if not enough data. diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index ade5ad82f07..bf31e660477 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -496,6 +496,7 @@ struct pg_conn SockAddr raddr; /* Remote address */ ProtocolVersion pversion; /* FE/BE protocol version in use */ int sversion; /* server version, e.g. 70401 for 7.4.1 */ + bool negotiate_pversion_received; /* true if NegotiateProtocolVersion received */ bool auth_req_received; /* true if any type of auth req received */ bool password_needed; /* true if server demanded a password */ bool gssapi_used; /* true if authenticated via gssapi */ -- 2.39.5