From 9626325da5e8e23ff90091bc96535495d350f06e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 28 Apr 2021 15:50:42 -0400 Subject: [PATCH] Add heuristic incoming-message-size limits in the server. We had a report of confusing server behavior caused by a client bug that sent junk to the server: the server thought the junk was a very long message length and waited patiently for data that would never come. We can reduce the risk of that by being less trusting about message lengths. For a long time, libpq has had a heuristic rule that it wouldn't believe large message size words, except for a small number of message types that are expected to be (potentially) long. This provides some defense against loss of message-boundary sync and other corrupted-data cases. The server does something similar, except that up to now it only limited the lengths of messages received during the connection authentication phase. Let's do the same as in libpq and put restrictions on the allowed length of all messages, while distinguishing between message types that are expected to be long and those that aren't. I used a limit of 10000 bytes for non-long messages. (libpq's corresponding limit is 30000 bytes, but given the asymmetry of the FE/BE protocol, there's no good reason why the numbers should be the same.) Experimentation suggests that this is at least a factor of 10, maybe a factor of 100, more than we really need; but plenty of daylight seems desirable to avoid false positives. In any case we can adjust the limit based on beta-test results. For long messages, set a limit of MaxAllocSize - 1, which is the most that we can absorb into the StringInfo buffer that the message is collected in. This just serves to make sure that a bogus message size is reported as such, rather than as a confusing gripe about not being able to enlarge a string buffer. While at it, make sure that non-mainline code paths (such as COPY FROM STDIN) are as paranoid as SocketBackend is, and validate the message type code before believing the message length. This provides an additional guard against getting stuck on corrupted input. Discussion: https://postgr.es/m/2003757.1619373089@sss.pgh.pa.us --- src/backend/commands/copyfromparse.c | 31 ++++++++++++++++++++++------ src/backend/libpq/auth.c | 3 ++- src/backend/libpq/pqcomm.c | 5 ++--- src/backend/replication/walsender.c | 29 ++++++++++++++++++++------ src/backend/tcop/postgres.c | 24 ++++++++++++++++++--- src/include/libpq/libpq.h | 9 ++++++++ 6 files changed, 82 insertions(+), 19 deletions(-) diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 0813424768f..fdf57f15560 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -265,6 +265,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) { /* Try to receive another message */ int mtype; + int maxmsglen; readmessage: HOLD_CANCEL_INTERRUPTS(); @@ -274,11 +275,33 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); - if (pq_getmessage(cstate->fe_msgbuf, 0)) + /* Validate message type and set packet size limit */ + switch (mtype) + { + case 'd': /* CopyData */ + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; + break; + case 'c': /* CopyDone */ + case 'f': /* CopyFail */ + case 'H': /* Flush */ + case 'S': /* Sync */ + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; + break; + default: + ereport(ERROR, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("unexpected message type 0x%02X during COPY from stdin", + mtype))); + maxmsglen = 0; /* keep compiler quiet */ + break; + } + /* Now collect the message body */ + if (pq_getmessage(cstate->fe_msgbuf, maxmsglen)) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); RESUME_CANCEL_INTERRUPTS(); + /* ... and process it */ switch (mtype) { case 'd': /* CopyData */ @@ -304,11 +327,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) */ goto readmessage; default: - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("unexpected message type 0x%02X during COPY from stdin", - mtype))); - break; + Assert(false); /* NOT REACHED */ } } avail = cstate->fe_msgbuf->len - cstate->fe_msgbuf->cursor; diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 27865b14a03..45a91235a45 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -210,6 +210,7 @@ static int PerformRadiusTransaction(const char *server, const char *secret, cons /* * Maximum accepted size of GSS and SSPI authentication tokens. + * We also use this as a limit on ordinary password packet lengths. * * Kerberos tickets are usually quite small, but the TGTs issued by Windows * domain controllers include an authorization field known as the Privilege @@ -724,7 +725,7 @@ recv_password_packet(Port *port) } initStringInfo(&buf); - if (pq_getmessage(&buf, 0)) /* receive password */ + if (pq_getmessage(&buf, PG_MAX_AUTH_TOKEN_LENGTH)) /* receive password */ { /* EOF - pq_getmessage already logged a suitable message */ pfree(buf.data); diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 8066ee1d1e0..b9ccd4473f7 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -1203,7 +1203,7 @@ pq_is_reading_msg(void) * is removed. Also, s->cursor is initialized to zero for convenience * in scanning the message contents. * - * If maxlen is not zero, it is an upper limit on the length of the + * maxlen is the upper limit on the length of the * message we are willing to accept. We abort the connection (by * returning EOF) if client tries to send more than that. * @@ -1230,8 +1230,7 @@ pq_getmessage(StringInfo s, int maxlen) len = pg_ntoh32(len); - if (len < 4 || - (maxlen > 0 && len > maxlen)) + if (len < 4 || len > maxlen) { ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 52fe9aba660..6fefc3bedc4 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1704,6 +1704,7 @@ static void ProcessRepliesIfAny(void) { unsigned char firstchar; + int maxmsglen; int r; bool received = false; @@ -1733,9 +1734,28 @@ ProcessRepliesIfAny(void) break; } + /* Validate message type and set packet size limit */ + switch (firstchar) + { + case 'd': + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; + break; + case 'c': + case 'X': + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; + break; + default: + ereport(FATAL, + (errcode(ERRCODE_PROTOCOL_VIOLATION), + errmsg("invalid standby message type \"%c\"", + firstchar))); + maxmsglen = 0; /* keep compiler quiet */ + break; + } + /* Read the message contents */ resetStringInfo(&reply_message); - if (pq_getmessage(&reply_message, 0)) + if (pq_getmessage(&reply_message, maxmsglen)) { ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), @@ -1743,7 +1763,7 @@ ProcessRepliesIfAny(void) proc_exit(0); } - /* Handle the very limited subset of commands expected in this phase */ + /* ... and process it */ switch (firstchar) { /* @@ -1776,10 +1796,7 @@ ProcessRepliesIfAny(void) proc_exit(0); default: - ereport(FATAL, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("invalid standby message type \"%c\"", - firstchar))); + Assert(false); /* NOT REACHED */ } } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 1216a2b397b..2d6d145ecc0 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -343,6 +343,7 @@ static int SocketBackend(StringInfo inBuf) { int qtype; + int maxmsglen; /* * Get message type code from the frontend. @@ -375,7 +376,9 @@ SocketBackend(StringInfo inBuf) /* * Validate message type code before trying to read body; if we have lost * sync, better to say "command unknown" than to run out of memory because - * we used garbage as a length word. + * we used garbage as a length word. We can also select a type-dependent + * limit on what a sane length word could be. (The limit could be chosen + * more granularly, but it's not clear it's worth fussing over.) * * This also gives us a place to set the doing_extended_query_message flag * as soon as possible. @@ -383,28 +386,37 @@ SocketBackend(StringInfo inBuf) switch (qtype) { case 'Q': /* simple query */ + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = false; break; case 'F': /* fastpath function call */ + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; doing_extended_query_message = false; break; case 'X': /* terminate */ + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = false; ignore_till_sync = false; break; case 'B': /* bind */ + case 'P': /* parse */ + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; + doing_extended_query_message = true; + break; + case 'C': /* close */ case 'D': /* describe */ case 'E': /* execute */ case 'H': /* flush */ - case 'P': /* parse */ + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = true; break; case 'S': /* sync */ + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; /* stop any active skip-till-Sync */ ignore_till_sync = false; /* mark not-extended, so that a new error doesn't begin skip */ @@ -412,8 +424,13 @@ SocketBackend(StringInfo inBuf) break; case 'd': /* copy data */ + maxmsglen = PQ_LARGE_MESSAGE_LIMIT; + doing_extended_query_message = false; + break; + case 'c': /* copy done */ case 'f': /* copy fail */ + maxmsglen = PQ_SMALL_MESSAGE_LIMIT; doing_extended_query_message = false; break; @@ -427,6 +444,7 @@ SocketBackend(StringInfo inBuf) ereport(FATAL, (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("invalid frontend message type %d", qtype))); + maxmsglen = 0; /* keep compiler quiet */ break; } @@ -435,7 +453,7 @@ SocketBackend(StringInfo inBuf) * after the type code; we can read the message contents independently of * the type. */ - if (pq_getmessage(inBuf, 0)) + if (pq_getmessage(inBuf, maxmsglen)) return EOF; /* suitable message already logged */ RESUME_CANCEL_INTERRUPTS(); diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index 3ebbc8d6656..6c51b2f20fa 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -21,6 +21,15 @@ #include "storage/latch.h" +/* + * Callers of pq_getmessage() must supply a maximum expected message size. + * By convention, if there's not any specific reason to use another value, + * use PQ_SMALL_MESSAGE_LIMIT for messages that shouldn't be too long, and + * PQ_LARGE_MESSAGE_LIMIT for messages that can be long. + */ +#define PQ_SMALL_MESSAGE_LIMIT 10000 +#define PQ_LARGE_MESSAGE_LIMIT (MaxAllocSize - 1) + typedef struct { void (*comm_reset) (void);