[PATCH] libxtables: duplicated loopback address via host_to_ipaddr()

[PATCH] libxtables: duplicated loopback address via host_to_ipaddr()

[Pablo Neira Ayuso]

Originally reported as a iptables-translate problem, but this also
affects iptables and ip6tables.

 $ iptables-translate -A INPUT -s localhost -j ACCEPT

gives duplicated rules:

 nft add rule ip filter INPUT ip saddr 127.0.0.1 counter accept
 nft add rule ip filter INPUT ip saddr 127.0.0.1 counter accept

This handling sucks, but libc seem to need if we have 127.0.0.1 and ::1
entries in /etc/hosts that are common in many distros.

For more info, see:

https://sourceware.org/bugzilla/show_bug.cgi?id=4980
https://bugzilla.redhat.com/show_bug.cgi?id=496300

Reported-by: Alexander Alemayhu <alexander@alemayhu.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
What a beauty...

 libxtables/xtables.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index d43f97066ea9..80b00420e039 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1358,12 +1358,18 @@ static struct in_addr *network_to_ipaddr(const char *name)
 	return NULL;
 }
 
+static const struct in_addr *addrinfo_get_sin_addr(const struct addrinfo *addr)
+{
+	return &((const struct sockaddr_in *)addr->ai_addr)->sin_addr;
+}
+
 static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 {
 	struct in_addr *addr;
 	struct addrinfo hints;
 	struct addrinfo *res, *p;
 	int err;
+	bool loopback_seen;
 	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
@@ -1375,13 +1381,39 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
 		return NULL;
 	} else {
-		for (p = res; p != NULL; p = p->ai_next)
+		loopback_seen = false;
+		for (p = res; p != NULL; p = p->ai_next) {
+			/*
+			 * This handling sucks, but libc seem to need this
+			 * workaround when 127.0.0.1 and ::1 entries in
+			 * /etc/hosts that are common in many distros, see:
+			 *
+			 * https://sourceware.org/bugzilla/show_bug.cgi?id=4980
+			 * https://bugzilla.redhat.com/show_bug.cgi?id=496300
+			 *
+			 * Note that we cannot use AI_ADDRCONFIG because this
+			 * needs to work with br_netfilter, where we may have no
+			 * configured address.
+			 */
+			if (loopback_seen)
+				continue;
+			if (addrinfo_get_sin_addr(p)->s_addr ==
+							htonl(INADDR_LOOPBACK))
+				loopback_seen = true;
+
 			++*naddr;
+		}
+		loopback_seen = false;
 		addr = xtables_calloc(*naddr, sizeof(struct in_addr));
-		for (i = 0, p = res; p != NULL; p = p->ai_next)
-			memcpy(&addr[i++],
-			       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
+		for (i = 0, p = res; p != NULL; p = p->ai_next) {
+			if (loopback_seen)
+				continue;
+			if (addrinfo_get_sin_addr(p)->s_addr ==
+							htonl(INADDR_LOOPBACK))
+				loopback_seen = true;
+			memcpy(&addr[i++], addrinfo_get_sin_addr(p),
 			       sizeof(struct in_addr));
+		}
 		freeaddrinfo(res);
 		return addr;
 	}
-- 
2.1.4

Re: [PATCH] libxtables: duplicated loopback address via host_to_ipaddr()

[Jan Engelhardt]

Pablo wrote:
>libc seem to need if we have 127.0.0.1 and ::1
>entries in /etc/hosts that are common in many distros.

I was trying to imply that this problem is not specific to localhost,
but could happen with any host name. Testing the memory contents for
for just htonl(INADDR_LOOPBACK) does not seem the right thing.

Following is a 3-patch set to illustrate my idea of filtering
all duplicates.

[PATCH 1/3] extensions: libxt_socket: add --restore-skmark option

[Jan Engelhardt]

From: Harout Hedeshian <harouth@codeaurora.org>

xt_socket is useful for matching sockets with IP_TRANSPARENT and
taking some action on the matching packets. However, it lacks the
ability to match only a small subset of transparent sockets.

Suppose there are 2 applications, each with its own set of transparent
sockets. The first application wants all matching packets dropped,
while the second application wants them forwarded somewhere else.

Add the ability to retore the skb->mark from the sk_mark. The mark
is only restored if a matching socket is found and the transparent /
nowildcard conditions are satisfied.

Now the 2 hypothetical applications can differentiate their sockets
based on a mark value set with SO_MARK.

iptables -t mangle -I PREROUTING -m socket --transparent \
                                           --restore-skmark -j action
iptables -t mangle -A action -m mark --mark 10 -j action2
iptables -t mangle -A action -m mark --mark 11 -j action3

Signed-off-by: Harout Hedeshian <harouth@codeaurora.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 extensions/libxt_socket.c           | 71 +++++++++++++++++++++++++++++++++++++
 include/linux/netfilter/xt_socket.h |  8 +++++
 2 files changed, 79 insertions(+)

diff --git a/extensions/libxt_socket.c b/extensions/libxt_socket.c
index f19c280..a99135c 100644
--- a/extensions/libxt_socket.c
+++ b/extensions/libxt_socket.c
@@ -10,6 +10,7 @@
 enum {
 	O_TRANSPARENT = 0,
 	O_NOWILDCARD = 1,
+	O_RESTORESKMARK = 2,
 };
 
 static const struct xt_option_entry socket_mt_opts[] = {
@@ -23,6 +24,13 @@ static const struct xt_option_entry socket_mt_opts_v2[] = {
 	XTOPT_TABLEEND,
 };
 
+static const struct xt_option_entry socket_mt_opts_v3[] = {
+	{.name = "transparent", .id = O_TRANSPARENT, .type = XTTYPE_NONE},
+	{.name = "nowildcard", .id = O_NOWILDCARD, .type = XTTYPE_NONE},
+	{.name = "restore-skmark", .id = O_RESTORESKMARK, .type = XTTYPE_NONE},
+	XTOPT_TABLEEND,
+};
+
 static void socket_mt_help(void)
 {
 	printf(
@@ -38,6 +46,17 @@ static void socket_mt_help_v2(void)
 		"  --transparent    Ignore non-transparent sockets\n\n");
 }
 
+static void socket_mt_help_v3(void)
+{
+	printf(
+		"socket match options:\n"
+		"  --nowildcard     Do not ignore LISTEN sockets bound on INADDR_ANY\n"
+		"  --transparent    Ignore non-transparent sockets\n"
+		"  --restore-skmark Set the packet mark to the socket mark if\n"
+		"                   the socket matches and transparent / \n"
+		"                   nowildcard conditions are satisfied\n\n");
+}
+
 static void socket_mt_parse(struct xt_option_call *cb)
 {
 	struct xt_socket_mtinfo1 *info = cb->data;
@@ -65,6 +84,24 @@ static void socket_mt_parse_v2(struct xt_option_call *cb)
 	}
 }
 
+static void socket_mt_parse_v3(struct xt_option_call *cb)
+{
+	struct xt_socket_mtinfo2 *info = cb->data;
+
+	xtables_option_parse(cb);
+	switch (cb->entry->id) {
+	case O_TRANSPARENT:
+		info->flags |= XT_SOCKET_TRANSPARENT;
+		break;
+	case O_NOWILDCARD:
+		info->flags |= XT_SOCKET_NOWILDCARD;
+		break;
+	case O_RESTORESKMARK:
+		info->flags |= XT_SOCKET_RESTORESKMARK;
+		break;
+	}
+}
+
 static void
 socket_mt_save(const void *ip, const struct xt_entry_match *match)
 {
@@ -101,6 +138,27 @@ socket_mt_print_v2(const void *ip, const struct xt_entry_match *match,
 	socket_mt_save_v2(ip, match);
 }
 
+static void
+socket_mt_save_v3(const void *ip, const struct xt_entry_match *match)
+{
+	const struct xt_socket_mtinfo3 *info = (const void *)match->data;
+
+	if (info->flags & XT_SOCKET_TRANSPARENT)
+		printf(" --transparent");
+	if (info->flags & XT_SOCKET_NOWILDCARD)
+		printf(" --nowildcard");
+	if (info->flags & XT_SOCKET_RESTORESKMARK)
+		printf(" --restore-skmark");
+}
+
+static void
+socket_mt_print_v3(const void *ip, const struct xt_entry_match *match,
+		   int numeric)
+{
+	printf(" socket");
+	socket_mt_save_v3(ip, match);
+}
+
 static struct xtables_match socket_mt_reg[] = {
 	{
 		.name          = "socket",
@@ -136,6 +194,19 @@ static struct xtables_match socket_mt_reg[] = {
 		.x6_parse      = socket_mt_parse_v2,
 		.x6_options    = socket_mt_opts_v2,
 	},
+	{
+		.name          = "socket",
+		.revision      = 3,
+		.family        = NFPROTO_UNSPEC,
+		.version       = XTABLES_VERSION,
+		.size          = XT_ALIGN(sizeof(struct xt_socket_mtinfo2)),
+		.userspacesize = XT_ALIGN(sizeof(struct xt_socket_mtinfo2)),
+		.help          = socket_mt_help_v3,
+		.print         = socket_mt_print_v3,
+		.save          = socket_mt_save_v3,
+		.x6_parse      = socket_mt_parse_v3,
+		.x6_options    = socket_mt_opts_v3,
+	},
 };
 
 void _init(void)
diff --git a/include/linux/netfilter/xt_socket.h b/include/linux/netfilter/xt_socket.h
index 6315e2a..87644f8 100644
--- a/include/linux/netfilter/xt_socket.h
+++ b/include/linux/netfilter/xt_socket.h
@@ -6,6 +6,7 @@
 enum {
 	XT_SOCKET_TRANSPARENT = 1 << 0,
 	XT_SOCKET_NOWILDCARD = 1 << 1,
+	XT_SOCKET_RESTORESKMARK = 1 << 2,
 };
 
 struct xt_socket_mtinfo1 {
@@ -18,4 +19,11 @@ struct xt_socket_mtinfo2 {
 };
 #define XT_SOCKET_FLAGS_V2 (XT_SOCKET_TRANSPARENT | XT_SOCKET_NOWILDCARD)
 
+struct xt_socket_mtinfo3 {
+	__u8 flags;
+};
+#define XT_SOCKET_FLAGS_V3 (XT_SOCKET_TRANSPARENT \
+			   | XT_SOCKET_NOWILDCARD \
+			   | XT_SOCKET_RESTORESKMARK)
+
 #endif /* _XT_SOCKET_H */
-- 
2.10.2

[PATCH 2/3] build: resolve build error involving libnftnl

[Jan Engelhardt]

make[2]: Entering directory '/home/jengelh/code/iptables/extensions'
  CC       libebt_limit.oo
In file included from ../iptables/nft.h:5:0,
                 from libebt_limit.c:21:
../iptables/nft-shared.h:6:27: fatal error: libnftnl/rule.h: No such file or directory
 #include <libnftnl/rule.h>
                           ^
compilation terminated.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/GNUmakefile.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/GNUmakefile.in b/extensions/GNUmakefile.in
index 4e94bd0..181e155 100644
--- a/extensions/GNUmakefile.in
+++ b/extensions/GNUmakefile.in
@@ -21,7 +21,7 @@ regular_CPPFLAGS   = @regular_CPPFLAGS@
 kinclude_CPPFLAGS  = @kinclude_CPPFLAGS@
 
 AM_CFLAGS       = ${regular_CFLAGS}
-AM_CPPFLAGS     = ${regular_CPPFLAGS} -I${top_builddir}/include -I${top_builddir} -I${top_srcdir}/include ${kinclude_CPPFLAGS} ${CPPFLAGS} @libnetfilter_conntrack_CFLAGS@
+AM_CPPFLAGS     = ${regular_CPPFLAGS} -I${top_builddir}/include -I${top_builddir} -I${top_srcdir}/include ${kinclude_CPPFLAGS} ${CPPFLAGS} @libnetfilter_conntrack_CFLAGS@ @libnftnl_CFLAGS@
 AM_DEPFLAGS     = -Wp,-MMD,$(@D)/.$(@F).d,-MT,$@
 AM_LDFLAGS      = @noundef_LDFLAGS@
 
-- 
2.10.2

[PATCH 3/3] extensions: restore matching any SPI id by default

[Jan Engelhardt]

This is the same as commit v1.4.15-12-g8a988f6.

If no id option is given, the extensions only match packets with a
zero-valued identification field. This behavior deviates from what it
used to do back in v1.4.10-273-g6944f2c^.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libip6t_ah.c | 9 +++++++++
 extensions/libip6t_ah.t | 1 +
 extensions/libip6t_rt.c | 8 ++++++++
 extensions/libip6t_rt.t | 1 +
 extensions/libipt_ah.c  | 8 ++++++++
 extensions/libipt_ah.t  | 1 +
 extensions/libxt_esp.c  | 8 ++++++++
 extensions/libxt_esp.t  | 1 +
 8 files changed, 37 insertions(+)

diff --git a/extensions/libip6t_ah.c b/extensions/libip6t_ah.c
index 26f8140..174d6d1 100644
--- a/extensions/libip6t_ah.c
+++ b/extensions/libip6t_ah.c
@@ -28,6 +28,14 @@ static const struct xt_option_entry ah_opts[] = {
 };
 #undef s
 
+static void ah_init(struct xt_entry_match *m)
+{
+	struct ip6t_ah *ahinfo = (void *)m->data;
+
+	/* Defaults for when no --ahspi is used at all */
+	ahinfo->spis[1] = ~0U;
+}
+
 static void ah_parse(struct xt_option_call *cb)
 {
 	struct ip6t_ah *ahinfo = cb->data;
@@ -127,6 +135,7 @@ static struct xtables_match ah_mt6_reg = {
 	.size          = XT_ALIGN(sizeof(struct ip6t_ah)),
 	.userspacesize = XT_ALIGN(sizeof(struct ip6t_ah)),
 	.help          = ah_help,
+	.init          = ah_init,
 	.print         = ah_print,
 	.save          = ah_save,
 	.x6_parse      = ah_parse,
diff --git a/extensions/libip6t_ah.t b/extensions/libip6t_ah.t
index 459e9ec..36ca7df 100644
--- a/extensions/libip6t_ah.t
+++ b/extensions/libip6t_ah.t
@@ -12,3 +12,4 @@
 -m ah --ahspi invalid;;FAIL
 -m ah --ahspi 0:invalid;;FAIL
 -m ah --ahspi;;FAIL
+-m ah;-m ah --ahspi 0;FAIL
diff --git a/extensions/libip6t_rt.c b/extensions/libip6t_rt.c
index d470488..cada779 100644
--- a/extensions/libip6t_rt.c
+++ b/extensions/libip6t_rt.c
@@ -99,6 +99,13 @@ parse_addresses(const char *addrstr, struct in6_addr *addrp)
 	return i;
 }
 
+static void rt_init(struct xt_entry_match *m)
+{
+	struct ip6t_rt *rtinfo = (void *)m->data;
+
+	rtinfo->segsleft[1] = ~0U;
+}
+
 static void rt_parse(struct xt_option_call *cb)
 {
 	struct ip6t_rt *rtinfo = cb->data;
@@ -245,6 +252,7 @@ static struct xtables_match rt_mt6_reg = {
 	.size		= XT_ALIGN(sizeof(struct ip6t_rt)),
 	.userspacesize	= XT_ALIGN(sizeof(struct ip6t_rt)),
 	.help		= rt_help,
+	.init		= rt_init,
 	.x6_parse	= rt_parse,
 	.print		= rt_print,
 	.save		= rt_save,
diff --git a/extensions/libip6t_rt.t b/extensions/libip6t_rt.t
index 7170138..553123e 100644
--- a/extensions/libip6t_rt.t
+++ b/extensions/libip6t_rt.t
@@ -2,3 +2,4 @@
 -m rt --rt-type 0 --rt-segsleft 1:23 --rt-len 42 --rt-0-res;=;OK
 -m rt --rt-type 0 ! --rt-segsleft 1:23 ! --rt-len 42 --rt-0-res;=;OK
 -m rt ! --rt-type 1 ! --rt-segsleft 12:23 ! --rt-len 42;=;OK
+-m rt;-m rt --rtsegsleft 0;FAIL
diff --git a/extensions/libipt_ah.c b/extensions/libipt_ah.c
index 8cf167c..a490729 100644
--- a/extensions/libipt_ah.c
+++ b/extensions/libipt_ah.c
@@ -21,6 +21,13 @@ static const struct xt_option_entry ah_opts[] = {
 	XTOPT_TABLEEND,
 };
 
+static void ah_init(struct xt_entry_match *m)
+{
+	struct ipt_ah *ahinfo = (void *)m->data;
+
+	ahinfo->spis[1] = ~0U;
+}
+
 static void ah_parse(struct xt_option_call *cb)
 {
 	struct ipt_ah *ahinfo = cb->data;
@@ -92,6 +99,7 @@ static struct xtables_match ah_mt_reg = {
 	.size		= XT_ALIGN(sizeof(struct ipt_ah)),
 	.userspacesize 	= XT_ALIGN(sizeof(struct ipt_ah)),
 	.help 		= ah_help,
+	.init		= ah_init,
 	.print 		= ah_print,
 	.save 		= ah_save,
 	.x6_parse	= ah_parse,
diff --git a/extensions/libipt_ah.t b/extensions/libipt_ah.t
index a0ce3b0..2993906 100644
--- a/extensions/libipt_ah.t
+++ b/extensions/libipt_ah.t
@@ -10,3 +10,4 @@
 -m ah --ahspi 0;;FAIL
 -m ah --ahspi;;FAIL
 -m ah;;FAIL
+-p ah -m ah;-p ah -m ah --ahspi 0;FAIL
diff --git a/extensions/libxt_esp.c b/extensions/libxt_esp.c
index 294338b..773d6af 100644
--- a/extensions/libxt_esp.c
+++ b/extensions/libxt_esp.c
@@ -21,6 +21,13 @@ static const struct xt_option_entry esp_opts[] = {
 	XTOPT_TABLEEND,
 };
 
+static void esp_init(struct xt_entry_match *m)
+{
+	struct xt_esp *espinfo = (void *)m->data;
+
+	espinfo->spis[1] = ~0U;
+}
+
 static void esp_parse(struct xt_option_call *cb)
 {
 	struct xt_esp *espinfo = cb->data;
@@ -86,6 +93,7 @@ static struct xtables_match esp_match = {
 	.size		= XT_ALIGN(sizeof(struct xt_esp)),
 	.userspacesize	= XT_ALIGN(sizeof(struct xt_esp)),
 	.help		= esp_help,
+	.init		= esp_init,
 	.print		= esp_print,
 	.save		= esp_save,
 	.x6_parse	= esp_parse,
diff --git a/extensions/libxt_esp.t b/extensions/libxt_esp.t
index 008013b..f207def 100644
--- a/extensions/libxt_esp.t
+++ b/extensions/libxt_esp.t
@@ -4,6 +4,7 @@
 -p esp -m esp --espspi 0:4294967295;-p esp -m esp;OK
 -p esp -m esp ! --espspi 0:4294967294;=;OK
 -p esp -m esp --espspi -1;;FAIL
+-p esp -m esp;-p esp -m esp --espspi 0;FAIL
 # should fail?
 -p esp -m esp;=;OK
 -m esp;;FAIL
-- 
2.10.2

Filter duplicate IP addresses from libxtables

[Jan Engelhardt]

(Of course that send went wrong.. with github and all that, I hardly
had to use git-send-email ever since.)
The right set follows.

[PATCH 1/3] libxtables: remove unnecessary nesting from host_to_ip(6)addr

[Jan Engelhardt]

The error path already terminally returns from the function, so there
is no point in having an explicit else block.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 54 +++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index d43f970..ae73a06 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1372,21 +1372,18 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	hints.ai_socktype = SOCK_RAW;
 
 	*naddr = 0;
-	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
+	err = getaddrinfo(name, NULL, &hints, &res);
+	if (err != 0)
 		return NULL;
-	} else {
-		for (p = res; p != NULL; p = p->ai_next)
-			++*naddr;
-		addr = xtables_calloc(*naddr, sizeof(struct in_addr));
-		for (i = 0, p = res; p != NULL; p = p->ai_next)
-			memcpy(&addr[i++],
-			       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
-			       sizeof(struct in_addr));
-		freeaddrinfo(res);
-		return addr;
-	}
-
-	return NULL;
+	for (p = res; p != NULL; p = p->ai_next)
+		++*naddr;
+	addr = xtables_calloc(*naddr, sizeof(struct in_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next)
+		memcpy(&addr[i++],
+		       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
+		       sizeof(struct in_addr));
+	freeaddrinfo(res);
+	return addr;
 }
 
 static struct in_addr *
@@ -1662,23 +1659,20 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	hints.ai_socktype = SOCK_RAW;
 
 	*naddr = 0;
-	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
+	err = getaddrinfo(name, NULL, &hints, &res);
+	if (err != 0)
 		return NULL;
-	} else {
-		/* Find length of address chain */
-		for (p = res; p != NULL; p = p->ai_next)
-			++*naddr;
-		/* Copy each element of the address chain */
-		addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
-		for (i = 0, p = res; p != NULL; p = p->ai_next)
-			memcpy(&addr[i++],
-			       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
-			       sizeof(struct in6_addr));
-		freeaddrinfo(res);
-		return addr;
-	}
-
-	return NULL;
+	/* Find length of address chain */
+	for (p = res; p != NULL; p = p->ai_next)
+		++*naddr;
+	/* Copy each element of the address chain */
+	addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next)
+		memcpy(&addr[i++],
+		       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
+		       sizeof(struct in6_addr));
+	freeaddrinfo(res);
+	return addr;
 }
 
 static struct in6_addr *network_to_ip6addr(const char *name)
-- 
2.10.2

Re: [PATCH 1/3] libxtables: remove unnecessary nesting from host_to_ip(6)addr

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 05:26:56PM +0100, Jan Engelhardt wrote:
> The error path already terminally returns from the function, so there
> is no point in having an explicit else block.

Applied.

[PATCH 2/3] libxtables: abolish AI_CANONNAME

[Jan Engelhardt]

ares->ai_canonname is never used, so there is no point in requesting
that piece of information with AI_CANONNAME.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index ae73a06..891d81a 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1367,7 +1367,6 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
-	hints.ai_flags    = AI_CANONNAME;
 	hints.ai_family   = AF_INET;
 	hints.ai_socktype = SOCK_RAW;
 
@@ -1654,7 +1653,6 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
-	hints.ai_flags    = AI_CANONNAME;
 	hints.ai_family   = AF_INET6;
 	hints.ai_socktype = SOCK_RAW;
 
-- 
2.10.2

Re: [PATCH 2/3] libxtables: abolish AI_CANONNAME

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 05:26:57PM +0100, Jan Engelhardt wrote:
> ares->ai_canonname is never used, so there is no point in requesting
> that piece of information with AI_CANONNAME.

I already had one like this here that derived from my previous patch,
anyway. Applied.

[PATCH 3/3] libxtables: avoid returning duplicate address for host resolution

[Jan Engelhardt]

A long-standing problem has been that `iptables -s any_host_here`
could yield multiple rules with the same address if the DNS was
indeed so populated.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 891d81a..d994306 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1364,7 +1364,7 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	struct addrinfo hints;
 	struct addrinfo *res, *p;
 	int err;
-	unsigned int i;
+	size_t i, j;
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_family   = AF_INET;
@@ -1374,14 +1374,23 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	err = getaddrinfo(name, NULL, &hints, &res);
 	if (err != 0)
 		return NULL;
-	for (p = res; p != NULL; p = p->ai_next)
-		++*naddr;
 	addr = xtables_calloc(*naddr, sizeof(struct in_addr));
-	for (i = 0, p = res; p != NULL; p = p->ai_next)
-		memcpy(&addr[i++],
-		       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
-		       sizeof(struct in_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next) {
+		const struct sockaddr_in *newaddr = (const void *)p->ai_addr;
+		bool seen = false;
+
+		for (j = 0; j < i; ++j) {
+			if (memcmp(newaddr, &addr[j], sizeof(*newaddr)) == 0) {
+				seen = true;
+				break;
+			}
+		}
+		if (seen)
+			continue;
+		memcpy(&addr[i++], &newaddr->sin_addr, sizeof(newaddr->sin_addr));
+	}
 	freeaddrinfo(res);
+	*naddr = i;
 	return addr;
 }
 
@@ -1650,7 +1659,7 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	struct addrinfo hints;
 	struct addrinfo *res, *p;
 	int err;
-	unsigned int i;
+	size_t i, j;
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_family   = AF_INET6;
@@ -1665,11 +1674,22 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 		++*naddr;
 	/* Copy each element of the address chain */
 	addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
-	for (i = 0, p = res; p != NULL; p = p->ai_next)
-		memcpy(&addr[i++],
-		       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
-		       sizeof(struct in6_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next) {
+		const struct sockaddr_in6 *newaddr = (const void *)p->ai_addr;
+		bool seen = false;
+
+		for (j = 0; j < i; ++j) {
+			if (memcmp(newaddr, &addr[j], sizeof(*newaddr)) == 0) {
+				seen = true;
+				break;
+			}
+		}
+		if (seen)
+			continue;
+		memcpy(&addr[i++], &newaddr->sin6_addr, sizeof(newaddr->sin6_addr));
+	}
 	freeaddrinfo(res);
+	*naddr = i;
 	return addr;
 }
 
-- 
2.10.2

Re: [PATCH 3/3] libxtables: avoid returning duplicate address for host resolution

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 05:26:58PM +0100, Jan Engelhardt wrote:
> A long-standing problem has been that `iptables -s any_host_here`
> could yield multiple rules with the same address if the DNS was
> indeed so populated.

When did anyone report this problem out of the localhost case?

Re: [PATCH 3/3] libxtables: avoid returning duplicate address for host resolution

[Jan Engelhardt]

On Wednesday 2017-03-08 17:45, Pablo Neira Ayuso wrote:

>On Wed, Mar 08, 2017 at 05:26:58PM +0100, Jan Engelhardt wrote:
>> A long-standing problem has been that `iptables -s any_host_here`
>> could yield multiple rules with the same address if the DNS was
>> indeed so populated.
>
>When did anyone report this problem out of the localhost case?

It's been a long time. I think the issue was actually that one can 
specify multiple host names, and if those hostnames happen to resolve to 
the same address in the end, iptables would emit two rules of which one 
is essentially redundant.

  iptables -A INPUT -s www2.company.com,www3.company.com

Re: [PATCH 3/3] libxtables: avoid returning duplicate address for host resolution

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 05:56:43PM +0100, Jan Engelhardt wrote:
> On Wednesday 2017-03-08 17:45, Pablo Neira Ayuso wrote:
> 
> >On Wed, Mar 08, 2017 at 05:26:58PM +0100, Jan Engelhardt wrote:
> >> A long-standing problem has been that `iptables -s any_host_here`
> >> could yield multiple rules with the same address if the DNS was
> >> indeed so populated.
> >
> >When did anyone report this problem out of the localhost case?
> 
> It's been a long time. I think the issue was actually that one can 
> specify multiple host names, and if those hostnames happen to resolve to 
> the same address in the end, iptables would emit two rules of which one 
> is essentially redundant.
> 
>   iptables -A INPUT -s www2.company.com,www3.company.com

Got me thinking, are you sure we want to fix this?

I think this rule expansion based on DNS is probably one of the most
creepy features that we have in iptables... Probably if we leave it
broken this will just scare people away from using this :).

But your call.

[PATCH 1/3] libxtables: remove unnecessary nesting from host_to_ip(6)addr

[Jan Engelhardt]

The error path already terminally returns from the function, so there
is no point in having an explicit else block.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 54 +++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index d43f970..ae73a06 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1372,21 +1372,18 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	hints.ai_socktype = SOCK_RAW;
 
 	*naddr = 0;
-	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
+	err = getaddrinfo(name, NULL, &hints, &res);
+	if (err != 0)
 		return NULL;
-	} else {
-		for (p = res; p != NULL; p = p->ai_next)
-			++*naddr;
-		addr = xtables_calloc(*naddr, sizeof(struct in_addr));
-		for (i = 0, p = res; p != NULL; p = p->ai_next)
-			memcpy(&addr[i++],
-			       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
-			       sizeof(struct in_addr));
-		freeaddrinfo(res);
-		return addr;
-	}
-
-	return NULL;
+	for (p = res; p != NULL; p = p->ai_next)
+		++*naddr;
+	addr = xtables_calloc(*naddr, sizeof(struct in_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next)
+		memcpy(&addr[i++],
+		       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
+		       sizeof(struct in_addr));
+	freeaddrinfo(res);
+	return addr;
 }
 
 static struct in_addr *
@@ -1662,23 +1659,20 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	hints.ai_socktype = SOCK_RAW;
 
 	*naddr = 0;
-	if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
+	err = getaddrinfo(name, NULL, &hints, &res);
+	if (err != 0)
 		return NULL;
-	} else {
-		/* Find length of address chain */
-		for (p = res; p != NULL; p = p->ai_next)
-			++*naddr;
-		/* Copy each element of the address chain */
-		addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
-		for (i = 0, p = res; p != NULL; p = p->ai_next)
-			memcpy(&addr[i++],
-			       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
-			       sizeof(struct in6_addr));
-		freeaddrinfo(res);
-		return addr;
-	}
-
-	return NULL;
+	/* Find length of address chain */
+	for (p = res; p != NULL; p = p->ai_next)
+		++*naddr;
+	/* Copy each element of the address chain */
+	addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next)
+		memcpy(&addr[i++],
+		       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
+		       sizeof(struct in6_addr));
+	freeaddrinfo(res);
+	return addr;
 }
 
 static struct in6_addr *network_to_ip6addr(const char *name)
-- 
2.10.2

[PATCH 2/3] libxtables: abolish AI_CANONNAME

[Jan Engelhardt]

ares->ai_canonname is never used, so there is no point in requesting
that piece of information with AI_CANONNAME.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index ae73a06..891d81a 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1367,7 +1367,6 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
-	hints.ai_flags    = AI_CANONNAME;
 	hints.ai_family   = AF_INET;
 	hints.ai_socktype = SOCK_RAW;
 
@@ -1654,7 +1653,6 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	unsigned int i;
 
 	memset(&hints, 0, sizeof(hints));
-	hints.ai_flags    = AI_CANONNAME;
 	hints.ai_family   = AF_INET6;
 	hints.ai_socktype = SOCK_RAW;
 
-- 
2.10.2

[PATCH 3/3] libxtables: avoid returning duplicate address for host resolution

[Jan Engelhardt]

A long-standing problem has been that `iptables -s any_host_here`
could yield multiple rules with the same address if the DNS was
indeed so populated.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 libxtables/xtables.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 891d81a..d994306 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -1364,7 +1364,7 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	struct addrinfo hints;
 	struct addrinfo *res, *p;
 	int err;
-	unsigned int i;
+	size_t i, j;
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_family   = AF_INET;
@@ -1374,14 +1374,23 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
 	err = getaddrinfo(name, NULL, &hints, &res);
 	if (err != 0)
 		return NULL;
-	for (p = res; p != NULL; p = p->ai_next)
-		++*naddr;
 	addr = xtables_calloc(*naddr, sizeof(struct in_addr));
-	for (i = 0, p = res; p != NULL; p = p->ai_next)
-		memcpy(&addr[i++],
-		       &((const struct sockaddr_in *)p->ai_addr)->sin_addr,
-		       sizeof(struct in_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next) {
+		const struct sockaddr_in *newaddr = (const void *)p->ai_addr;
+		bool seen = false;
+
+		for (j = 0; j < i; ++j) {
+			if (memcmp(newaddr, &addr[j], sizeof(*newaddr)) == 0) {
+				seen = true;
+				break;
+			}
+		}
+		if (seen)
+			continue;
+		memcpy(&addr[i++], &newaddr->sin_addr, sizeof(newaddr->sin_addr));
+	}
 	freeaddrinfo(res);
+	*naddr = i;
 	return addr;
 }
 
@@ -1650,7 +1659,7 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 	struct addrinfo hints;
 	struct addrinfo *res, *p;
 	int err;
-	unsigned int i;
+	size_t i, j;
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_family   = AF_INET6;
@@ -1665,11 +1674,22 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
 		++*naddr;
 	/* Copy each element of the address chain */
 	addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
-	for (i = 0, p = res; p != NULL; p = p->ai_next)
-		memcpy(&addr[i++],
-		       &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
-		       sizeof(struct in6_addr));
+	for (i = 0, p = res; p != NULL; p = p->ai_next) {
+		const struct sockaddr_in6 *newaddr = (const void *)p->ai_addr;
+		bool seen = false;
+
+		for (j = 0; j < i; ++j) {
+			if (memcmp(newaddr, &addr[j], sizeof(*newaddr)) == 0) {
+				seen = true;
+				break;
+			}
+		}
+		if (seen)
+			continue;
+		memcpy(&addr[i++], &newaddr->sin6_addr, sizeof(newaddr->sin6_addr));
+	}
 	freeaddrinfo(res);
+	*naddr = i;
 	return addr;
 }
 
-- 
2.10.2

Re: [PATCH] libxtables: duplicated loopback address via host_to_ipaddr()

[Alexander Alemayhu]

On Wed, Mar 08, 2017 at 03:33:04PM +0100, Pablo Neira Ayuso wrote:
> Originally reported as a iptables-translate problem, but this also
> affects iptables and ip6tables.
> 
>  $ iptables-translate -A INPUT -s localhost -j ACCEPT
> 
> gives duplicated rules:
> 
>  nft add rule ip filter INPUT ip saddr 127.0.0.1 counter accept
>  nft add rule ip filter INPUT ip saddr 127.0.0.1 counter accept
> 
> This handling sucks, but libc seem to need if we have 127.0.0.1 and ::1
> entries in /etc/hosts that are common in many distros.
> 
> For more info, see:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=4980
> https://bugzilla.redhat.com/show_bug.cgi?id=496300
> 
> Reported-by: Alexander Alemayhu <alexander@alemayhu.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Was going to test it, but it does not apply.

Applying: libxtables: duplicated loopback address via host_to_ipaddr()
error: patch failed: libxtables/xtables.c:1375
error: libxtables/xtables.c: patch does not apply
Patch failed at 0001 libxtables: duplicated loopback address via host_to_ipaddr()
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

-- 
Mit freundlichen Grüßen

Alexander Alemayhu