8000 Be more careful to not lose sync in the FE/BE protocol. · postgrespro/postgres@2b3a8b2 · GitHub
[go: up one dir, main page]
Skip to content

    • Commit 2b3a8b2

    Browse files
    hlinnaka
    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 59b9198 commit 2b3a8b2

    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
    int mtype;
    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
    StringInfoData buf;
    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 int PqRecvLength; /* End of data available in PqRecvBuffer */
    127127
    /*
    128128
    * Message status
    129129
    */
    130-
    static bool PqCommBusy;
    131-
    static bool DoingCopyOut;
    130+
    static bool PqCommBusy; /* busy sending data to the client */
    131+
    static bool PqCommReadingMsg; /* in the middle of reading a message */
    132+
    static bool DoingCopyOut; /* 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
    int r;
    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_t amount;
    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_t amount;
    10401048

    1049+
    Assert(PqCommReadingMsg);
    1050+
    10411051
    while (len > 0)
    10421052
    {
    10431053
    while (PqRecvPointer >= PqRecvLength)
    @@ -1074,6 +1084,8 @@ pq_getstring(StringInfo s)
    10741084
    {
    10751085
    int i;
    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+
    return PqCommReadingMsg;
    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
    int32 len;
    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
    return 0;
    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
    ProtocolVersion proto;
    17621762
    MemoryContext oldcontext;
    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
    return STATUS_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
    char msgtype;
    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
    DisableTimeoutParams timeouts[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-
    static int
    78+
    int
    7979
    GetOldFunctionMessage(StringInfo buf)
    8080
    {
    8181
    int32 ibuf;
    @@ -280,33 +280,6 @@ HandleFunctionRequest(StringInfo msgBuf)
    280280
    bool was_logged = false;
    281281
    char msec_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-
    return EOF;
    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)
    0