Horton

Thread

Commits

  1. Add heuristic incoming-message-size limits in the server.

  1. Better sanity checking for message length words

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-25T17:51:29Z

    We had a report [1] of a case where a broken client application
sent some garbage to the server, which then hung up because it
misinterpreted the garbage as a very long message length, and was
sitting waiting for data that would never arrive.  There is already
a sanity check on the message type byte in postgres.c's SocketBackend
(which the trouble case accidentally got past because 'S' is a valid
type code); but the only check on the message length is that it be
at least 4.

pq_getmessage() does have the ability to enforce an upper limit on
message length, but we only use that capability for authentication
messages, and not entirely consistently even there.

Meanwhile on the client side, libpq has had simple message-length
sanity checking for ages: it disbelieves message lengths greater
than 30000 bytes unless the message type is one of a short list
of types that can be long.

So the attached proposed patch changes things to make it required
not optional for callers of pq_getmessage to provide an upper length
bound, and installs the same sort of short-vs-long message heuristic
as libpq has in the server.  This is also a good opportunity for
other callers to absorb the lesson SocketBackend learned many years
ago: we should validate the message type code *before* believing
anything about the message length.  All of this is just heuristic
of course, but I think it makes for a substantial reduction in the
trouble surface.

Given the small number of complaints to date, I'm hesitant to
back-patch this: if there's anybody out there with valid use for
long messages that I didn't think should be long, this might break
things for them.  But I think it'd be reasonable to sneak it into
v14, since we've not started beta yet.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/YIKCCXcEozx9iiBU%40c720-r368166.fritz.box

  2. Re: Better sanity checking for message length words

    Aleksander Alekseev <aleksander@timescale.com> — 2021-04-27T10:29:02Z

    Hi Tom,

> ...
> Given the small number of complaints to date, I'm hesitant to
> back-patch this: if there's anybody out there with valid use for
> long messages that I didn't think should be long, this might break
> things for them.  But I think it'd be reasonable to sneak it into
> v14, since we've not started beta yet.
>
> Thoughts?

I'm having slight issues applying your patch to the `master` branch.
Is it the right target?

Regarding the idea, I think extra checks are a good thing and
definitely something that can be introduced in the next major version.
If we receive a complaint during beta-testing we can revert the patch
or maybe increase the limit for small messages.

-- 
Best regards,
Aleksander Alekseev

  3. Re: Better sanity checking for message length words

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-27T14:38:23Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> I'm having slight issues applying your patch to the `master` branch.
> Is it the right target?

[ scratches head ... ]  The patch still applies perfectly cleanly
for me, using either "patch" or "git apply".

> Regarding the idea, I think extra checks are a good thing and
> definitely something that can be introduced in the next major version.
> If we receive a complaint during beta-testing we can revert the patch
> or maybe increase the limit for small messages.

Actually, I did some more testing yesterday and found that
"make check-world" passes with PQ_SMALL_MESSAGE_LIMIT values
as small as 16.  That may say more about our testing than
anything else --- for example, it implies we're not using long
statement or portal names anywhere.  But still, it suggests
that 30000 is between one and two orders of magnitude too large.
I'm now thinking that 10000 would be a good conservative setting,
or we could try 1000 if we want to be a bit more aggressive.
As you say, beta-testing feedback could result in further
modifications.

			regards, tom lane

  4. Re: Better sanity checking for message length words

    Aleksander Alekseev <aleksander@timescale.com> — 2021-04-28T08:40:27Z

    Hi Tom,

> scratches head ... ]  The patch still applies perfectly cleanly
> for me, using either "patch" or "git apply".

Sorry, my bad. It was about lines separating on different platforms. The
patch is fine and passes installcheck-world on MacOS.

-- 
Best regards,
Aleksander Alekseev

  5. Re: Better sanity checking for message length words

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-28T19:51:27Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> Sorry, my bad. It was about lines separating on different platforms. The
> patch is fine and passes installcheck-world on MacOS.

Pushed, thanks for looking at it!

			regards, tom lane