[PATCH] Fix unbounded authentication exchanges during PQconnectPoll()

On 14/02/2023 01:22, Jacob Champion wrote:

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.

Looks mostly OK to me. Just a few nits on the error message style:

This patch adds the following error messages:

"server sent overlong v2 error message"
"server sent truncated error message"
"server sent truncated protocol negotiation message"
"server sent truncated authentication request"

Existing messages that are most similar to this:

"received invalid response to SSL negotiation: %c"
"received unencrypted data after SSL response"
"received invalid response to GSSAPI negotiation: %c"
"received unencrypted data after GSSAPI encryption response"
"expected authentication request from server, but received %c"
"unexpected message from server during startup"

The existing style emphasizes receiving the message, rather than what
the server sent. In that style, I'd suggest:

"received invalid error message"
"received invalid protocol negotiation message"
"received invalid authentication request"

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. Similarly,
the "truncated" cases mean that we didn't receive a null-terminator when
we expected one. It might be because the message was truncated, i.e. the
server sent it with a too-short message length. But just as likely, it
forgot to send the null-terminator, or it got confused in some other
way. So I'd go with just "invalid".

For similar reasons, I don't think we need to distinguish between the V3
and pre-V3 errors in the error message. If it's garbage, we probably
didn't guess correctly which one it was.

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.

@@ -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.

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.

(Wow, that was a long message for such a simple patch. I may have fallen
into the trap of bikeshedding, sorry :-) )

- Heikki

On Tue, Feb 21, 2023 at 12:35 PM Heikki Linnakangas <hlinnaka@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

On 22/02/2023 20:49, Jacob Champion wrote:

On Tue, Feb 21, 2023 at 12:35 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

@@ -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?

I separated the earlier message-length checks so that you get "invalid
invalid authentication request" or "received invalid protocol
negotiation message", depending on whether it was an 'R' or 'v' message.
With that, "invalid invalid authentication request" becomes translatable
anyway, which makes the point on translation overhead moot. I added a
comment to mention that it's unreachable, though.

I also reformatted the comments a little more.

Pushed with those changes, thanks!

- Heikki

On Wed, Feb 22, 2023 at 11:43 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

I separated the earlier message-length checks so that you get "invalid
invalid authentication request" or "received invalid protocol
negotiation message", depending on whether it was an 'R' or 'v' message.
With that, "invalid invalid authentication request" becomes translatable
anyway, which makes the point on translation overhead moot. I added a
comment to mention that it's unreachable, though.

Looks good, thank you!

--Jacob