More magic cleanup, found even couple potential bugs.

- get_header() allowed < 5 length, which could trigger unsigned overflow.
- error from server when setting parameters did not do "return false"
- broken ParameterStatus should close connection.

Again, thanks to David Fetter who pushed for cleaner code.

diff --git a/src/admin.c b/src/admin.c
index 9c2134953f1552c12df4f5c63f78904025b499fe..2b41b53c6e698e991f74f59013d5ca6d342e259d 100644 (file)
--- a/src/admin.c
+++ b/src/admin.c
@@ -867,7 +867,7 @@ bool admin_handle_client(PgSocket *admin, MBuf *pkt, int pkt_type, int pkt_len)
        bool res;
 
        /* dont tolerate partial packets */
-       if (mbuf_avail(pkt) < pkt_len - 5) {
+       if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN) {
                disconnect_client(admin, true, "incomplete pkt");
                return false;
        }

diff --git a/src/bouncer.h b/src/bouncer.h
index 82e23970d31d722408188b9019a7acb718efc87d..c85ffc05ca25933be6c6c4bc73820e71d1c2c416 100644 (file)
--- a/src/bouncer.h
+++ b/src/bouncer.h
@@ -98,15 +98,17 @@ typedef enum SocketState SocketState;
 
 /* type codes for weird pkts */
 #define PKT_STARTUP     0x30000
-#define PKT_SSLREQ      80877103
 #define PKT_CANCEL      80877102
+#define PKT_SSLREQ      80877103
 
 #define POOL_SESSION   0
 #define POOL_TX                1
 #define POOL_STMT      2
 
+/* old style V2 header: len:4b code:4b */
+#define OLD_HEADER_LEN 8
 /* new style V3 packet header len - type:1b, len:4b */ 
-#define PQ_HEADER_LEN  5
+#define NEW_HEADER_LEN 5
 
 struct PgAddr {
        struct in_addr ip_addr;

diff --git a/src/client.c b/src/client.c
index 5d3707abff290f6e2d847efa45fb334403395751..57ff1cec900ecb1603dd97e9b6b46c40ea379d0f 100644 (file)
--- a/src/client.c
+++ b/src/client.c
@@ -238,7 +238,7 @@ static bool handle_client_startup(PgSocket *client, MBuf *pkt)
                }
                break;
        case 'p':               /* PasswordMessage */
-               if (mbuf_avail(pkt) < pkt_len - PQ_HEADER_LEN) {
+               if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN) {
                        disconnect_client(client, true, "client sent partial pkt in startup");
                        return false;
                }
@@ -374,7 +374,7 @@ bool client_proto(SBuf *sbuf, SBufEvent evtype, MBuf *pkt, void *arg)
                disconnect_server(client->link, false, "Server connection closed");
                break;
        case SBUF_EV_READ:
-               if (mbuf_avail(pkt) < 5) {
+               if (mbuf_avail(pkt) < NEW_HEADER_LEN) {
                        slog_noise(client, "C: got partial header, trying to wait a bit");
                        return false;
                }

diff --git a/src/proto.c b/src/proto.c
index 730458ef959ae3444ad1a68cf698f93d287fa6ff..d47f3c73a006add44f7995b87a1146acfe6b2ccd 100644 (file)
--- a/src/proto.c
+++ b/src/proto.c
@@ -33,7 +33,7 @@ bool get_header(MBuf *pkt, unsigned *pkt_type_p, unsigned *pkt_len_p)
        unsigned len;
        unsigned code;
 
-       if (mbuf_avail(pkt) < 5) {
+       if (mbuf_avail(pkt) < NEW_HEADER_LEN) {
                log_noise("get_header: less then 5 bytes available");
                return false;
        }
@@ -47,15 +47,15 @@ bool get_header(MBuf *pkt, unsigned *pkt_type_p, unsigned *pkt_len_p)
                        return false;
                }
                /* dont tolerate partial pkt */
-               if (mbuf_avail(pkt) < 6) {
-                       log_noise("get_header: less that 6 bytes for special pkt");
+               if (mbuf_avail(pkt) < OLD_HEADER_LEN - 2) {
+                       log_noise("get_header: less than 8 bytes for special pkt");
                        return false;
                }
                len = mbuf_get_uint16(pkt);
                code = mbuf_get_uint32(pkt);
-               if (code == 80877102)
+               if (code == PKT_CANCEL)
                        type = PKT_CANCEL;
-               else if (code == 80877103)
+               else if (code == PKT_SSLREQ)
                        type = PKT_SSLREQ;
                else if ((code >> 16) == 3 && (code & 0xFFFF) < 2)
                        type = PKT_STARTUP;
@@ -64,6 +64,11 @@ bool get_header(MBuf *pkt, unsigned *pkt_type_p, unsigned *pkt_len_p)
                        return false;
                }
        }
+
+       /* don't believe nonsense */
+       if (len < NEW_HEADER_LEN || len >= 0x80000000)
+               return false;
+
        *pkt_type_p = type;
        *pkt_len_p = len;
        return true;
@@ -132,7 +137,7 @@ bool add_welcome_parameter(PgSocket *server,
                return true;
 
        /* incomplete startup msg from server? */
-       if (pkt_len - 5 > mbuf_avail(pkt))
+       if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN)
                return false;
 
        pktbuf_static(&msg, pool->welcome_msg + pool->welcome_msg_len,
@@ -254,13 +259,17 @@ bool answer_authreq(PgSocket *server,
        unsigned cmd;
        const uint8 *salt;
        bool res = false;
+       unsigned pkt_remain;
 
-       if (pkt_len < 5 + 4)
+       /* authreq body must contain 32bit cmd */
+       if (pkt_len < NEW_HEADER_LEN + 4)
                return false;
-       if (mbuf_avail(pkt) < pkt_len - 5)
+       /* is packet fully received? */
+       if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN)
                return false;
 
        cmd = mbuf_get_uint32(pkt);
+       pkt_remain = pkt_len - NEW_HEADER_LEN - 4;
        switch (cmd) {
        case 0:
                slog_debug(server, "S: auth ok");
@@ -271,14 +280,14 @@ bool answer_authreq(PgSocket *server,
                res = login_clear_psw(server);
                break;
        case 4:
-               if (pkt_len < 5 + 4 + 2)
+               if (pkt_remain < 2)
                        return false;
                slog_debug(server, "S: req crypt psw");
                salt = mbuf_get_bytes(pkt, 2);
                res = login_crypt_psw(server, salt);
                break;
        case 5:
-               if (pkt_len < 5 + 4 + 4)
+               if (pkt_remain < 4)
                        return false;
                slog_debug(server, "S: req md5-crypted psw");
                salt = mbuf_get_bytes(pkt, 4);

diff --git a/src/server.c b/src/server.c
index 24b13d288ed61a210311d713b74829f2623863de..e6ad062bbac920bd9d6b668c5f2d4ef587bfe8a4 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -22,20 +22,23 @@
 
 #include "bouncer.h"
 
-static void check_parameters(PgSocket *server, MBuf *pkt, unsigned pkt_len)
+static bool load_parameter(PgSocket *server, MBuf *pkt, unsigned pkt_len)
 {
        const char *key, *val;
        PgSocket *client = server->link;
 
-       /* incomplete startup msg from server? */
-       if (pkt_len - 5 > mbuf_avail(pkt))
-               return;
+       /*
+        * incomplete startup msg from server?
+        * (hdr is already parsed here)
+        */
+       if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN)
+               return false;
 
        key = mbuf_get_string(pkt);
        val = mbuf_get_string(pkt);
        if (!key || !val) {
-               slog_error(server, "broken ParameterStatus packet");
-               return;
+               disconnect_server(server, true, "broken ParameterStatus packet");
+               return false;
        }
        slog_debug(server, "S: param: %s = %s", key, val);
 
@@ -46,7 +49,7 @@ static void check_parameters(PgSocket *server, MBuf *pkt, unsigned pkt_len)
                varcache_set(&client->vars, key, val);
        }
 
-       return;
+       return true;
 }
 
 /* process packets on server auth phase */
@@ -62,7 +65,7 @@ static bool handle_server_startup(PgSocket *server, MBuf *pkt)
                return false;
        }
 
-       if (pkt_len > mbuf_avail(pkt) + 5) {
+       if (mbuf_avail(pkt) < pkt_len - NEW_HEADER_LEN) {
                disconnect_server(server, true, "partial pkt in login phase");
                return false;
        }
@@ -169,7 +172,8 @@ static bool handle_server_work(PgSocket *server, MBuf *pkt)
                break;
 
        case 'S':               /* ParameterStatus */
-               check_parameters(server, pkt, pkt_len);
+               if (!load_parameter(server, pkt, pkt_len))
+                       return false;
                break;
 
        /*
@@ -196,6 +200,7 @@ static bool handle_server_work(PgSocket *server, MBuf *pkt)
                         * no reason to keep such guys.
                         */
                        disconnect_client(server->link, true, "invalid server parameter");
+                       return false;
                }
        case 'N':               /* NoticeResponse */
                break;
@@ -299,7 +304,7 @@ bool server_proto(SBuf *sbuf, SBufEvent evtype, MBuf *pkt, void *arg)
                disconnect_client(server->link, false, "unexpected eof");
                break;
        case SBUF_EV_READ:
-               if (mbuf_avail(pkt) < 5) {
+               if (mbuf_avail(pkt) < NEW_HEADER_LEN) {
                        slog_noise(server, "S: got partial header, trying to wait a bit");
                        return false;
                }

diff --git a/src/takeover.c b/src/takeover.c
index 4599897c6801e591265defdc585ad0d06fd7f230..89221516ce71804715774631090da4adeaad02aa 100644 (file)
--- a/src/takeover.c
+++ b/src/takeover.c
@@ -192,8 +192,9 @@ static void takeover_parse_data(PgSocket *bouncer,
                if (!get_header(data, &pkt_type, &pkt_len))
                        fatal("cannot parse packet");
 
-               pktptr = (uint8*)mbuf_get_bytes(data, pkt_len - 5);
-               mbuf_init(&pkt, pktptr, pkt_len - 5);
+               /* crash on overflow is ok here */
+               pktptr = (uint8*)mbuf_get_bytes(data, pkt_len - NEW_HEADER_LEN);
+               mbuf_init(&pkt, pktptr, pkt_len - NEW_HEADER_LEN);
 
                switch (pkt_type) {
                case 'T': /* RowDescription */