iptables: AH/ESP init fix, and a build fix

iptables: AH/ESP init fix, and a build fix

[Jan Engelhardt]

One build fix I needed to get it running again, and then a lightly-tested
(manual only, without python) fix for
the email thread

"Subject: [Q] iptables AH module api mismatch between -master and 1.4.7"
(Message-ID: <20150715121521.GK2034@uranus>)


===
The following changes since commit 16964a99a61ff1d7cb0c260ed50b9f91f7b7a783:

  extensions: libxt_socket: add --restore-skmark option (2015-06-30 17:26:37 +0200)

are available in the git repository at:

  git://git.inai.de/iptables HEAD

for you to fetch changes up to 8c03f8ee5e4959bd5057e6b85314c6314e46843d:

  extensions: restore matching any SPI id by default (2015-07-15 14:50:51 +0200)

----------------------------------------------------------------
Jan Engelhardt (2):
      build: resolve build error involving libnftnl
      extensions: restore matching any SPI id by default

 extensions/GNUmakefile.in | 2 +-
 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 +
 9 files changed, 38 insertions(+), 1 deletion(-)

[PATCH 1/2] 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.4.3

Re: [PATCH 1/2] build: resolve build error involving libnftnl

[Pablo Neira Ayuso]

On Wed, Jul 15, 2015 at 02:53:38PM +0200, Jan Engelhardt wrote:
> 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.

Applied, thanks.

[PATCH 2/2] 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.4.3

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

[Pablo Neira Ayuso]

On Wed, Jul 15, 2015 at 02:53:39PM +0200, Jan Engelhardt wrote:
[...]
> 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

This line looks very similar to the one above the comment, but it says OK.

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

[Jan Engelhardt]

On Wednesday 2015-07-15 18:24, Pablo Neira Ayuso wrote:
>On Wed, Jul 15, 2015 at 02:53:39PM +0200, Jan Engelhardt wrote:
>[...]
>> 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
>
>This line looks very similar to the one above the comment, but it says OK.

Indeed the FAIL line is indeed mirroring the OK line, the latter of 
which should have failed the testsuite previously.

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

[Pablo Neira Ayuso]

On Wed, Jul 15, 2015 at 06:41:54PM +0200, Jan Engelhardt wrote:
> 
> On Wednesday 2015-07-15 18:24, Pablo Neira Ayuso wrote:
> >On Wed, Jul 15, 2015 at 02:53:39PM +0200, Jan Engelhardt wrote:
> >[...]
> >> 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
> >
> >This line looks very similar to the one above the comment, but it says OK.
> 
> Indeed the FAIL line is indeed mirroring the OK line, the latter of 
> which should have failed the testsuite previously.

Right.

Given that this is changing the behaviour again, I would suggest that
-p esp -m esp displays -p -m esp --espspi 0:4294967295 via
iptables-save.

I think this default behaviour is less obscure which they can still
type less from the command line.

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

[Jan Engelhardt]

On Wednesday 2015-07-15 18:55, Pablo Neira Ayuso wrote:
>> >> +-p esp -m esp;-p esp -m esp --espspi 0;FAIL
>> >>  -p esp -m esp;=;OK
>> >
>
>Given that this is changing the behaviour again, I would suggest that
>-p esp -m esp displays -p -m esp --espspi 0:4294967295 via
>iptables-save.

The printing via iptables -S was not the problem.
The patch is about that no AH/ESP packets were matched when using
just "-m esp" because of the implied --espspi 0:0.

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

[Pablo Neira Ayuso]

On Wed, Jul 15, 2015 at 07:10:29PM +0200, Jan Engelhardt wrote:
> 
> On Wednesday 2015-07-15 18:55, Pablo Neira Ayuso wrote:
> >> >> +-p esp -m esp;-p esp -m esp --espspi 0;FAIL
> >> >>  -p esp -m esp;=;OK
> >> >
> >
> >Given that this is changing the behaviour again, I would suggest that
> >-p esp -m esp displays -p -m esp --espspi 0:4294967295 via
> >iptables-save.
> 
> The printing via iptables -S was not the problem.
> The patch is about that no AH/ESP packets were matched when using
> just "-m esp" because of the implied --espspi 0:0.

Without your patch:

iptables -A INPUT -p ah

# iptables-save
...
-A INPUT -p ah -m ah --ahspi 0

With your patch:

iptables -A INPUT -p ah
iptables -A INPUT -p ah --ahspi 0:4294967295
# iptables-save
...
-A INPUT -p ah -m ah
-A INPUT -p ah -m ah

what I'm suggesting is that this prints what it indeed does:

# iptables-save
...
-A INPUT -p ah --ahspi 0:4294967295

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

[Jan Engelhardt]

On Wednesday 2015-07-15 19:30, Pablo Neira Ayuso wrote:
>> The printing via iptables -S was not the problem.
>> The patch is about that no AH/ESP packets were matched when using
>> just "-m esp" because of the implied --espspi 0:0.
>
>Without your patch:
>
>iptables -A INPUT -p ah
>
># iptables-save
>...
>-A INPUT -p ah -m ah --ahspi 0

That should not happen. -p implies -m only magically if one of the
options is used, i.e. "-p ah" alone should never imply "-m ah".

# XTABLES_LIBDIR=$PWD/extensions iptables/xtables-multi main4 -A z -p ah
# XTABLES_LIBDIR=$PWD/extensions iptables/xtables-multi main4 -S z
-N z
-A z -p ah


Second, without my patch:

# XTABLES_LIBDIR=$PWD/extensions iptables/xtables-multi main4 -A z -p ah -m ah
# XTABLES_LIBDIR=$PWD/extensions iptables/xtables-multi main4 -S z
-A z -p ah -m ah --ahspi 0

And that was the bug: --ahspi 0 is undesired behavior for when --ahspi
is never specified.


Printing it differently is a separate concern one can think about,
but with a separate patch. :-)

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

[Pablo Neira Ayuso]

On Wed, Jul 15, 2015 at 07:46:05PM +0200, Jan Engelhardt wrote:
[...]
> Printing it differently is a separate concern one can think about,
> but with a separate patch. :-)

Either way...

Will you send me that follow up patch so I can get this applied?
Thanks.

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

[Jan Engelhardt]

On Friday 2015-08-07 13:07, Pablo Neira Ayuso wrote:
>On Wed, Jul 15, 2015 at 07:46:05PM +0200, Jan Engelhardt wrote:
>[...]
>> Printing it differently is a separate concern one can think about,
>> but with a separate patch. :-)
>
>Either way...
>
>Will you send me that follow up patch so I can get this applied?


When specifying e.g. "-m policy --dir in", the xt_policy kernel
module will indeedx test for much more than just the direction, but
the additional tests it does on other fields are idempotent after
all.

I oppose that idempotent expressions in rules, implicit or explicit,
shall lead to output when the ruleset is read back. A rule like

	-A INPUT -m policy --dir in

should not, by default, cause `iptables -S` to output a
rule with terms essentially irrelevant to the human reader.

	-A INPUT -m policy --dir in --reqid 0:4294967295 --spi
	0:4294967295 proto 0 --mode 0 --tunnel-src 0.0.0.0/0
	--tunnel-dst 0.0.0.0/0

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

[Pablo Neira Ayuso]

On Fri, Aug 07, 2015 at 01:38:01PM +0200, Jan Engelhardt wrote:
[...]
> When specifying e.g. "-m policy --dir in", the xt_policy kernel
> module will indeedx test for much more than just the direction, but
> the additional tests it does on other fields are idempotent after
> all.
> 
> I oppose that idempotent expressions in rules, implicit or explicit,
> shall lead to output when the ruleset is read back. A rule like
> 
> 	-A INPUT -m policy --dir in
> 
> should not, by default, cause `iptables -S` to output a
> rule with terms essentially irrelevant to the human reader.
> 
> 	-A INPUT -m policy --dir in --reqid 0:4294967295 --spi
> 	0:4294967295 proto 0 --mode 0 --tunnel-src 0.0.0.0/0
> 	--tunnel-dst 0.0.0.0/0

We're not discussing a policy.

The point is that this has been broken for two years, chances that
users have fixed this in the ruleset without reporting is high, so
restoring the old behaviour may break things again for them.

That's why I'm insisting on the fact that switching to a less obscure
behaviour is a good idea in the very specific case of 'ah' since they
can easily detect that things have change by diffing the new and old
iptables-save output.

If you don't want to send me that follow up patch, that's very bad,
but if I have no other chance I'll make it myself.

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

[Jan Engelhardt]

On Monday 2015-08-10 14:04, Pablo Neira Ayuso wrote:
>> I oppose that idempotent expressions in rules, implicit or explicit,
>> shall lead to output when the ruleset is read back. A rule like
>> 
>> 	-A INPUT -m policy --dir in
>> 
>> should not, by default, cause `iptables -S` to output a
>> rule with terms essentially irrelevant to the human reader.
>> 
>> 	-A INPUT -m policy --dir in --reqid 0:4294967295 [...]
>
>The point is that this has been broken for two years, chances that
>users have fixed this in the ruleset without reporting is high, so
>restoring the old behaviour may break things again for them.

Users that went that route have nothing to worry. If their input
ruleset explicitly specified some --reqid x:y, then they will get the
desired x <= reqid <= y test no matter the iptables version.


>That's why I'm insisting on the fact that switching to a less obscure
>behaviour is a good idea in the very specific case of 'ah' since they
>can easily detect that things have change by diffing the new and old
>iptables-save output.

Mind you, diffing is exactly how this bug _was_ discovered. A modified 
print/save function would have done _nothing_ to prevent the bug.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html