Re: [PATCH] Fix unbounded authentication exchanges during PQconnectPoll() - Mailing list pgsql-hackers

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
thereis
 
>                                 /* 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