Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2b3a8b2

Browse files
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 protocolmessage from the client, we could lose sync, and incorrectly try tointerpret a part of another message as a new protocol message. That willusually lead to an "invalid frontend message" error that terminates theconnection. However, this is a security issue because an attacker mightbe able to deliberately cause an error, inject a Query message in what'ssupposed to be just user data, and have the server execute it.We were quite careful to not have CHECK_FOR_INTERRUPTS() calls or otheroperations that could ereport(ERROR) in the middle of processing a message,but a query cancel interrupt or statement timeout could nevertheless causeit 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 justterminate the connection on error. In practice, that's what happenedpreviously anyway, as we lost protocol sync.To fix, add a new variable in pqcomm.c, PqCommReadingMsg, that is setwhenever we're in the middle of reading a message. When it's set, we cannotsafely ERROR out and continue running, because we might've read only partof a message. PqCommReadingMsg acts somewhat similarly to critical sectionsin that if an error occurs while it's set, the error handler will force theconnection to be terminated, as if the error was FATAL. It's notimplemented by promoting ERROR to FATAL in elog.c, like ERROR is promotedto PANIC in critical sections, because we want to be able to usePG_TRY/CATCH to recover and regain protocol sync. pq_getmessage() takesadvantage of that to prevent an OOM error from terminating the connection.To prevent unnecessary connection terminations, add a holdoff mechanismsimilar to HOLD/RESUME_INTERRUPTS() that can be used hold off query cancelinterrupts, but still allow die interrupts. The rules on which interruptsare processed when are now a bit more complicated, so refactorProcessInterrupts() and the calls to it in signal handlers so that thesignal handlers always call it if ImmediateInterruptOK is set, andProcessInterrupts() can decide to not do anything if the other conditionsare not met.Reported by Emil Lenngren. Patch reviewed by Noah Misch and Andres Freund.Backpatch to all supported versions.Security:CVE-2015-0244
1 parent59b9198 commit2b3a8b2

File tree

13 files changed

+244
-107
lines changed

13 files changed

+244
-107
lines changed

‎src/backend/commands/copy.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,8 @@ ReceiveCopyBegin(CopyState cstate)
410410
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
411411
errmsg("COPY BINARY is not supported to stdout or from stdin")));
412412
pq_putemptymessage('G');
413+
/* any error in old protocol will make us lose sync */
414+
pq_startmsgread();
413415
cstate->copy_dest=COPY_OLD_FE;
414416
}
415417
else
@@ -420,6 +422,8 @@ ReceiveCopyBegin(CopyState cstate)
420422
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
421423
errmsg("COPY BINARY is not supported to stdout or from stdin")));
422424
pq_putemptymessage('D');
425+
/* any error in old protocol will make us lose sync */
426+
pq_startmsgread();
423427
cstate->copy_dest=COPY_OLD_FE;
424428
}
425429
/* We *must* flush here to ensure FE knows it can send. */
@@ -606,6 +610,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
606610
intmtype;
607611

608612
readmessage:
613+
HOLD_CANCEL_INTERRUPTS();
614+
pq_startmsgread();
609615
mtype=pq_getbyte();
610616
if (mtype==EOF)
611617
ereport(ERROR,
@@ -615,6 +621,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
615621
ereport(ERROR,
616622
(errcode(ERRCODE_CONNECTION_FAILURE),
617623
errmsg("unexpected EOF on client connection with an open transaction")));
624+
RESUME_CANCEL_INTERRUPTS();
618625
switch (mtype)
619626
{
620627
case'd':/* CopyData */
@@ -2463,6 +2470,13 @@ CopyFrom(CopyState cstate)
24632470

24642471
MemoryContextSwitchTo(oldcontext);
24652472

2473+
/*
2474+
* In the old protocol, tell pqcomm that we can process normal protocol
2475+
* messages again.
2476+
*/
2477+
if (cstate->copy_dest==COPY_OLD_FE)
2478+
pq_endmsgread();
2479+
24662480
/* Execute AFTER STATEMENT insertion triggers */
24672481
ExecASInsertTriggers(estate,resultRelInfo);
24682482

‎src/backend/libpq/auth.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,7 @@ recv_password_packet(Port *port)
625625
{
626626
StringInfoDatabuf;
627627

628+
pq_startmsgread();
628629
if (PG_PROTOCOL_MAJOR(port->proto) >=3)
629630
{
630631
/* Expect 'p' message type */
@@ -849,6 +850,7 @@ pg_GSS_recvauth(Port *port)
849850
*/
850851
do
851852
{
853+
pq_startmsgread();
852854
mtype=pq_getbyte();
853855
if (mtype!='p')
854856
{
@@ -1083,6 +1085,7 @@ pg_SSPI_recvauth(Port *port)
10831085
*/
10841086
do
10851087
{
1088+
pq_startmsgread();
10861089
mtype=pq_getbyte();
10871090
if (mtype!='p')
10881091
{

‎src/backend/libpq/pqcomm.c

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,9 @@ static intPqRecvLength;/* End of data available in PqRecvBuffer */
127127
/*
128128
* Message status
129129
*/
130-
staticboolPqCommBusy;
131-
staticboolDoingCopyOut;
130+
staticboolPqCommBusy;/* busy sending data to the client */
131+
staticboolPqCommReadingMsg;/* in the middle of reading a message */
132+
staticboolDoingCopyOut;/* in old-protocol COPY OUT processing */
132133

133134

134135
/* Internal functions */
@@ -177,6 +178,7 @@ pq_init(void)
177178
PqSendBuffer=MemoryContextAlloc(TopMemoryContext,PqSendBufferSize);
178179
PqSendPointer=PqSendStart=PqRecvPointer=PqRecvLength=0;
179180
PqCommBusy= false;
181+
PqCommReadingMsg= false;
180182
DoingCopyOut= false;
181183
on_proc_exit(socket_close,0);
182184
}
@@ -916,6 +918,8 @@ pq_recvbuf(void)
916918
int
917919
pq_getbyte(void)
918920
{
921+
Assert(PqCommReadingMsg);
922+
919923
while (PqRecvPointer >=PqRecvLength)
920924
{
921925
if (pq_recvbuf())/* If nothing in buffer, then recv some */
@@ -954,6 +958,8 @@ pq_getbyte_if_available(unsigned char *c)
954958
{
955959
intr;
956960

961+
Assert(PqCommReadingMsg);
962+
957963
if (PqRecvPointer<PqRecvLength)
958964
{
959965
*c=PqRecvBuffer[PqRecvPointer++];
@@ -1006,6 +1012,8 @@ pq_getbytes(char *s, size_t len)
10061012
{
10071013
size_tamount;
10081014

1015+
Assert(PqCommReadingMsg);
1016+
10091017
while (len>0)
10101018
{
10111019
while (PqRecvPointer >=PqRecvLength)
@@ -1038,6 +1046,8 @@ pq_discardbytes(size_t len)
10381046
{
10391047
size_tamount;
10401048

1049+
Assert(PqCommReadingMsg);
1050+
10411051
while (len>0)
10421052
{
10431053
while (PqRecvPointer >=PqRecvLength)
@@ -1074,6 +1084,8 @@ pq_getstring(StringInfo s)
10741084
{
10751085
inti;
10761086

1087+
Assert(PqCommReadingMsg);
1088+
10771089
resetStringInfo(s);
10781090

10791091
/* Read until we get the terminating '\0' */
@@ -1105,6 +1117,58 @@ pq_getstring(StringInfo s)
11051117
}
11061118

11071119

1120+
/* --------------------------------
1121+
*pq_startmsgread- begin reading a message from the client.
1122+
*
1123+
*This must be called before any of the pq_get* functions.
1124+
* --------------------------------
1125+
*/
1126+
void
1127+
pq_startmsgread(void)
1128+
{
1129+
/*
1130+
* There shouldn't be a read active already, but let's check just to be
1131+
* sure.
1132+
*/
1133+
if (PqCommReadingMsg)
1134+
ereport(FATAL,
1135+
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1136+
errmsg("terminating connection because protocol sync was lost")));
1137+
1138+
PqCommReadingMsg= true;
1139+
}
1140+
1141+
1142+
/* --------------------------------
1143+
*pq_endmsgread- finish reading message.
1144+
*
1145+
*This must be called after reading a V2 protocol message with
1146+
*pq_getstring() and friends, to indicate that we have read the whole
1147+
*message. In V3 protocol, pq_getmessage() does this implicitly.
1148+
* --------------------------------
1149+
*/
1150+
void
1151+
pq_endmsgread(void)
1152+
{
1153+
Assert(PqCommReadingMsg);
1154+
1155+
PqCommReadingMsg= false;
1156+
}
1157+
1158+
/* --------------------------------
1159+
*pq_is_reading_msg - are we currently reading a message?
1160+
*
1161+
* This is used in error recovery at the outer idle loop to detect if we have
1162+
* lost protocol sync, and need to terminate the connection. pq_startmsgread()
1163+
* will check for that too, but it's nicer to detect it earlier.
1164+
* --------------------------------
1165+
*/
1166+
bool
1167+
pq_is_reading_msg(void)
1168+
{
1169+
returnPqCommReadingMsg;
1170+
}
1171+
11081172
/* --------------------------------
11091173
*pq_getmessage- get a message with length word from connection
11101174
*
@@ -1126,6 +1190,8 @@ pq_getmessage(StringInfo s, int maxlen)
11261190
{
11271191
int32len;
11281192

1193+
Assert(PqCommReadingMsg);
1194+
11291195
resetStringInfo(s);
11301196

11311197
/* Read message length word */
@@ -1167,6 +1233,9 @@ pq_getmessage(StringInfo s, int maxlen)
11671233
ereport(COMMERROR,
11681234
(errcode(ERRCODE_PROTOCOL_VIOLATION),
11691235
errmsg("incomplete message from client")));
1236+
1237+
/* we discarded the rest of the message so we're back in sync. */
1238+
PqCommReadingMsg= false;
11701239
PG_RE_THROW();
11711240
}
11721241
PG_END_TRY();
@@ -1184,6 +1253,9 @@ pq_getmessage(StringInfo s, int maxlen)
11841253
s->data[len]='\0';
11851254
}
11861255

1256+
/* finished reading the message. */
1257+
PqCommReadingMsg= false;
1258+
11871259
return0;
11881260
}
11891261

‎src/backend/postmaster/postmaster.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1761,6 +1761,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
17611761
ProtocolVersionproto;
17621762
MemoryContextoldcontext;
17631763

1764+
pq_startmsgread();
17641765
if (pq_getbytes((char*)&len,4)==EOF)
17651766
{
17661767
/*
@@ -1805,6 +1806,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
18051806
errmsg("incomplete startup packet")));
18061807
returnSTATUS_ERROR;
18071808
}
1809+
pq_endmsgread();
18081810

18091811
/*
18101812
* The first field is either a protocol version number or a special

‎src/backend/replication/walsender.c

Lines changed: 12 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1357,6 +1357,7 @@ ProcessRepliesIfAny(void)
13571357

13581358
for (;;)
13591359
{
1360+
pq_startmsgread();
13601361
r=pq_getbyte_if_available(&firstchar);
13611362
if (r<0)
13621363
{
@@ -1369,9 +1370,20 @@ ProcessRepliesIfAny(void)
13691370
if (r==0)
13701371
{
13711372
/* no data available without blocking */
1373+
pq_endmsgread();
13721374
break;
13731375
}
13741376

1377+
/* Read the message contents */
1378+
resetStringInfo(&reply_message);
1379+
if (pq_getmessage(&reply_message,0))
1380+
{
1381+
ereport(COMMERROR,
1382+
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1383+
errmsg("unexpected EOF on standby connection")));
1384+
proc_exit(0);
1385+
}
1386+
13751387
/*
13761388
* If we already received a CopyDone from the frontend, the frontend
13771389
* should not send us anything until we've closed our end of the COPY.
@@ -1407,16 +1419,6 @@ ProcessRepliesIfAny(void)
14071419
streamingDoneSending= true;
14081420
}
14091421

1410-
/* consume the CopyData message */
1411-
resetStringInfo(&reply_message);
1412-
if (pq_getmessage(&reply_message,0))
1413-
{
1414-
ereport(COMMERROR,
1415-
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1416-
errmsg("unexpected EOF on standby connection")));
1417-
proc_exit(0);
1418-
}
1419-
14201422
streamingDoneReceiving= true;
14211423
received= true;
14221424
break;
@@ -1453,19 +1455,6 @@ ProcessStandbyMessage(void)
14531455
{
14541456
charmsgtype;
14551457

1456-
resetStringInfo(&reply_message);
1457-
1458-
/*
1459-
* Read the message contents.
1460-
*/
1461-
if (pq_getmessage(&reply_message,0))
1462-
{
1463-
ereport(COMMERROR,
1464-
(errcode(ERRCODE_PROTOCOL_VIOLATION),
1465-
errmsg("unexpected EOF on standby connection")));
1466-
proc_exit(0);
1467-
}
1468-
14691458
/*
14701459
* Check message type from the first byte.
14711460
*/

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -655,11 +655,16 @@ LockErrorCleanup(void)
655655
LWLock*partitionLock;
656656
DisableTimeoutParamstimeouts[2];
657657

658+
HOLD_INTERRUPTS();
659+
658660
AbortStrongLockAcquire();
659661

660662
/* Nothing to do if we weren't waiting for a lock */
661663
if (lockAwaited==NULL)
664+
{
665+
RESUME_INTERRUPTS();
662666
return;
667+
}
663668

664669
/*
665670
* Turn off the deadlock and lock timeout timers, if they are still
@@ -709,6 +714,8 @@ LockErrorCleanup(void)
709714
* wakeup signal isn't harmful, and it seems not worth expending cycles to
710715
* get rid of a signal that most likely isn't there.
711716
*/
717+
718+
RESUME_INTERRUPTS();
712719
}
713720

714721

‎src/backend/tcop/fastpath.c

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ static int16 parse_fcall_arguments_20(StringInfo msgBuf, struct fp_info * fip,
7575
* The caller should already have initialized buf to empty.
7676
* ----------------
7777
*/
78-
staticint
78+
int
7979
GetOldFunctionMessage(StringInfobuf)
8080
{
8181
int32ibuf;
@@ -280,33 +280,6 @@ HandleFunctionRequest(StringInfo msgBuf)
280280
boolwas_logged= false;
281281
charmsec_str[32];
282282

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

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp