From 74ed8f9e19f3d96b6ce090a4840a3be705bc255d Mon Sep 17 00:00:00 2001 From: bodea Date: Sat, 2 Dec 2006 14:09:14 +0000 Subject: [PATCH] Security: Rhys Kidd identified a vulnerability in the handling of heartbeat packets. Drop oversize heartbeat packets. --- Changes | 4 ++ THANKS | 2 +- cluster.c | 166 ++++++---------------------------------------------- l2tpns.h | 4 +- l2tpns.spec | 6 +- 5 files changed, 28 insertions(+), 154 deletions(-) diff --git a/Changes b/Changes index ee938a7..abe7854 100644 --- a/Changes +++ b/Changes @@ -1,3 +1,7 @@ +* Fri Dec 1 2006 Brendan O'Dea 2.1.21 +- Security: Rhys Kidd identified a vulnerability in the handling of + heartbeat packets. Drop oversize heartbeat packets. + * Thu Aug 3 2006 Brendan O'Dea 2.1.20 - Fix sign problem with reporting of unknown VSAs. - Allow DNS servers to be specified either using the old or new diff --git a/THANKS b/THANKS index 046a513..c39a07d 100644 --- a/THANKS +++ b/THANKS @@ -26,4 +26,4 @@ Jon Morby Paul Martin Jonathan Yarden Patrick Cole -Khaled Al Hamwi +Rhys Kidd diff --git a/cluster.c b/cluster.c index abe2a63..538da20 100644 --- a/cluster.c +++ b/cluster.c @@ -1,6 +1,6 @@ // L2TPNS Clustering Stuff -char const *cvs_id_cluster = "$Id: cluster.c,v 1.51 2006/04/27 09:53:49 bodea Exp $"; +char const *cvs_id_cluster = "$Id: cluster.c,v 1.50.2.1 2006/12/02 14:09:14 bodea Exp $"; #include #include @@ -42,7 +42,6 @@ extern int cluster_sockfd; // The filedescriptor for the cluster communications in_addr_t my_address = 0; // The network address of my ethernet port. static int walk_session_number = 0; // The next session to send when doing the slow table walk. -static int walk_bundle_number = 0; // The next bundle to send when doing the slow table walk. static int walk_tunnel_number = 0; // The next tunnel to send when doing the slow table walk. int forked = 0; // Sanity check: CLI must not diddle with heartbeat table @@ -86,7 +85,6 @@ int cluster_init() int opt; config->cluster_undefined_sessions = MAXSESSION-1; - config->cluster_undefined_bundles = MAXBUNDLE-1; config->cluster_undefined_tunnels = MAXTUNNEL-1; if (!config->cluster_address) @@ -229,7 +227,7 @@ static void cluster_uptodate(void) if (config->cluster_iam_uptodate) return; - if (config->cluster_undefined_sessions || config->cluster_undefined_tunnels || config->cluster_undefined_bundles) + if (config->cluster_undefined_sessions || config->cluster_undefined_tunnels) return; config->cluster_iam_uptodate = 1; @@ -522,7 +520,7 @@ void cluster_check_slaves(void) // void cluster_check_master(void) { - int i, count, tcount, bcount, high_unique_id = 0; + int i, count, tcount, high_unique_id = 0; int last_free = 0; clockt t = TIME; static int probed = 0; @@ -616,19 +614,6 @@ void cluster_check_master(void) config->cluster_highest_tunnelid = i; } - // - // Go through and mark all the bundles as defined. - // Count the highest used bundle number as well. - // - config->cluster_highest_bundleid = 0; - for (i = 0, bcount = 0; i < MAXBUNDLE; ++i) { - if (bundle[i].state == BUNDLEUNDEF) - bundle[i].state = BUNDLEFREE; - - if (bundle[i].state != BUNDLEFREE && i > config->cluster_highest_bundleid) - config->cluster_highest_bundleid = i; - } - // // Go through and mark all the sessions as being defined. // reset the idle timeouts. @@ -690,11 +675,10 @@ void cluster_check_master(void) rebuild_address_pool(); // If we're not the very first master, this is a big issue! - if (count > 0) + if(count>0) LOG(0, 0, 0, "Warning: Fixed %d uninitialized sessions in becoming master!\n", count); config->cluster_undefined_sessions = 0; - config->cluster_undefined_bundles = 0; config->cluster_undefined_tunnels = 0; config->cluster_iam_uptodate = 1; // assume all peers are up-to-date @@ -712,7 +696,7 @@ void cluster_check_master(void) // we fix it up here, and we ensure that the 'first free session' // pointer is valid. // -static void cluster_check_sessions(int highsession, int freesession_ptr, int highbundle, int hightunnel) +static void cluster_check_sessions(int highsession, int freesession_ptr, int hightunnel) { int i; @@ -721,7 +705,7 @@ static void cluster_check_sessions(int highsession, int freesession_ptr, int hig if (config->cluster_iam_uptodate) return; - if (highsession > config->cluster_undefined_sessions && highbundle > config->cluster_undefined_bundles && hightunnel > config->cluster_undefined_tunnels) + if (highsession > config->cluster_undefined_sessions && hightunnel > config->cluster_undefined_tunnels) return; // Clear out defined sessions, counting the number of @@ -737,19 +721,6 @@ static void cluster_check_sessions(int highsession, int freesession_ptr, int hig ++config->cluster_undefined_sessions; } - // Clear out defined bundles, counting the number of - // undefs remaining. - config->cluster_undefined_bundles = 0; - for (i = 1 ; i < MAXBUNDLE; ++i) { - if (i > highbundle) { - if (bundle[i].state == BUNDLEUNDEF) bundle[i].state = BUNDLEFREE; // Defined. - continue; - } - - if (bundle[i].state == BUNDLEUNDEF) - ++config->cluster_undefined_bundles; - } - // Clear out defined tunnels, counting the number of // undefs remaining. config->cluster_undefined_tunnels = 0; @@ -764,9 +735,9 @@ static void cluster_check_sessions(int highsession, int freesession_ptr, int hig } - if (config->cluster_undefined_sessions || config->cluster_undefined_tunnels || config->cluster_undefined_bundles) { - LOG(2, 0, 0, "Cleared undefined sessions/bundles/tunnels. %d sess (high %d), %d bund (high %d), %d tunn (high %d)\n", - config->cluster_undefined_sessions, highsession, config->cluster_undefined_bundles, highbundle, config->cluster_undefined_tunnels, hightunnel); + if (config->cluster_undefined_sessions || config->cluster_undefined_tunnels) { + LOG(2, 0, 0, "Cleared undefined sessions/tunnels. %d sess (high %d), %d tunn (high %d)\n", + config->cluster_undefined_sessions, highsession, config->cluster_undefined_tunnels, hightunnel); return; } @@ -799,27 +770,6 @@ static int hb_add_type(uint8_t **p, int type, int id) add_type(p, C_SESSION, id, (uint8_t *) &session[id], sizeof(sessiont)); break; - case C_CBUNDLE: { // Compressed C_BUNDLE - uint8_t c[sizeof(bundlet) * 2]; // Bigger than worst case. - uint8_t *d = (uint8_t *) &bundle[id]; - uint8_t *orig = d; - int size; - - size = rle_compress( &d, sizeof(bundlet), c, sizeof(c) ); - - // Did we compress the full structure, and is the size actually - // reduced?? - if ( (d - orig) == sizeof(bundlet) && size < sizeof(bundlet) ) { - add_type(p, C_CBUNDLE, id, c, size); - break; - } - // Failed to compress : Fall through. - } - - case C_BUNDLE: - add_type(p, C_BUNDLE, id, (uint8_t *) &bundle[id], sizeof(bundlet)); - break; - case C_CTUNNEL: { // Compressed C_TUNNEL uint8_t c[sizeof(tunnelt) * 2]; // Bigger than worst case. uint8_t *d = (uint8_t *) &tunnel[id]; @@ -852,7 +802,7 @@ static int hb_add_type(uint8_t **p, int type, int id) // void cluster_heartbeat() { - int i, count = 0, tcount = 0, bcount = 0; + int i, count = 0, tcount = 0; uint8_t buff[MAX_HEART_SIZE + sizeof(heartt) + sizeof(int) ]; heartt h; uint8_t *p = buff; @@ -873,9 +823,7 @@ void cluster_heartbeat() h.highsession = config->cluster_highest_sessionid; h.freesession = sessionfree; h.hightunnel = config->cluster_highest_tunnelid; - h.highbundle = config->cluster_highest_bundleid; h.size_sess = sizeof(sessiont); // Just in case. - h.size_bund = sizeof(bundlet); h.size_tunn = sizeof(tunnelt); h.interval = config->cluster_hb_interval; h.timeout = config->cluster_hb_timeout; @@ -930,22 +878,6 @@ void cluster_heartbeat() ++tcount; } - // - // Fill out the packet with bundles from the bundle table... - while ( (p + sizeof(uint32_t) * 2 + sizeof(bundlet) ) < (buff + MAX_HEART_SIZE) ) { - - if (!walk_bundle_number) // bundle #0 isn't valid. - ++walk_bundle_number; - - if (bcount >= config->cluster_highest_bundleid) - break; - - hb_add_type(&p, C_CTUNNEL, walk_bundle_number); - walk_tunnel_number = (1+walk_bundle_number)%(config->cluster_highest_bundleid+1); // +1 avoids divide by zero. - - ++bcount; - } - // // Did we do something wrong? if (p > (buff + sizeof(buff))) { // Did we somehow manage to overun the buffer? @@ -955,9 +887,9 @@ void cluster_heartbeat() } LOG(3, 0, 0, "Sending v%d heartbeat #%d, change #%" PRIu64 " with %d changes " - "(%d x-sess, %d x-bundles, %d x-tunnels, %d highsess, %d highbund, %d hightun, size %d)\n", + "(%d x-sess, %d x-tunnels, %d highsess, %d hightun, size %d)\n", HB_VERSION, h.seq, h.table_version, config->cluster_num_changes, - count, bcount, tcount, config->cluster_highest_sessionid, config->cluster_highest_bundleid, + count, tcount, config->cluster_highest_sessionid, config->cluster_highest_tunnelid, (int) (p - buff)); config->cluster_num_changes = 0; @@ -1006,17 +938,6 @@ int cluster_send_session(int sid) return type_changed(C_CSESSION, sid); } -// A particular bundle has been changed! -int cluster_send_bundle(int bid) -{ - if (!config->cluster_iam_master) { - LOG(0, 0, bid, "I'm not a master, but I just tried to change a bundle!\n"); - return -1; - } - - return type_changed(C_CBUNDLE, bid); -} - // A particular tunnel has been changed! int cluster_send_tunnel(int tid) { @@ -1254,31 +1175,6 @@ static int cluster_recv_session(int more, uint8_t *p) return 0; } -static int cluster_recv_bundle(int more, uint8_t *p) -{ - if (more >= MAXBUNDLE) { - LOG(0, 0, 0, "DANGER: Received a bundle id > MAXBUNDLE!\n"); - return -1; - } - - if (bundle[more].state == BUNDLEUNDEF) { - if (config->cluster_iam_uptodate) { // Sanity. - LOG(0, 0, 0, "I thought I was uptodate but I just found an undefined bundle!\n"); - } else { - --config->cluster_undefined_bundles; - } - } - - memcpy(&bundle[more], p, sizeof(bundle[more]) ); - - LOG(5, 0, more, "Received bundle update\n"); - - if (!config->cluster_iam_uptodate) - cluster_uptodate(); // Check to see if we're up to date. - - return 0; -} - static int cluster_recv_tunnel(int more, uint8_t *p) { if (more >= MAXTUNNEL) { @@ -1451,7 +1347,11 @@ static int cluster_process_heartbeat(uint8_t *data, int size, int more, uint8_t return -1; // Ignore it?? } - // Ok. It's a heartbeat packet from a cluster master! + if (size > sizeof(past_hearts[0].data)) { + LOG(0, 0, 0, "Received an oversize heartbeat from %s (%d)!\n", fmtaddr(addr, 0), size); + return -1; + } + if (s < sizeof(*h)) goto shortpacket; @@ -1565,7 +1465,7 @@ static int cluster_process_heartbeat(uint8_t *data, int size, int more, uint8_t // Check that we don't have too many undefined sessions, and // that the free session pointer is correct. - cluster_check_sessions(h->highsession, h->freesession, h->highbundle, h->hightunnel); + cluster_check_sessions(h->highsession, h->freesession, h->hightunnel); if (h->interval != config->cluster_hb_interval) { @@ -1673,34 +1573,6 @@ static int cluster_process_heartbeat(uint8_t *data, int size, int more, uint8_t p += sizeof(tunnel[more]); s -= sizeof(tunnel[more]); break; - - case C_CBUNDLE: { // Compressed bundle structure. - uint8_t c[ sizeof(bundlet) + 2]; - int size; - uint8_t *orig_p = p; - - size = rle_decompress((uint8_t **) &p, s, c, sizeof(c)); - s -= (p - orig_p); - - if (size != sizeof(bundlet) ) { // Ouch! Very very bad! - LOG(0, 0, 0, "DANGER: Received a CBUNDLE that didn't decompress correctly!\n"); - // Now what? Should exit! No-longer up to date! - break; - } - - cluster_recv_bundle(more, c); - break; - - } - case C_BUNDLE: - if ( s < sizeof(bundle[more])) - goto shortpacket; - - cluster_recv_bundle(more, p); - - p += sizeof(bundle[more]); - s -= sizeof(bundle[more]); - break; default: LOG(0, 0, 0, "DANGER: I received a heartbeat element where I didn't understand the type! (%d)\n", type); return -1; // can't process any more of the packet!! @@ -1886,13 +1758,11 @@ int cmd_show_cluster(struct cli_def *cli, char *command, char **argv, int argc) cli_print(cli, "Table version # : %" PRIu64, config->cluster_table_version); cli_print(cli, "Next sequence number expected: %d", config->cluster_seq_number); cli_print(cli, "%d sessions undefined of %d", config->cluster_undefined_sessions, config->cluster_highest_sessionid); - cli_print(cli, "%d bundles undefined of %d", config->cluster_undefined_bundles, config->cluster_highest_bundleid); cli_print(cli, "%d tunnels undefined of %d", config->cluster_undefined_tunnels, config->cluster_highest_tunnelid); } else { cli_print(cli, "Table version # : %" PRIu64, config->cluster_table_version); cli_print(cli, "Next heartbeat # : %d", config->cluster_seq_number); cli_print(cli, "Highest session : %d", config->cluster_highest_sessionid); - cli_print(cli, "Highest bundle : %d", config->cluster_highest_bundleid); cli_print(cli, "Highest tunnel : %d", config->cluster_highest_tunnelid); cli_print(cli, "%d changes queued for sending", config->cluster_num_changes); } diff --git a/l2tpns.h b/l2tpns.h index 209b38b..6c38bbf 100644 --- a/l2tpns.h +++ b/l2tpns.h @@ -1,5 +1,5 @@ // L2TPNS Global Stuff -// $Id: l2tpns.h,v 1.113.2.2 2006/08/02 12:53:35 bodea Exp $ +// $Id: l2tpns.h,v 1.113.2.3 2006/12/02 14:09:14 bodea Exp $ #ifndef __L2TPNS_H__ #define __L2TPNS_H__ @@ -14,7 +14,7 @@ #include #include -#define VERSION "2.1.20" +#define VERSION "2.1.21" // Limits #define MAXTUNNEL 500 // could be up to 65535 diff --git a/l2tpns.spec b/l2tpns.spec index 3ed4c39..4cd9ca8 100644 --- a/l2tpns.spec +++ b/l2tpns.spec @@ -1,6 +1,6 @@ Summary: A high-speed clustered L2TP LNS Name: l2tpns -Version: 2.1.20 +Version: 2.1.21 Release: 1 License: GPL Group: System Environment/Daemons @@ -43,5 +43,5 @@ rm -rf %{buildroot} %attr(644,root,root) /usr/share/man/man[58]/* %changelog -* Thu Aug 3 2006 Brendan O'Dea 2.1.20-1 -- 2.1.20 release, see /usr/share/doc/l2tpns-2.1.20/Changes +* Fri Dec 1 2006 Brendan O'Dea 2.1.21-1 +- 2.1.21 release, see /usr/share/doc/l2tpns-2.1.21/Changes -- 2.20.1