Re: [PATCH] Fix unbounded authentication exchanges during PQconnectPoll()

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