[PATCH 2/5] ipaddr_to_string: SECURITY: Do not allocate

[Ian Jackson]

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 <ijackson at chiark.greenend.org.uk>
---
 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; i<st->subnets->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);
     }
-- 
1.7.10.4