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 <ijackson@chiark.greenend.org.uk>

diff --git a/ipaddr.c b/ipaddr.c
index f8cda00..6d0b02a 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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);
     }