From bc5e5e713b955a1a7e1edf66c33599e5a71985ed Mon Sep 17 00:00:00 2001 From: Marko Kreen Date: Mon, 13 Aug 2007 20:37:11 +0000 Subject: [PATCH] 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. --- src/admin.c | 2 +- src/bouncer.h | 6 ++++-- src/client.c | 4 ++-- src/proto.c | 29 +++++++++++++++++++---------- src/server.c | 25 +++++++++++++++---------- src/takeover.c | 5 +++-- 6 files changed, 44 insertions(+), 27 deletions(-) diff --git a/src/admin.c b/src/admin.c index 9c21349..2b41b53 100644 --- 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 82e2397..c85ffc0 100644 --- 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 5d3707a..57ff1ce 100644 --- 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 730458e..d47f3c7 100644 --- 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 24b13d2..e6ad062 100644 --- 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 4599897..8922151 100644 --- 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 */ -- 2.40.0