From 5540bab74ca1a39639a098d15e87daf45d83b7ec Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 19 Sep 2014 23:35:06 +0100 Subject: [PATCH] ipaddr_to_string: SECURITY: Do not allocate ipaddr_to_string is used in many places including runtime logging. Handling its memory allocation is annoyingly fiddly. Indeed there is at least one possible memory leak, which represents a potential denial of service bug. None of the callers keep the answers for any length of time. So make it return the next one of a series of round-robin buffers, instead, and remove all the freeing at all the call sites. Signed-off-by: Ian Jackson --- ipaddr.c | 16 +++++++++++++++- netlink.c | 7 ------- tun.c | 2 -- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/ipaddr.c b/ipaddr.c index f8cda00..6d0b02a 100644 --- a/ipaddr.c +++ b/ipaddr.c @@ -323,13 +323,27 @@ struct subnet_list *ipset_to_subnet_list(struct ipset *is) return r; } +#define IPADDR_NBUFS_SHIFT 4 +#define IPADDR_NBUFS (1 << IPADDR_NBUFS_SHIFT) +#define IPADDR_BUFLEN 20 + +static char *ipaddr_getbuf(void) +{ + static int ipaddr_bufnum; + static char ipaddr_bufs[IPADDR_NBUFS][IPADDR_BUFLEN]; + + ipaddr_bufnum++; + ipaddr_bufnum &= IPADDR_NBUFS-1; + return ipaddr_bufs[ipaddr_bufnum]; +} + /* The string buffer must be at least 16 bytes long */ string_t ipaddr_to_string(uint32_t addr) { uint8_t a,b,c,d; string_t s; - s=safe_malloc(16,"ipaddr_to_string"); + s=ipaddr_getbuf(); a=addr>>24; b=addr>>16; c=addr>>8; diff --git a/netlink.c b/netlink.c index ea1221d..b54327b 100644 --- a/netlink.c +++ b/netlink.c @@ -637,7 +637,6 @@ static void netlink_client_deliver(struct netlink *st, d=ipaddr_to_string(dest); Message(M_ERR,"%s: dropping %s->%s, client not registered\n", st->name,s,d); - free(s); free(d); BUF_FREE(buf); return; } @@ -744,7 +743,6 @@ static void netlink_packet_deliver(struct netlink *st, d=ipaddr_to_string(dest); Message(M_DEBUG,"%s: don't know where to deliver packet " "(s=%s, d=%s)\n", st->name, s, d); - free(s); free(d); netlink_icmp_simple(st,sender,buf,ICMP_TYPE_UNREACHABLE, ICMP_CODE_NET_UNREACHABLE, icmp_noinfo); BUF_FREE(buf); @@ -760,7 +758,6 @@ static void netlink_packet_deliver(struct netlink *st, with destination network administratively prohibited */ Message(M_NOTICE,"%s: denied forwarding for packet (s=%s, d=%s)\n", st->name,s,d); - free(s); free(d); netlink_icmp_simple(st,sender,buf,ICMP_TYPE_UNREACHABLE, ICMP_CODE_NET_PROHIBITED, icmp_noinfo); @@ -899,7 +896,6 @@ static void netlink_incoming(struct netlink *st, struct netlink_client *sender, d=ipaddr_to_string(dest); Message(M_WARNING,"%s: packet from tunnel %s with bad " "source address (s=%s,d=%s)\n",st->name,sender->name,s,d); - free(s); free(d); BUF_FREE(buf); return; } @@ -913,7 +909,6 @@ static void netlink_incoming(struct netlink *st, struct netlink_client *sender, d=ipaddr_to_string(dest); Message(M_WARNING,"%s: outgoing packet with bad source address " "(s=%s,d=%s)\n",st->name,s,d); - free(s); free(d); BUF_FREE(buf); return; } @@ -996,7 +991,6 @@ static void netlink_dump_routes(struct netlink *st, bool_t requested) net=ipaddr_to_string(st->secnet_address); Message(c,"%s: point-to-point (remote end is %s); routes: ", st->name, net); - free(net); netlink_output_subnets(st,c,st->clients->subnets); Message(c,"\n"); } else { @@ -1017,7 +1011,6 @@ static void netlink_dump_routes(struct netlink *st, bool_t requested) net=ipaddr_to_string(st->secnet_address); Message(c,"%s/32 -> netlink \"%s\" (use %d)\n", net,st->name,st->localcount); - free(net); for (i=0; isubnets->entries; i++) { net=subnet_to_string(st->subnets->list[i]); Message(c,"%s ",net); diff --git a/tun.c b/tun.c index dcfd623..c511dbe 100644 --- a/tun.c +++ b/tun.c @@ -240,9 +240,7 @@ static bool_t tun_set_route(void *sst, struct netlink_client *routes) fatal("tun_set_route: unsupported route command type"); break; } - free(network); free(mask); } - free(secnetaddr); if (st->route_type==TUN_CONFIG_IOCTL) { close(fd); } -- 2.30.2