Commit 2b3a8b20 authored by Heikki Linnakangas's avatar Heikki Linnakangas

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
parent 59b91982
...@@ -410,6 +410,8 @@ ReceiveCopyBegin(CopyState cstate) ...@@ -410,6 +410,8 @@ ReceiveCopyBegin(CopyState cstate)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED), (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY BINARY is not supported to stdout or from stdin"))); errmsg("COPY BINARY is not supported to stdout or from stdin")));
pq_putemptymessage('G'); pq_putemptymessage('G');
/* any error in old protocol will make us lose sync */
pq_startmsgread();
cstate->copy_dest = COPY_OLD_FE; cstate->copy_dest = COPY_OLD_FE;
} }
else else
...@@ -420,6 +422,8 @@ ReceiveCopyBegin(CopyState cstate) ...@@ -420,6 +422,8 @@ ReceiveCopyBegin(CopyState cstate)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED), (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("COPY BINARY is not supported to stdout or from stdin"))); errmsg("COPY BINARY is not supported to stdout or from stdin")));
pq_putemptymessage('D'); pq_putemptymessage('D');
/* any error in old protocol will make us lose sync */
pq_startmsgread();
cstate->copy_dest = COPY_OLD_FE; cstate->copy_dest = COPY_OLD_FE;
} }
/* We *must* flush here to ensure FE knows it can send. */ /* We *must* flush here to ensure FE knows it can send. */
...@@ -606,6 +610,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread) ...@@ -606,6 +610,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
int mtype; int mtype;
readmessage: readmessage:
HOLD_CANCEL_INTERRUPTS();
pq_startmsgread();
mtype = pq_getbyte(); mtype = pq_getbyte();
if (mtype == EOF) if (mtype == EOF)
ereport(ERROR, ereport(ERROR,
...@@ -615,6 +621,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread) ...@@ -615,6 +621,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_CONNECTION_FAILURE), (errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("unexpected EOF on client connection with an open transaction"))); errmsg("unexpected EOF on client connection with an open transaction")));
RESUME_CANCEL_INTERRUPTS();
switch (mtype) switch (mtype)
{ {
case 'd': /* CopyData */ case 'd': /* CopyData */
...@@ -2463,6 +2470,13 @@ CopyFrom(CopyState cstate) ...@@ -2463,6 +2470,13 @@ CopyFrom(CopyState cstate)
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);
/*
* In the old protocol, tell pqcomm that we can process normal protocol
* messages again.
*/
if (cstate->copy_dest == COPY_OLD_FE)
pq_endmsgread();
/* Execute AFTER STATEMENT insertion triggers */ /* Execute AFTER STATEMENT insertion triggers */
ExecASInsertTriggers(estate, resultRelInfo); ExecASInsertTriggers(estate, resultRelInfo);
......
...@@ -625,6 +625,7 @@ recv_password_packet(Port *port) ...@@ -625,6 +625,7 @@ recv_password_packet(Port *port)
{ {
StringInfoData buf; StringInfoData buf;
pq_startmsgread();
if (PG_PROTOCOL_MAJOR(port->proto) >= 3) if (PG_PROTOCOL_MAJOR(port->proto) >= 3)
{ {
/* Expect 'p' message type */ /* Expect 'p' message type */
...@@ -849,6 +850,7 @@ pg_GSS_recvauth(Port *port) ...@@ -849,6 +850,7 @@ pg_GSS_recvauth(Port *port)
*/ */
do do
{ {
pq_startmsgread();
mtype = pq_getbyte(); mtype = pq_getbyte();
if (mtype != 'p') if (mtype != 'p')
{ {
...@@ -1083,6 +1085,7 @@ pg_SSPI_recvauth(Port *port) ...@@ -1083,6 +1085,7 @@ pg_SSPI_recvauth(Port *port)
*/ */
do do
{ {
pq_startmsgread();
mtype = pq_getbyte(); mtype = pq_getbyte();
if (mtype != 'p') if (mtype != 'p')
{ {
......
...@@ -127,8 +127,9 @@ static int PqRecvLength; /* End of data available in PqRecvBuffer */ ...@@ -127,8 +127,9 @@ static int PqRecvLength; /* End of data available in PqRecvBuffer */
/* /*
* Message status * Message status
*/ */
static bool PqCommBusy; static bool PqCommBusy; /* busy sending data to the client */
static bool DoingCopyOut; static bool PqCommReadingMsg; /* in the middle of reading a message */
static bool DoingCopyOut; /* in old-protocol COPY OUT processing */
/* Internal functions */ /* Internal functions */
...@@ -177,6 +178,7 @@ pq_init(void) ...@@ -177,6 +178,7 @@ pq_init(void)
PqSendBuffer = MemoryContextAlloc(TopMemoryContext, PqSendBufferSize); PqSendBuffer = MemoryContextAlloc(TopMemoryContext, PqSendBufferSize);
PqSendPointer = PqSendStart = PqRecvPointer = PqRecvLength = 0; PqSendPointer = PqSendStart = PqRecvPointer = PqRecvLength = 0;
PqCommBusy = false; PqCommBusy = false;
PqCommReadingMsg = false;
DoingCopyOut = false; DoingCopyOut = false;
on_proc_exit(socket_close, 0); on_proc_exit(socket_close, 0);
} }
...@@ -916,6 +918,8 @@ pq_recvbuf(void) ...@@ -916,6 +918,8 @@ pq_recvbuf(void)
int int
pq_getbyte(void) pq_getbyte(void)
{ {
Assert(PqCommReadingMsg);
while (PqRecvPointer >= PqRecvLength) while (PqRecvPointer >= PqRecvLength)
{ {
if (pq_recvbuf()) /* If nothing in buffer, then recv some */ if (pq_recvbuf()) /* If nothing in buffer, then recv some */
...@@ -954,6 +958,8 @@ pq_getbyte_if_available(unsigned char *c) ...@@ -954,6 +958,8 @@ pq_getbyte_if_available(unsigned char *c)
{ {
int r; int r;
Assert(PqCommReadingMsg);
if (PqRecvPointer < PqRecvLength) if (PqRecvPointer < PqRecvLength)
{ {
*c = PqRecvBuffer[PqRecvPointer++]; *c = PqRecvBuffer[PqRecvPointer++];
...@@ -1006,6 +1012,8 @@ pq_getbytes(char *s, size_t len) ...@@ -1006,6 +1012,8 @@ pq_getbytes(char *s, size_t len)
{ {
size_t amount; size_t amount;
Assert(PqCommReadingMsg);
while (len > 0) while (len > 0)
{ {
while (PqRecvPointer >= PqRecvLength) while (PqRecvPointer >= PqRecvLength)
...@@ -1038,6 +1046,8 @@ pq_discardbytes(size_t len) ...@@ -1038,6 +1046,8 @@ pq_discardbytes(size_t len)
{ {
size_t amount; size_t amount;
Assert(PqCommReadingMsg);
while (len > 0) while (len > 0)
{ {
while (PqRecvPointer >= PqRecvLength) while (PqRecvPointer >= PqRecvLength)
...@@ -1074,6 +1084,8 @@ pq_getstring(StringInfo s) ...@@ -1074,6 +1084,8 @@ pq_getstring(StringInfo s)
{ {
int i; int i;
Assert(PqCommReadingMsg);
resetStringInfo(s); resetStringInfo(s);
/* Read until we get the terminating '\0' */ /* Read until we get the terminating '\0' */
...@@ -1105,6 +1117,58 @@ pq_getstring(StringInfo s) ...@@ -1105,6 +1117,58 @@ pq_getstring(StringInfo s)
} }
/* --------------------------------
* pq_startmsgread - begin reading a message from the client.
*
* This must be called before any of the pq_get* functions.
* --------------------------------
*/
void
pq_startmsgread(void)
{
/*
* There shouldn't be a read active already, but let's check just to be
* sure.
*/
if (PqCommReadingMsg)
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("terminating connection because protocol sync was lost")));
PqCommReadingMsg = true;
}
/* --------------------------------
* pq_endmsgread - finish reading message.
*
* This must be called after reading a V2 protocol message with
* pq_getstring() and friends, to indicate that we have read the whole
* message. In V3 protocol, pq_getmessage() does this implicitly.
* --------------------------------
*/
void
pq_endmsgread(void)
{
Assert(PqCommReadingMsg);
PqCommReadingMsg = false;
}
/* --------------------------------
* pq_is_reading_msg - are we currently reading a message?
*
* This is used in error recovery at the outer idle loop to detect if we have
* lost protocol sync, and need to terminate the connection. pq_startmsgread()
* will check for that too, but it's nicer to detect it earlier.
* --------------------------------
*/
bool
pq_is_reading_msg(void)
{
return PqCommReadingMsg;
}
/* -------------------------------- /* --------------------------------
* pq_getmessage - get a message with length word from connection * pq_getmessage - get a message with length word from connection
* *
...@@ -1126,6 +1190,8 @@ pq_getmessage(StringInfo s, int maxlen) ...@@ -1126,6 +1190,8 @@ pq_getmessage(StringInfo s, int maxlen)
{ {
int32 len; int32 len;
Assert(PqCommReadingMsg);
resetStringInfo(s); resetStringInfo(s);
/* Read message length word */ /* Read message length word */
...@@ -1167,6 +1233,9 @@ pq_getmessage(StringInfo s, int maxlen) ...@@ -1167,6 +1233,9 @@ pq_getmessage(StringInfo s, int maxlen)
ereport(COMMERROR, ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION), (errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("incomplete message from client"))); errmsg("incomplete message from client")));
/* we discarded the rest of the message so we're back in sync. */
PqCommReadingMsg = false;
PG_RE_THROW(); PG_RE_THROW();
} }
PG_END_TRY(); PG_END_TRY();
...@@ -1184,6 +1253,9 @@ pq_getmessage(StringInfo s, int maxlen) ...@@ -1184,6 +1253,9 @@ pq_getmessage(StringInfo s, int maxlen)
s->data[len] = '\0'; s->data[len] = '\0';
} }
/* finished reading the message. */
PqCommReadingMsg = false;
return 0; return 0;
} }
......
...@@ -1761,6 +1761,7 @@ ProcessStartupPacket(Port *port, bool SSLdone) ...@@ -1761,6 +1761,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
ProtocolVersion proto; ProtocolVersion proto;
MemoryContext oldcontext; MemoryContext oldcontext;
pq_startmsgread();
if (pq_getbytes((char *) &len, 4) == EOF) if (pq_getbytes((char *) &len, 4) == EOF)
{ {
/* /*
...@@ -1805,6 +1806,7 @@ ProcessStartupPacket(Port *port, bool SSLdone) ...@@ -1805,6 +1806,7 @@ ProcessStartupPacket(Port *port, bool SSLdone)
errmsg("incomplete startup packet"))); errmsg("incomplete startup packet")));
return STATUS_ERROR; return STATUS_ERROR;
} }
pq_endmsgread();
/* /*
* The first field is either a protocol version number or a special * The first field is either a protocol version number or a special
......
...@@ -1357,6 +1357,7 @@ ProcessRepliesIfAny(void) ...@@ -1357,6 +1357,7 @@ ProcessRepliesIfAny(void)
for (;;) for (;;)
{ {
pq_startmsgread();
r = pq_getbyte_if_available(&firstchar); r = pq_getbyte_if_available(&firstchar);
if (r < 0) if (r < 0)
{ {
...@@ -1369,9 +1370,20 @@ ProcessRepliesIfAny(void) ...@@ -1369,9 +1370,20 @@ ProcessRepliesIfAny(void)
if (r == 0) if (r == 0)
{ {
/* no data available without blocking */ /* no data available without blocking */
pq_endmsgread();
break; break;
} }
/* Read the message contents */
resetStringInfo(&reply_message);
if (pq_getmessage(&reply_message, 0))
{
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("unexpected EOF on standby connection")));
proc_exit(0);
}
/* /*
* If we already received a CopyDone from the frontend, the frontend * If we already received a CopyDone from the frontend, the frontend
* should not send us anything until we've closed our end of the COPY. * should not send us anything until we've closed our end of the COPY.
...@@ -1407,16 +1419,6 @@ ProcessRepliesIfAny(void) ...@@ -1407,16 +1419,6 @@ ProcessRepliesIfAny(void)
streamingDoneSending = true; streamingDoneSending = true;
} }
/* consume the CopyData message */
resetStringInfo(&reply_message);
if (pq_getmessage(&reply_message, 0))
{
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("unexpected EOF on standby connection")));
proc_exit(0);
}
streamingDoneReceiving = true; streamingDoneReceiving = true;
received = true; received = true;
break; break;
...@@ -1453,19 +1455,6 @@ ProcessStandbyMessage(void) ...@@ -1453,19 +1455,6 @@ ProcessStandbyMessage(void)
{ {
char msgtype; char msgtype;
resetStringInfo(&reply_message);
/*
* Read the message contents.
*/
if (pq_getmessage(&reply_message, 0))
{
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("unexpected EOF on standby connection")));
proc_exit(0);
}
/* /*
* Check message type from the first byte. * Check message type from the first byte.
*/ */
......
...@@ -655,11 +655,16 @@ LockErrorCleanup(void) ...@@ -655,11 +655,16 @@ LockErrorCleanup(void)
LWLock *partitionLock; LWLock *partitionLock;
DisableTimeoutParams timeouts[2]; DisableTimeoutParams timeouts[2];
HOLD_INTERRUPTS();
AbortStrongLockAcquire(); AbortStrongLockAcquire();
/* Nothing to do if we weren't waiting for a lock */ /* Nothing to do if we weren't waiting for a lock */
if (lockAwaited == NULL) if (lockAwaited == NULL)
{
RESUME_INTERRUPTS();
return; return;
}
/* /*
* Turn off the deadlock and lock timeout timers, if they are still * Turn off the deadlock and lock timeout timers, if they are still
...@@ -709,6 +714,8 @@ LockErrorCleanup(void) ...@@ -709,6 +714,8 @@ LockErrorCleanup(void)
* wakeup signal isn't harmful, and it seems not worth expending cycles to * wakeup signal isn't harmful, and it seems not worth expending cycles to
* get rid of a signal that most likely isn't there. * get rid of a signal that most likely isn't there.
*/ */
RESUME_INTERRUPTS();
} }
......
...@@ -75,7 +75,7 @@ static int16 parse_fcall_arguments_20(StringInfo msgBuf, struct fp_info * fip, ...@@ -75,7 +75,7 @@ static int16 parse_fcall_arguments_20(StringInfo msgBuf, struct fp_info * fip,
* The caller should already have initialized buf to empty. * The caller should already have initialized buf to empty.
* ---------------- * ----------------
*/ */
static int int
GetOldFunctionMessage(StringInfo buf) GetOldFunctionMessage(StringInfo buf)
{ {
int32 ibuf; int32 ibuf;
...@@ -280,33 +280,6 @@ HandleFunctionRequest(StringInfo msgBuf) ...@@ -280,33 +280,6 @@ HandleFunctionRequest(StringInfo msgBuf)
bool was_logged = false; bool was_logged = false;
char msec_str[32]; char msec_str[32];
/*
* Read message contents if not already done.
*/
if (PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
{
if (GetOldFunctionMessage(msgBuf))
{
if (IsTransactionState())
ereport(COMMERROR,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("unexpected EOF on client connection with an open transaction")));
else
{
/*
* Can't send DEBUG log messages to client at this point.
* Since we're disconnecting right away, we don't need to
* restore whereToSendOutput.
*/
whereToSendOutput = DestNone;
ereport(DEBUG1,
(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
errmsg("unexpected EOF on client connection")));
}
return EOF;
}
}
/* /*
* Now that we've eaten the input message, check to see if we actually * Now that we've eaten the input message, check to see if we actually
* want to do the function call or not. It's now safe to ereport(); we * want to do the function call or not. It's now safe to ereport(); we
......
This diff is collapsed.
...@@ -469,6 +469,7 @@ errfinish(int dummy,...) ...@@ -469,6 +469,7 @@ errfinish(int dummy,...)
* while doing error cleanup. * while doing error cleanup.
*/ */
InterruptHoldoffCount = 0; InterruptHoldoffCount = 0;
QueryCancelHoldoffCount = 0;
CritSectionCount = 0; /* should be unnecessary, but... */ CritSectionCount = 0; /* should be unnecessary, but... */
......
...@@ -32,6 +32,7 @@ volatile bool ProcDiePending = false; ...@@ -32,6 +32,7 @@ volatile bool ProcDiePending = false;
volatile bool ClientConnectionLost = false; volatile bool ClientConnectionLost = false;
volatile bool ImmediateInterruptOK = false; volatile bool ImmediateInterruptOK = false;
volatile uint32 InterruptHoldoffCount = 0; volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 QueryCancelHoldoffCount = 0;
volatile uint32 CritSectionCount = 0; volatile uint32 CritSectionCount = 0;
int MyProcPid; int MyProcPid;
......
...@@ -78,6 +78,9 @@ extern void TouchSocketFiles(void); ...@@ -78,6 +78,9 @@ extern void TouchSocketFiles(void);
extern void pq_init(void); extern void pq_init(void);
extern int pq_getbytes(char *s, size_t len); extern int pq_getbytes(char *s, size_t len);
extern int pq_getstring(StringInfo s); extern int pq_getstring(StringInfo s);
extern void pq_startmsgread(void);
extern void pq_endmsgread(void);
extern bool pq_is_reading_msg(void);
extern int pq_getmessage(StringInfo s, int maxlen); extern int pq_getmessage(StringInfo s, int maxlen);
extern int pq_getbyte(void); extern int pq_getbyte(void);
extern int pq_peekbyte(void); extern int pq_peekbyte(void);
......
...@@ -52,6 +52,10 @@ ...@@ -52,6 +52,10 @@
* will be held off until CHECK_FOR_INTERRUPTS() is done outside any * will be held off until CHECK_FOR_INTERRUPTS() is done outside any
* HOLD_INTERRUPTS() ... RESUME_INTERRUPTS() section. * HOLD_INTERRUPTS() ... RESUME_INTERRUPTS() section.
* *
* There is also a mechanism to prevent query cancel interrupts, while still
* allowing die interrupts: HOLD_CANCEL_INTERRUPTS() and
* RESUME_CANCEL_INTERRUPTS().
*
* Special mechanisms are used to let an interrupt be accepted when we are * Special mechanisms are used to let an interrupt be accepted when we are
* waiting for a lock or when we are waiting for command input (but, of * waiting for a lock or when we are waiting for command input (but, of
* course, only if the interrupt holdoff counter is zero). See the * course, only if the interrupt holdoff counter is zero). See the
...@@ -82,6 +86,7 @@ extern volatile bool ClientConnectionLost; ...@@ -82,6 +86,7 @@ extern volatile bool ClientConnectionLost;
/* these are marked volatile because they are examined by signal handlers: */ /* these are marked volatile because they are examined by signal handlers: */
extern PGDLLIMPORT volatile bool ImmediateInterruptOK; extern PGDLLIMPORT volatile bool ImmediateInterruptOK;
extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount;
extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount;
extern PGDLLIMPORT volatile uint32 CritSectionCount; extern PGDLLIMPORT volatile uint32 CritSectionCount;
/* in tcop/postgres.c */ /* in tcop/postgres.c */
...@@ -114,6 +119,14 @@ do { \ ...@@ -114,6 +119,14 @@ do { \
InterruptHoldoffCount--; \ InterruptHoldoffCount--; \
} while(0) } while(0)
#define HOLD_CANCEL_INTERRUPTS() (QueryCancelHoldoffCount++)
#define RESUME_CANCEL_INTERRUPTS() \
do { \
Assert(QueryCancelHoldoffCount > 0); \
QueryCancelHoldoffCount--; \
} while(0)
#define START_CRIT_SECTION() (CritSectionCount++) #define START_CRIT_SECTION() (CritSectionCount++)
#define END_CRIT_SECTION() \ #define END_CRIT_SECTION() \
......
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
#include "lib/stringinfo.h" #include "lib/stringinfo.h"
extern int GetOldFunctionMessage(StringInfo buf);
extern int HandleFunctionRequest(StringInfo msgBuf); extern int HandleFunctionRequest(StringInfo msgBuf);
#endif /* FASTPATH_H */ #endif /* FASTPATH_H */
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment