Re: [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