Обсуждение: [PATCH] Fix unbounded authentication exchanges during PQconnectPoll()

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

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

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

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,
butreceived %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
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.
> 
> 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