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

Commitaf9c5c0

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 parent8d412e0 commitaf9c5c0

File tree

13 files changed

+236
-93
lines changed

13 files changed

+236
-93
lines changed

‎src/backend/commands/copy.c

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,8 @@ ReceiveCopyBegin(CopyState cstate)
385385
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
386386
errmsg("COPY BINARY is not supported to stdout or from stdin")));
387387
pq_putemptymessage('G');
388+
/* any error in old protocol will make us lose sync */
389+
pq_startmsgread();
388390
cstate->copy_dest=COPY_OLD_FE;
389391
}
390392
else
@@ -395,6 +397,8 @@ ReceiveCopyBegin(CopyState cstate)
395397
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
396398
errmsg("COPY BINARY is not supported to stdout or from stdin")));
397399
pq_putemptymessage('D');
400+
/* any error in old protocol will make us lose sync */
401+
pq_startmsgread();
398402
cstate->copy_dest=COPY_OLD_FE;
399403
}
400404
/* We *must* flush here to ensure FE knows it can send. */
@@ -555,6 +559,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
555559
intmtype;
556560

557561
readmessage:
562+
HOLD_CANCEL_INTERRUPTS();
563+
pq_startmsgread();
558564
mtype=pq_getbyte();
559565
if (mtype==EOF)
560566
ereport(ERROR,
@@ -564,6 +570,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
564570
ereport(ERROR,
565571
(errcode(ERRCODE_CONNECTION_FAILURE),
566572
errmsg("unexpected EOF on client connection")));
573+
RESUME_CANCEL_INTERRUPTS();
567574
switch (mtype)
568575
{
569576
case'd':/* CopyData */
@@ -2049,6 +2056,13 @@ CopyFrom(CopyState cstate)
20492056

20502057
MemoryContextSwitchTo(oldcontext);
20512058

2059+
/*
2060+
* In the old protocol, tell pqcomm that we can process normal protocol
2061+
* messages again.
2062+
*/
2063+
if (cstate->copy_dest==COPY_OLD_FE)
2064+
pq_endmsgread();
2065+
20522066
/* Execute AFTER STATEMENT insertion triggers */
20532067
ExecASInsertTriggers(estate,resultRelInfo);
20542068

‎src/backend/libpq/auth.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,7 @@ recv_password_packet(Port *port)
653653
{
654654
StringInfoDatabuf;
655655

656+
pq_startmsgread();
656657
if (PG_PROTOCOL_MAJOR(port->proto) >=3)
657658
{
658659
/* Expect 'p' message type */
@@ -1058,6 +1059,7 @@ pg_GSS_recvauth(Port *port)
10581059
*/
10591060
do
10601061
{
1062+
pq_startmsgread();
10611063
mtype=pq_getbyte();
10621064
if (mtype!='p')
10631065
{
@@ -1296,6 +1298,7 @@ pg_SSPI_recvauth(Port *port)
12961298
*/
12971299
do
12981300
{
1301+
pq_startmsgread();
12991302
mtype=pq_getbyte();
13001303
if (mtype!='p')
13011304
{

‎src/backend/libpq/pqcomm.c

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,9 @@ static intPqRecvLength;/* End of data available in PqRecvBuffer */
129129
/*
130130
* Message status
131131
*/
132-
staticboolPqCommBusy;
133-
staticboolDoingCopyOut;
132+
staticboolPqCommBusy;/* busy sending data to the client */
133+
staticboolPqCommReadingMsg;/* in the middle of reading a message */
134+
staticboolDoingCopyOut;/* 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
intr;
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_tamount;
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_tamount;
984992

993+
Assert(PqCommReadingMsg);
994+
985995
while (len>0)
986996
{
987997
while (PqRecvPointer >=PqRecvLength)
@@ -1018,6 +1028,8 @@ pq_getstring(StringInfo s)
10181028
{
10191029
inti;
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+
returnPqCommReadingMsg;
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
int32len;
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
return0;
11321204
}
11331205

‎src/backend/postmaster/postmaster.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,6 +1617,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
16171617
ProtocolVersionproto;
16181618
MemoryContextoldcontext;
16191619

1620+
pq_startmsgread();
16201621
if (pq_getbytes((char*)&len,4)==EOF)
16211622
{
16221623
/*
@@ -1661,6 +1662,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
16611662
errmsg("incomplete startup packet")));
16621663
returnSTATUS_ERROR;
16631664
}
1665+
pq_endmsgread();
16641666

16651667
/*
16661668
* 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
@@ -207,8 +207,16 @@ WalSndHandshake(void)
207207
set_ps_display("idle", false);
208208

209209
/* Wait for a command to arrive */
210+
pq_startmsgread();
210211
firstchar=pq_getbyte();
211212

213+
/* Read the message contents */
214+
if (firstchar!=EOF)
215+
{
216+
if (pq_getmessage(&input_message,0))
217+
firstchar=EOF;/* suitable message already logged */
218+
}
219+
212220
/*
213221
* Emergency bailout if postmaster has died. This is to avoid the
214222
* necessity for manual cleanup of all postmaster children.
@@ -226,16 +234,6 @@ WalSndHandshake(void)
226234
ProcessConfigFile(PGC_SIGHUP);
227235
}
228236

229-
if (firstchar!=EOF)
230-
{
231-
/*
232-
* Read the message contents. This is expected to be done without
233-
* blocking because we've been able to get message type code.
234-
*/
235-
if (pq_getmessage(&input_message,0))
236-
firstchar=EOF;/* suitable message already logged */
237-
}
238-
239237
/* Handle the very limited subset of commands expected in this phase */
240238
switch (firstchar)
241239
{
@@ -481,6 +479,7 @@ ProcessRepliesIfAny(void)
481479

482480
for (;;)
483481
{
482+
pq_startmsgread();
484483
r=pq_getbyte_if_available(&firstchar);
485484
if (r<0)
486485
{
@@ -493,9 +492,20 @@ ProcessRepliesIfAny(void)
493492
if (r==0)
494493
{
495494
/* no data available without blocking */
495+
pq_endmsgread();
496496
break;
497497
}
498498

499+
/* Read the message contents */
500+
resetStringInfo(&reply_message);
501+
if (pq_getmessage(&reply_message,0))
502+
{
503+
ereport(COMMERROR,
504+
(errcode(ERRCODE_PROTOCOL_VIOLATION),
505+
errmsg("unexpected EOF on standby connection")));
506+
proc_exit(0);
507+
}
508+
499509
/* Handle the very limited subset of commands expected in this phase */
500510
switch (firstchar)
501511
{
@@ -536,19 +546,6 @@ ProcessStandbyMessage(void)
536546
{
537547
charmsgtype;
538548

539-
resetStringInfo(&reply_message);
540-
541-
/*
542-
* Read the message contents.
543-
*/
544-
if (pq_getmessage(&reply_message,0))
545-
{
546-
ereport(COMMERROR,
547-
(errcode(ERRCODE_PROTOCOL_VIOLATION),
548-
errmsg("unexpected EOF on standby connection")));
549-
proc_exit(0);
550-
}
551-
552549
/*
553550
* Check message type from the first byte.
554551
*/

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -608,9 +608,14 @@ LockWaitCancel(void)
608608
{
609609
LWLockIdpartitionLock;
610610

611+
HOLD_INTERRUPTS();
612+
611613
/* Nothing to do if we weren't waiting for a lock */
612614
if (lockAwaited==NULL)
615+
{
616+
RESUME_INTERRUPTS();
613617
return;
618+
}
614619

615620
/* Turn off the deadlock timer, if it's still running (see ProcSleep) */
616621
disable_sig_alarm(false);
@@ -649,6 +654,8 @@ LockWaitCancel(void)
649654
* wakeup signal isn't harmful, and it seems not worth expending cycles to
650655
* get rid of a signal that most likely isn't there.
651656
*/
657+
658+
RESUME_INTERRUPTS();
652659
}
653660

654661

‎src/backend/tcop/fastpath.c

Lines changed: 1 addition & 15 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-
staticint
76+
int
7777
GetOldFunctionMessage(StringInfobuf)
7878
{
7979
int32ibuf;
@@ -278,20 +278,6 @@ HandleFunctionRequest(StringInfo msgBuf)
278278
boolwas_logged= false;
279279
charmsec_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-
ereport(COMMERROR,
289-
(errcode(ERRCODE_PROTOCOL_VIOLATION),
290-
errmsg("unexpected EOF on client connection")));
291-
returnEOF;
292-
}
293-
}
294-
295281
/*
296282
* Now that we've eaten the input message, check to see if we actually
297283
* 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