Skip to content

Commit 289592b

Browse files
hlinnakahlinnaka
committed
Be more careful to not lose sync in the FE/BE protocol.
If any error occurred while we were in the middle of reading a protocol message from the client, we could lose sync, and incorrectly try to interpret a part of another message as a new protocol message. That will usually lead to an "invalid frontend message" error that terminates the connection. However, this is a security issue because an attacker might be able to deliberately cause an error, inject a Query message in what's supposed to be just user data, and have the server execute it. We were quite careful to not have CHECK_FOR_INTERRUPTS() calls or other operations that could ereport(ERROR) in the middle of processing a message, but a query cancel interrupt or statement timeout could nevertheless cause it to happen. Also, the V2 fastpath and COPY handling were not so careful. It's very difficult to recover in the V2 COPY protocol, so we will just terminate the connection on error. In practice, that's what happened previously anyway, as we lost protocol sync. To fix, add a new variable in pqcomm.c, PqCommReadingMsg, that is set whenever we're in the middle of reading a message. When it's set, we cannot safely ERROR out and continue running, because we might've read only part of a message. PqCommReadingMsg acts somewhat similarly to critical sections in that if an error occurs while it's set, the error handler will force the connection to be terminated, as if the error was FATAL. It's not implemented by promoting ERROR to FATAL in elog.c, like ERROR is promoted to PANIC in critical sections, because we want to be able to use PG_TRY/CATCH to recover and regain protocol sync. pq_getmessage() takes advantage of that to prevent an OOM error from terminating the connection. To prevent unnecessary connection terminations, add a holdoff mechanism similar to HOLD/RESUME_INTERRUPTS() that can be used hold off query cancel interrupts, but still allow die interrupts. The rules on which interrupts are processed when are now a bit more complicated, so refactor ProcessInterrupts() and the calls to it in signal handlers so that the signal handlers always call it if ImmediateInterruptOK is set, and ProcessInterrupts() can decide to not do anything if the other conditions are not met. Reported by Emil Lenngren. Patch reviewed by Noah Misch and Andres Freund. Backpatch to all supported versions. Security: CVE-2015-0244
1 parent d1972da commit 289592b

File tree

13 files changed

+251
-107
lines changed

13 files changed

+251
-107
lines changed

‎src/backend/commands/copy.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,8 @@ ReceiveCopyBegin(CopyState cstate)
395395
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
396396
errmsg("COPY BINARY is not supported to stdout or from stdin")));
397397
pq_putemptymessage('G');
398+
/* any error in old protocol will make us lose sync */
399+
pq_startmsgread();
398400
cstate->copy_dest = COPY_OLD_FE;
399401
}
400402
else
@@ -405,6 +407,8 @@ ReceiveCopyBegin(CopyState cstate)
405407
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
406408
errmsg("COPY BINARY is not supported to stdout or from stdin")));
407409
pq_putemptymessage('D');
410+
/* any error in old protocol will make us lose sync */
411+
pq_startmsgread();
408412
cstate->copy_dest = COPY_OLD_FE;
409413
}
410414
/* We *must* flush here to ensure FE knows it can send. */
@@ -565,6 +569,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
565569
int mtype;
566570

567571
readmessage:
572+
HOLD_CANCEL_INTERRUPTS();
573+
pq_startmsgread();
568574
mtype = pq_getbyte();
569575
if (mtype == EOF)
570576
ereport(ERROR,
@@ -574,6 +580,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
574580
ereport(ERROR,
575581
(errcode(ERRCODE_CONNECTION_FAILURE),
576582
errmsg("unexpected EOF on client connection with an open transaction")));
583+
RESUME_CANCEL_INTERRUPTS();
577584
switch (mtype)
578585
{
579586
case 'd': /* CopyData */
@@ -2137,6 +2144,13 @@ CopyFrom(CopyState cstate)
21372144

21382145
MemoryContextSwitchTo(oldcontext);
21392146

2147+
/*
2148+
* In the old protocol, tell pqcomm that we can process normal protocol
2149+
* messages again.
2150+
*/
2151+
if (cstate->copy_dest == COPY_OLD_FE)
2152+
pq_endmsgread();
2153+
21402154
/* Execute AFTER STATEMENT insertion triggers */
21412155
ExecASInsertTriggers(estate, resultRelInfo);
21422156

‎src/backend/libpq/auth.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,7 @@ recv_password_packet(Port *port)
649649
{
650650
StringInfoData buf;
651651

652+
pq_startmsgread();
652653
if (PG_PROTOCOL_MAJOR(port->proto) >= 3)
653654
{
654655
/* Expect 'p' message type */
@@ -1054,6 +1055,7 @@ pg_GSS_recvauth(Port *port)
10541055
*/
10551056
do
10561057
{
1058+
pq_startmsgread();
10571059
mtype = pq_getbyte();
10581060
if (mtype != 'p')
10591061
{
@@ -1292,6 +1294,7 @@ pg_SSPI_recvauth(Port *port)
12921294
*/
12931295
do
12941296
{
1297+
pq_startmsgread();
12951298
mtype = pq_getbyte();
12961299
if (mtype != 'p')
12971300
{

‎src/backend/libpq/pqcomm.c

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,9 @@ static int PqRecvLength; /* End of data available in PqRecvBuffer */
129129
/*
130130
* Message status
131131
*/
132-
static bool PqCommBusy;
133-
static bool DoingCopyOut;
132+
static bool PqCommBusy; /* busy sending data to the client */
133+
static bool PqCommReadingMsg; /* in the middle of reading a message */
134+
static bool DoingCopyOut; /* in old-protocol COPY OUT processing */
134135

135136

136137
/* Internal functions */
@@ -156,6 +157,7 @@ pq_init(void)
156157
PqSendBuffer = MemoryContextAlloc(TopMemoryContext, PqSendBufferSize);
157158
PqSendPointer = PqSendStart = PqRecvPointer = PqRecvLength = 0;
158159
PqCommBusy = false;
160+
PqCommReadingMsg = false;
159161
DoingCopyOut = false;
160162
on_proc_exit(pq_close, 0);
161163
}
@@ -860,6 +862,8 @@ pq_recvbuf(void)
860862
int
861863
pq_getbyte(void)
862864
{
865+
Assert(PqCommReadingMsg);
866+
863867
while (PqRecvPointer >= PqRecvLength)
864868
{
865869
if (pq_recvbuf()) /* If nothing in buffer, then recv some */
@@ -898,6 +902,8 @@ pq_getbyte_if_available(unsigned char *c)
898902
{
899903
int r;
900904

905+
Assert(PqCommReadingMsg);
906+
901907
if (PqRecvPointer < PqRecvLength)
902908
{
903909
*c = PqRecvBuffer[PqRecvPointer++];
@@ -950,6 +956,8 @@ pq_getbytes(char *s, size_t len)
950956
{
951957
size_t amount;
952958

959+
Assert(PqCommReadingMsg);
960+
953961
while (len > 0)
954962
{
955963
while (PqRecvPointer >= PqRecvLength)
@@ -982,6 +990,8 @@ pq_discardbytes(size_t len)
982990
{
983991
size_t amount;
984992

993+
Assert(PqCommReadingMsg);
994+
985995
while (len > 0)
986996
{
987997
while (PqRecvPointer >= PqRecvLength)
@@ -1018,6 +1028,8 @@ pq_getstring(StringInfo s)
10181028
{
10191029
int i;
10201030

1031+
Assert(PqCommReadingMsg);
1032+
10211033
resetStringInfo(s);
10221034

10231035
/* Read until we get the terminating '\0' */
@@ -1049,6 +1061,58 @@ pq_getstring(StringInfo s)
10491061
}
10501062

10511063

1064+
/* --------------------------------
1065+
* pq_startmsgread - begin reading a message from the client.
1066+
*
1067+
* This must be called before any of the pq_get* functions.
1068+
* --------------------------------
1069+
*/
1070+
void
1071+
pq_startmsgread(void)
1072+
{
1073+
/*
1074+
* There shouldn't be a read active already, but let's check just to be
1075+
* sure.
1076+
*/
1077+
if (PqCommReadingMsg)
1078+
ereport(FATAL,
1079+
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1080+
errmsg("terminating connection because protocol sync was lost")));
1081+
1082+
PqCommReadingMsg = true;
1083+
}
1084+
1085+
1086+
/* --------------------------------
1087+
* pq_endmsgread - finish reading message.
1088+
*
1089+
* This must be called after reading a V2 protocol message with
1090+
* pq_getstring() and friends, to indicate that we have read the whole
1091+
* message. In V3 protocol, pq_getmessage() does this implicitly.
1092+
* --------------------------------
1093+
*/
1094+
void
1095+
pq_endmsgread(void)
1096+
{
1097+
Assert(PqCommReadingMsg);
1098+
1099+
PqCommReadingMsg = false;
1100+
}
1101+
1102+
/* --------------------------------
1103+
* pq_is_reading_msg - are we currently reading a message?
1104+
*
1105+
* This is used in error recovery at the outer idle loop to detect if we have
1106+
* lost protocol sync, and need to terminate the connection. pq_startmsgread()
1107+
* will check for that too, but it's nicer to detect it earlier.
1108+
* --------------------------------
1109+
*/
1110+
bool
1111+
pq_is_reading_msg(void)
1112+
{
1113+
return PqCommReadingMsg;
1114+
}
1115+
10521116
/* --------------------------------
10531117
* pq_getmessage - get a message with length word from connection
10541118
*
@@ -1070,6 +1134,8 @@ pq_getmessage(StringInfo s, int maxlen)
10701134
{
10711135
int32 len;
10721136

1137+
Assert(PqCommReadingMsg);
1138+
10731139
resetStringInfo(s);
10741140

10751141
/* Read message length word */
@@ -1111,6 +1177,9 @@ pq_getmessage(StringInfo s, int maxlen)
11111177
ereport(COMMERROR,
11121178
(errcode(ERRCODE_PROTOCOL_VIOLATION),
11131179
errmsg("incomplete message from client")));
1180+
1181+
/* we discarded the rest of the message so we're back in sync. */
1182+
PqCommReadingMsg = false;
11141183
PG_RE_THROW();
11151184
}
11161185
PG_END_TRY();
@@ -1128,6 +1197,9 @@ pq_getmessage(StringInfo s, int maxlen)
11281197
s->data[len] = '\0';
11291198
}
11301199

1200+
/* finished reading the message. */
1201+
PqCommReadingMsg = false;
1202+
11311203
return 0;
11321204
}
11331205

‎src/backend/postmaster/postmaster.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1523,6 +1523,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
15231523
ProtocolVersion proto;
15241524
MemoryContext oldcontext;
15251525

1526+
pq_startmsgread();
15261527
if (pq_getbytes((char *) &len, 4) == EOF)
15271528
{
15281529
/*
@@ -1567,6 +1568,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
15671568
errmsg("incomplete startup packet")));
15681569
return STATUS_ERROR;
15691570
}
1571+
pq_endmsgread();
15701572

15711573
/*
15721574
* The first field is either a protocol version number or a special

‎src/backend/replication/walsender.c

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -216,8 +216,16 @@ WalSndHandshake(void)
216216
set_ps_display("idle", false);
217217

218218
/* Wait for a command to arrive */
219+
pq_startmsgread();
219220
firstchar = pq_getbyte();
220221

222+
/* Read the message contents */
223+
if (firstchar != EOF)
224+
{
225+
if (pq_getmessage(&input_message, 0))
226+
firstchar = EOF; /* suitable message already logged */
227+
}
228+
221229
/*
222230
* Emergency bailout if postmaster has died. This is to avoid the
223231
* necessity for manual cleanup of all postmaster children.
@@ -235,16 +243,6 @@ WalSndHandshake(void)
235243
ProcessConfigFile(PGC_SIGHUP);
236244
}
237245

238-
if (firstchar != EOF)
239-
{
240-
/*
241-
* Read the message contents. This is expected to be done without
242-
* blocking because we've been able to get message type code.
243-
*/
244-
if (pq_getmessage(&input_message, 0))
245-
firstchar = EOF; /* suitable message already logged */
246-
}
247-
248246
/* Handle the very limited subset of commands expected in this phase */
249247
switch (firstchar)
250248
{
@@ -513,6 +511,7 @@ ProcessRepliesIfAny(void)
513511

514512
for (;;)
515513
{
514+
pq_startmsgread();
516515
r = pq_getbyte_if_available(&firstchar);
517516
if (r < 0)
518517
{
@@ -525,9 +524,20 @@ ProcessRepliesIfAny(void)
525524
if (r == 0)
526525
{
527526
/* no data available without blocking */
527+
pq_endmsgread();
528528
break;
529529
}
530530

531+
/* Read the message contents */
532+
resetStringInfo(&reply_message);
533+
if (pq_getmessage(&reply_message, 0))
534+
{
535+
ereport(COMMERROR,
536+
(errcode(ERRCODE_PROTOCOL_VIOLATION),
537+
errmsg("unexpected EOF on standby connection")));
538+
proc_exit(0);
539+
}
540+
531541
/* Handle the very limited subset of commands expected in this phase */
532542
switch (firstchar)
533543
{
@@ -568,19 +578,6 @@ ProcessStandbyMessage(void)
568578
{
569579
char msgtype;
570580

571-
resetStringInfo(&reply_message);
572-
573-
/*
574-
* Read the message contents.
575-
*/
576-
if (pq_getmessage(&reply_message, 0))
577-
{
578-
ereport(COMMERROR,
579-
(errcode(ERRCODE_PROTOCOL_VIOLATION),
580-
errmsg("unexpected EOF on standby connection")));
581-
proc_exit(0);
582-
}
583-
584581
/*
585582
* Check message type from the first byte.
586583
*/

‎src/backend/storage/lmgr/proc.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -665,11 +665,16 @@ LockErrorCleanup(void)
665665
{
666666
LWLockId partitionLock;
667667

668+
HOLD_INTERRUPTS();
669+
668670
AbortStrongLockAcquire();
669671

670672
/* Nothing to do if we weren't waiting for a lock */
671673
if (lockAwaited == NULL)
674+
{
675+
RESUME_INTERRUPTS();
672676
return;
677+
}
673678

674679
/* Turn off the deadlock timer, if it's still running (see ProcSleep) */
675680
disable_sig_alarm(false);
@@ -708,6 +713,8 @@ LockErrorCleanup(void)
708713
* wakeup signal isn't harmful, and it seems not worth expending cycles to
709714
* get rid of a signal that most likely isn't there.
710715
*/
716+
717+
RESUME_INTERRUPTS();
711718
}
712719

713720

‎src/backend/tcop/fastpath.c

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ static int16 parse_fcall_arguments_20(StringInfo msgBuf, struct fp_info * fip,
7373
* The caller should already have initialized buf to empty.
7474
* ----------------
7575
*/
76-
static int
76+
int
7777
GetOldFunctionMessage(StringInfo buf)
7878
{
7979
int32 ibuf;
@@ -278,33 +278,6 @@ HandleFunctionRequest(StringInfo msgBuf)
278278
bool was_logged = false;
279279
char msec_str[32];
280280

281-
/*
282-
* Read message contents if not already done.
283-
*/
284-
if (PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
285-
{
286-
if (GetOldFunctionMessage(msgBuf))
287-
{
288-
if (IsTransactionState())
289-
ereport(COMMERROR,
290-
(errcode(ERRCODE_CONNECTION_FAILURE),
291-
errmsg("unexpected EOF on client connection with an open transaction")));
292-
else
293-
{
294-
/*
295-
* Can't send DEBUG log messages to client at this point.
296-
* Since we're disconnecting right away, we don't need to
297-
* restore whereToSendOutput.
298-
*/
299-
whereToSendOutput = DestNone;
300-
ereport(DEBUG1,
301-
(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
302-
errmsg("unexpected EOF on client connection")));
303-
}
304-
return EOF;
305-
}
306-
}
307-
308281
/*
309282
* Now that we've eaten the input message, check to see if we actually
310283
* want to do the function call or not. It's now safe to ereport(); we

0 commit comments

Comments
 (0)