From: Jonathan Y. <jy...@bl...> - 2006-01-30 23:47:54
|
These patches apply to the CVS version of l2tpns, but may also apply to the 2.1.15 tarball. Note that the cluster peer changes are UNTESTED since I do not use l2tpns in a cluster environment (yet). Some of these changes were based on the evaluation of the output of Flawfinder, but not all. I consider none of these to be critical flaws BTW. This patch addresses the use of the cluster interface name when sending an GARP packet to the cluster. In my setup, eth1 is slated to be the cluster network. -------------------------------------------------------------------------------- --- l2tpns.c.orig 2006-01-19 15:55:03.000000000 -0500 +++ l2tpns.c 2006-01-30 16:06:12.000000000 -0500 @@ -900,7 +900,7 @@ return; } memset(&ifr, 0, sizeof(ifr)); - strncpy(ifr.ifr_name, "eth0", sizeof(ifr.ifr_name) - 1); + strncpy(ifr.ifr_name, config->cluster_interface, sizeof(ifr.ifr_name) - 1); if (ioctl(s, SIOCGIFHWADDR, &ifr) < 0) { LOG(0, 0, 0, "Error getting eth0 hardware address for GARP: %s\n", strerror(errno)); @@ -910,7 +910,7 @@ memcpy(mac, &ifr.ifr_hwaddr.sa_data, 6*sizeof(char)); if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) { - LOG(0, 0, 0, "Error getting eth0 interface index for GARP: %s\n", strerror(errno)); + LOG(0, 0, 0, "Error getting %s interface index for GARP: %s\n", config->cluster_interface, strerror(errno)); close(s); return; } -------------------------------------------------------------------------------- This patch addresses two areas in cli.c that *could* overflow data to a statically-allocated buffers. *TESTED* -------------------------------------------------------------------------------- --- cli.c.orig 2005-12-06 04:43:42.000000000 -0500 +++ cli.c 2006-01-30 16:27:47.000000000 -0500 @@ -586,7 +586,7 @@ for (x = 0; x < MAXSESSION; x++) if (session[x].tunnel == t && session[x].opened && !session[x].die) - sprintf(s, "%s%u ", s, x); + snprintf(s, sizeof(s), "%s%u ", s, x); cli_print(cli, "\tSessions:\t%s", s); } @@ -2379,7 +2379,7 @@ if (!*ip_filters[filt].name) { memset(&ip_filters[filt], 0, sizeof(ip_filters[filt])); - strcpy(ip_filters[filt].name, argv[1]); + strncpy(ip_filters[filt].name, argv[1], sizeof(ip_filters[filt].name)-1); ip_filters[filt].extended = extended; } else if (ip_filters[filt].extended != extended) -------------------------------------------------------------------------------- This patch enforces boundaries on the radius user and password to ensure they do not overflow their buffers. *TESTED* -------------------------------------------------------------------------------- --- radius.c.orig 2005-12-19 01:18:13.000000000 -0500 +++ radius.c 2006-01-30 17:32:10.000000000 -0500 @@ -195,8 +195,8 @@ if (s) { *p = 1; // user name - p[1] = strlen(session[s].user) + 2; - strcpy((char *) p + 2, session[s].user); + p[1] = (strlen(session[s].user) < sizeof(session[s].user) ? strlen(session[s].user) : sizeof(session[s].user)-1) + 2; + strncpy((char *) p + 2, session[s].user, sizeof(session[s].user)-1); p += p[1]; } if (state == RADIUSAUTH) @@ -215,7 +215,8 @@ } else { - strcpy(pass, radius[r].pass); + memset(pass,0,sizeof(pass)); + strncpy(pass, radius[r].pass, sizeof(radius[r].pass)-1); pl = strlen(pass); while (pl & 15) pass[pl++] = 0; // pad @@ -363,15 +364,15 @@ if (*session[s].called) { *p = 30; // called - p[1] = strlen(session[s].called) + 2; - strcpy((char *) p + 2, session[s].called); + p[1] = (strlen(session[s].called) < sizeof(session[s].called) ? strlen(session[s].called) : sizeof(session[s].called)-1) + 2; + strncpy((char *) p + 2, session[s].called, sizeof(session[s].called)-1); p += p[1]; } if (*session[s].calling) { *p = 31; // calling - p[1] = strlen(session[s].calling) + 2; - strcpy((char *) p + 2, session[s].calling); + p[1] = (strlen(session[s].calling) < sizeof(session[s].calling) ? strlen(session[s].calling) : sizeof(session[s].calling)-1) + 2; + strncpy((char *) p + 2, session[s].calling, sizeof(session[s].calling)-1); p += p[1]; } // NAS-IP-Address -------------------------------------------------------------------------------- This patch enforces a buffer size on the cluster interface name and replaces some of the large automatic variable usage in the cluster peer code with dynamic allocation. *UNTESTED* -------------------------------------------------------------------------------- --- cluster.c.orig 2005-12-05 09:10:42.000000000 -0500 +++ cluster.c 2006-01-30 18:31:23.000000000 -0500 @@ -109,7 +109,7 @@ return -1; } - strcpy(ifr.ifr_name, config->cluster_interface); + strncpy(ifr.ifr_name, config->cluster_interface, sizeof(ifr.ifr_name)-1); if (ioctl(cluster_sockfd, SIOCGIFADDR, &ifr) < 0) { LOG(0, 0, 0, "Failed to get interface address for (%s): %s\n", config->cluster_interface, strerror(errno)); @@ -270,8 +270,17 @@ // static int peer_send_message(in_addr_t peer, int type, int more, uint8_t *data, int size) { - uint8_t buf[65536]; // Vast overkill. - uint8_t *p = buf; + static uint8_t *buf=(uint8_t) NULL; + static int bufsize=0; + int sz; + uint8_t *p; + + if (!buf || bufsize < size) { // create or grow our buffer + sz = (size ? size : 1024); // specify minumum size if none + buf = (uint8_t *) realloc (buf,sz); + bufsize=sz; + } + p=buf; LOG(4, 0, 0, "Sending message to peer (type %d, more %d, size %d)\n", type, more, size); add_type(&p, type, more, data, size); @@ -282,12 +291,21 @@ // send a packet to the master static int _forward_packet(uint8_t *data, int size, in_addr_t addr, int port, int type) { - uint8_t buf[65536]; // Vast overkill. - uint8_t *p = buf; + static uint8_t *buf=(uint8_t) NULL; + static int bufsize=0; + int sz; + uint8_t *p; if (!config->cluster_master_address) // No election has been held yet. Just skip it. return -1; + if (!buf || bufsize < size) { // create or grow our buffer + sz = (size ? size : 1024); // specify minumum size if none + buf = (uint8_t *) realloc (buf,sz); + bufsize=sz; + } + p=buf; + LOG(4, 0, 0, "Forwarding packet from %s to master (size %d)\n", fmtaddr(addr, 0), size); STAT(c_forwarded); @@ -324,12 +342,21 @@ // int master_throttle_packet(int tbfid, uint8_t *data, int size) { - uint8_t buf[65536]; // Vast overkill. - uint8_t *p = buf; + static uint8_t *buf=(uint8_t *) NULL; + static int bufsize=0; + int sz; + uint8_t *p; if (!config->cluster_master_address) // No election has been held yet. Just skip it. return -1; + if (!buf || bufsize < size) { // create or grow our buffer + sz = (size ? size : 1024); // specify minumum size if none + buf = (uint8_t *) realloc (buf,sz); + bufsize=sz; + } + p=buf; + LOG(4, 0, 0, "Throttling packet master (size %d, tbfid %d)\n", size, tbfid); add_type(&p, C_THROTTLE, tbfid, data, size); @@ -349,12 +376,21 @@ // as the start of the data). int master_garden_packet(sessionidt s, uint8_t *data, int size) { - uint8_t buf[65536]; // Vast overkill. - uint8_t *p = buf; + static uint8_t *buf=(uint8_t *) NULL; + static int bufsize=0; + int sz; + uint8_t *p; if (!config->cluster_master_address) // No election has been held yet. Just skip it. return -1; + if (!buf || bufsize < size) { // create or grow our buffer + sz = (size ? size : 1024); // specify minumum size if none + buf = (uint8_t *) realloc (buf,sz); + bufsize=sz; + } + p=buf; + LOG(4, 0, 0, "Walled garden packet to master (size %d)\n", size); add_type(&p, C_GARDEN, s, data, size); -------------------------------------------------------------------------------- -- Those who make peaceful revolution impossible will make violent revolution inevitable. -- John F. Kennedy |