| Message ID | 1436964819-28109-3-git-send-email-jengelh@inai.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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 -- 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
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. :-) -- 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
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. -- 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
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 -- 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
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. -- 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
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
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
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(+)