xtables: various fixes for handling multiple src/dst addresses

xtables: various fixes for handling multiple src/dst addresses

[Wes Campaigne]

Hi! I've been working on enabling robust IPv6 support in the TomatoUSB router firmware. In the process, I found and fixed a few old iptables bugs, mostly related to lookups and the handling of multiple addresses. Please review!

NOTE: TomatoUSB is actually still using an old 1.3.8 branch of iptables, and analogous versions of these patches against that version have been commited on the TomatoUSB repo at repo.or.cz/w/tomato.git tomato-RT

===
Wes Campaigne (4):
  xtables: use *all* IPv6 addresses resolved from a hostname
  xtables: fix excessive memory allocation in host_to_ipaddr
  xtables: fix the broken detection/removal of redundant addresses
  xtables: use the correct loop count when applying masks to addresses

 xtables.c |   45 ++++++++++++++++++++-------------------------
 1 files changed, 20 insertions(+), 25 deletions(-)

[PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname

[Wes Campaigne]

Fixes a long-standing issue where host_to_ip6addr would only ever
examine/return the only the first item of the address chain
returned by getaddrinfo, instead of traversing the chain and
copying each of them.

This has always been how host_to_ipaddr behaves, and all of the other
related ipv6 code is already written to handle multiple possible addresses.

Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
 xtables.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/xtables.c b/xtables.c
index 57d5d13..aea32ca 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1415,17 +1415,17 @@ struct in6_addr *xtables_numeric_to_ip6addr(const char *num)
 static struct in6_addr *
 host_to_ip6addr(const char *name, unsigned int *naddr)
 {
-	static struct in6_addr *addr;
+	struct in6_addr *addr;
 	struct addrinfo hints;
 	struct addrinfo *res;
+	struct addrinfo *p;
 	int err;
+	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_flags    = AI_CANONNAME;
 	hints.ai_family   = AF_INET6;
 	hints.ai_socktype = SOCK_RAW;
-	hints.ai_protocol = IPPROTO_IPV6;
-	hints.ai_next     = NULL;
 
 	*naddr = 0;
 	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
@@ -1434,20 +1434,19 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 #endif
 		return NULL;
 	} else {
-		if (res->ai_family != AF_INET6 ||
-		    res->ai_addrlen != sizeof(struct sockaddr_in6))
-			return NULL;
-
+		/* Find length of address-chain */
+		for(p = res; p != NULL; p = p->ai_next)
+			(*naddr)++;
 #ifdef DEBUG
 		fprintf(stderr, "resolved: len=%d  %s ", res->ai_addrlen,
 		        xtables_ip6addr_to_numeric(&((struct sockaddr_in6 *)res->ai_addr)->sin6_addr));
 #endif
-		/* Get the first element of the address-chain */
-		addr = xtables_malloc(sizeof(struct in6_addr));
-		memcpy(addr, &((const struct sockaddr_in6 *)res->ai_addr)->sin6_addr,
-		       sizeof(struct in6_addr));
+		/* Copy each element of the address-chain */
+		addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
+		for(p = res, i = 0; p != NULL && i < *naddr; p = p->ai_next, i++)
+			memcpy(&addr[i], &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
+			       sizeof(struct in6_addr));
 		freeaddrinfo(res);
-		*naddr = 1;
 		return addr;
 	}
 
@@ -1559,12 +1558,6 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
 			strcpy(buf, "::");
 
 		addrp = ip6parse_hostnetwork(buf, &n);
-		/* ip6parse_hostnetwork only ever returns one IP
-		address (it exits if the resolution fails).
-		Therefore, n will always be 1 here.  Leaving the
-		code below in anyway in case ip6parse_hostnetwork
-		is improved some day to behave like
-		ipparse_hostnetwork: */
 		if (n > 1) {
 			count += n - 1;
 			*addrpp = xtables_realloc(*addrpp,
-- 
1.7.1

[PATCH 2/4] xtables: fix excessive memory allocation in host_to_ipaddr

[Wes Campaigne]

host_to_ipaddr was unnecessarily asking for an array of length n^2
to store just n addresses.

Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
 xtables.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xtables.c b/xtables.c
index aea32ca..9a51e81 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1143,7 +1143,7 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 
 		while (host->h_addr_list[*naddr] != NULL)
 			++*naddr;
-		addr = xtables_calloc(*naddr, sizeof(struct in_addr) * *naddr);
+		addr = xtables_calloc(*naddr, sizeof(struct in_addr));
 		for (i = 0; i < *naddr; i++)
 			memcpy(&addr[i], host->h_addr_list[i],
 			       sizeof(struct in_addr));
-- 
1.7.1

[PATCH 3/4] xtables: fix the broken detection/removal of redundant addresses

[Wes Campaigne]

This same block of code, apparently to detect if addresses are
identical after applying the mask, and to skip the duplicates and
the ones made redundant by the mask, has been present and unchanged
from as far back as I could find (circa iptables 1.2).

By inspection, it was wrong, and always has been: once the code
finds a duplicate, it will drop the rest of the array one by one as
it re-detects the same duplicate over and over. When the addresses
came from a single hostname lookup, and their order was random,
then this created unpredictable behaviour by iptables, which seem
to ignore some of those addresses at random times.

I suspect the original idea also involved a swap between the
duplicate and the address from the (current) end of the array, but
a line of code to do that seems to have never existed. I've finally
added it. (Well, as much as is needed: there doesn't need to be a
full swap, because we're just going to ignore the duplicate,
pretend the array is one shorter, and never look at the contents of
the end again. So, we can get away with just copying from the end.)

Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
 xtables.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xtables.c b/xtables.c
index 9a51e81..c96efa0 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1306,13 +1306,14 @@ void xtables_ipparse_any(const char *name, struct in_addr **addrpp,
 		strcpy(buf, "0.0.0.0");
 
 	addrp = *addrpp = ipparse_hostnetwork(buf, naddrs);
+	/* Shuffle from end of array to remove redundant addresses */
 	n = *naddrs;
 	for (i = 0, j = 0; i < n; ++i) {
 		addrp[j++].s_addr &= maskp->s_addr;
 		for (k = 0; k < j - 1; ++k)
 			if (addrp[k].s_addr == addrp[j-1].s_addr) {
-				--*naddrs;
-				--j;
+				memcpy(&addrp[--j], &addrp[--*naddrs],
+				       sizeof(struct in_addr));
 				break;
 			}
 	}
@@ -1608,6 +1609,7 @@ void xtables_ip6parse_any(const char *name, struct in6_addr **addrpp,
 		strcpy(buf, "::");
 
 	addrp = *addrpp = ip6parse_hostnetwork(buf, naddrs);
+	/* Shuffle from end of array to remove redundant addresses */
 	n = *naddrs;
 	for (i = 0, j = 0; i < n; ++i) {
 		for (k = 0; k < 4; ++k)
@@ -1615,8 +1617,8 @@ void xtables_ip6parse_any(const char *name, struct in6_addr **addrpp,
 		++j;
 		for (k = 0; k < j - 1; ++k)
 			if (IN6_ARE_ADDR_EQUAL(&addrp[k], &addrp[j - 1])) {
-				--*naddrs;
-				--j;
+				memcpy(&addrp[--j], &addrp[--*naddrs],
+				       sizeof(struct in_addr));
 				break;
 			}
 	}
-- 
1.7.1

[PATCH 4/4] xtables: use the correct loop count when applying masks to addresses

[Wes Campaigne]

'n' is a subtotal from the inner loop; 'count' is the total number
of addrs/masks. This problem wasn't merely cosmetic -- rules where
the address has not been zeroed in accordance with the mask are 
silently ignored by netfilter.

Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
 xtables.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xtables.c b/xtables.c
index c96efa0..f872754 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1272,7 +1272,7 @@ void xtables_ipparse_multiple(const char *name, struct in_addr **addrpp,
 		free(addrp);
 	}
 	*naddrs = count;
-	for (i = 0; i < n; ++i)
+	for (i = 0; i < count; ++i)
 		(*addrpp+i)->s_addr &= (*maskpp+i)->s_addr;
 }
 
@@ -1581,7 +1581,7 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
 		free(addrp);
 	}
 	*naddrs = count;
-	for (i = 0; i < n; ++i)
+	for (i = 0; i < count; ++i)
 		for (j = 0; j < 4; ++j)
 			(*addrpp+i)->s6_addr32[j] &= (*maskpp+i)->s6_addr32[j];
 }
-- 
1.7.1

Re: xtables: various fixes for handling multiple src/dst addresses

[Jan Engelhardt]

On Tuesday 2011-02-22 01:10, Wes Campaigne wrote:

>Hi! I've been working on enabling robust IPv6 support in the TomatoUSB router firmware. In the process, I found and fixed a few old iptables bugs, mostly related to lookups and the handling of multiple addresses. Please review!
>
>NOTE: TomatoUSB is actually still using an old 1.3.8 branch of iptables, and analogous versions of these patches against that version have been commited on the TomatoUSB repo at repo.or.cz/w/tomato.git tomato-RT
>
>===
>Wes Campaigne (4):
>  xtables: use *all* IPv6 addresses resolved from a hostname
>  xtables: fix excessive memory allocation in host_to_ipaddr
>  xtables: fix the broken detection/removal of redundant addresses
>  xtables: use the correct loop count when applying masks to addresses

The last one has already beek taken care of (cf.
http://bugs.debian.org/611990), I am checking the rest as the
lines enlighten me :)