Better sanity checking for message length words

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Matthias Apitz <guru(at)unixarea(dot)de>
Subject: Better sanity checking for message length words
Date: 2021-04-25 17:51:29
Message-ID: 2003757.1619373089@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
tighten-sanity-checks-on-message-lengths-1.patch text/x-diff 8.8 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2021-04-25 18:33:43 Re: Allowing to create LEAKPROOF functions to non-superuser
Previous Message Julien Rouhaud 2021-04-25 17:32:06 Re: compute_query_id and pg_stat_statements