| Message ID | 20160530194748.GA25328@sonyv |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On 30 May 2016 at 21:47, Laura Garcia Liebana <nevola@gmail.com> wrote: > Add translation for multiport to nftables, which it's supported natively. > > Examples: > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80,81 -j ACCEPT > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80,81} counter accept > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80:88 -j ACCEPT > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80-88} counter accept > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport ! --dports 80:88 -j ACCEPT > nft add rule ip filter INPUT ip protocol tcp tcp dport != 80-88 counter accept > Lets clarify the syntax, this is valid: tcp dport 8000-8100 tcp dport { 8000-8100} tcp dport { 8000-8100, 9000-9100} but they mean different things. It seems we should avoid the braces {} for the range case, otherwise we would be using a set with a single element. However, tcp dport {8000,8100} <-- valid tcp dport 8000,8100 <-- invalid So we should always use braces {} in the non-range case. Same seems to apply in the case of inversion. so we end with this combinations: tcp dport {x,x} tcp dport != {x,x} tcp dport x-x tcp dport != x-x BTW, related to this, there seems to be a bug in nftables: % nft add rule t c tcp dport != {80, 81} BUG: invalid expression type set nft: evaluate.c:1463: expr_evaluate_relational: Assertion `0' failed. Aborted > Signed-off-by: Laura Garcia Liebana <nevola@gmail.com> > --- > Changes in v2: > - Add curley brackets to lists and range of ports. > > extensions/libxt_multiport.c | 116 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 116 insertions(+) > > diff --git a/extensions/libxt_multiport.c b/extensions/libxt_multiport.c > index 03af5a9..25b5589 100644 > --- a/extensions/libxt_multiport.c > +++ b/extensions/libxt_multiport.c > @@ -468,6 +468,118 @@ static void multiport_save6_v1(const void *ip_void, > __multiport_save_v1(match, ip->proto); > } > > +static int __multiport_xlate(const void *ip, const struct xt_entry_match *match, > + struct xt_xlate *xl, int numeric) > +{ > + const struct xt_multiport *multiinfo > + = (const struct xt_multiport *)match->data; > + unsigned int i; > + > + switch (multiinfo->flags) { > + case XT_MULTIPORT_SOURCE: > + xt_xlate_add(xl, "sport "); > + break; > + case XT_MULTIPORT_DESTINATION: > + xt_xlate_add(xl, "dport "); > + break; > + case XT_MULTIPORT_EITHER: > + return 0; > + } this case XT_MULTIPORT_EITHER seems unsupported in nftables. Is there anything established to do in case we find an impossible translation? print a warning or something? I don't know right now. I guess we should avoid printing an invalid nftables rule as if the translation was 100% ok (which is not true in this case). Just wondering, I should check myself because I don't know this right now. > + > + if (multiinfo->count > 1) > + xt_xlate_add(xl, "{ "); > + > + for (i = 0; i < multiinfo->count; i++) > + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); > + > + if (multiinfo->count > 1) > + xt_xlate_add(xl, "}"); > + > + xt_xlate_add(xl, " "); > + > + return 1; > +} > + > +static int multiport_xlate(const void *ip, const struct xt_entry_match *match, > + struct xt_xlate *xl, int numeric) > +{ > + uint8_t proto = ((const struct ipt_ip *)ip)->proto; > + > + xt_xlate_add(xl, "%s ", proto_to_name(proto)); > + return __multiport_xlate(ip, match, xl, numeric); > +} > + > +static int multiport_xlate6(const void *ip, const struct xt_entry_match *match, > + struct xt_xlate *xl, int numeric) > +{ > + uint8_t proto = ((const struct ip6t_ip6 *)ip)->proto; > + > + xt_xlate_add(xl, "%s ", proto_to_name(proto)); > + return __multiport_xlate(ip, match, xl, numeric); > +} > + > +static int __multiport_xlate_v1(const void *ip, > + const struct xt_entry_match *match, > + struct xt_xlate *xl, int numeric) > +{ > + const struct xt_multiport_v1 *multiinfo > + = (const struct xt_multiport_v1 *)match->data; > + unsigned int i; > + > + switch (multiinfo->flags) { > + case XT_MULTIPORT_SOURCE: > + xt_xlate_add(xl, "sport "); > + break; > + case XT_MULTIPORT_DESTINATION: > + xt_xlate_add(xl, "dport "); > + break; > + case XT_MULTIPORT_EITHER: > + return 0; > + } same XT_MULTIPORT_EITHER here. > + > + if (multiinfo->invert) { > + xt_xlate_add(xl, "!= "); > + } else { > + if (multiinfo->count > 1) > + xt_xlate_add(xl, "{ "); > + } This if/else seems bogus. We only allow port sets if not inverting? > + > + for (i = 0; i < multiinfo->count; i++) { > + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); > + if (i && multiinfo->invert) > + return 0; This return here could mean that we build an incomplete nftables rule (ie, missing '}') > + if (multiinfo->pflags[i]) > + xt_xlate_add(xl, "-%u", multiinfo->ports[++i]); > + } > + > + if (multiinfo->count > 1 && !multiinfo->invert) > + xt_xlate_add(xl, "}"); > + > + xt_xlate_add(xl, " "); > + > + return 1; > +}
On Tue, May 31, 2016 at 12:08:57AM +0200, Arturo Borrero Gonzalez wrote: > On 30 May 2016 at 21:47, Laura Garcia Liebana <nevola@gmail.com> wrote: > > Add translation for multiport to nftables, which it's supported natively. > > > > Examples: > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80,81 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80,81} counter accept > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80:88 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80-88} counter accept > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport ! --dports 80:88 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport != 80-88 counter accept > > > > Lets clarify the syntax, this is valid: > > tcp dport 8000-8100 > tcp dport { 8000-8100} > tcp dport { 8000-8100, 9000-9100} > > but they mean different things. It seems we should avoid the braces {} > for the range case, otherwise we would be using a set with a single > element. > > However, > > tcp dport {8000,8100} <-- valid > tcp dport 8000,8100 <-- invalid > > So we should always use braces {} in the non-range case. > > Same seems to apply in the case of inversion. > > so we end with this combinations: > > tcp dport {x,x} > tcp dport != {x,x} > tcp dport x-x > tcp dport != x-x > > BTW, related to this, there seems to be a bug in nftables: > > % nft add rule t c tcp dport != {80, 81} > BUG: invalid expression type set > nft: evaluate.c:1463: expr_evaluate_relational: Assertion `0' failed. > Aborted This is not yet supported. This requires a small kernel patch to allow inversions in the nft_lookup.c. Then, the little extra code for libnftnl and nft. All tests for this usecase are disabled at the moment. -- 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 Tue, May 31, 2016 at 12:08:57AM +0200, Arturo Borrero Gonzalez wrote: > On 30 May 2016 at 21:47, Laura Garcia Liebana <nevola@gmail.com> wrote: > > Add translation for multiport to nftables, which it's supported natively. > > > > Examples: > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80,81 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80,81} counter accept > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80:88 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport { 80-88} counter accept > > > > $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport ! --dports 80:88 -j ACCEPT > > nft add rule ip filter INPUT ip protocol tcp tcp dport != 80-88 counter accept > > > > Lets clarify the syntax, this is valid: > > tcp dport 8000-8100 > tcp dport { 8000-8100} > tcp dport { 8000-8100, 9000-9100} > > but they mean different things. It seems we should avoid the braces {} > for the range case, otherwise we would be using a set with a single > element. > Yes, you're right. I'll change it in order to allow port ranges without {}. > However, > > tcp dport {8000,8100} <-- valid > tcp dport 8000,8100 <-- invalid > > So we should always use braces {} in the non-range case. > > Same seems to apply in the case of inversion. > > so we end with this combinations: > > tcp dport {x,x} > tcp dport != {x,x} This is not supported. > tcp dport x-x > tcp dport != x-x > > BTW, related to this, there seems to be a bug in nftables: > > % nft add rule t c tcp dport != {80, 81} > BUG: invalid expression type set > nft: evaluate.c:1463: expr_evaluate_relational: Assertion `0' failed. > Aborted > > > > Signed-off-by: Laura Garcia Liebana <nevola@gmail.com> > > --- > > Changes in v2: > > - Add curley brackets to lists and range of ports. > > > > extensions/libxt_multiport.c | 116 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 116 insertions(+) > > > > diff --git a/extensions/libxt_multiport.c b/extensions/libxt_multiport.c > > index 03af5a9..25b5589 100644 > > --- a/extensions/libxt_multiport.c > > +++ b/extensions/libxt_multiport.c > > @@ -468,6 +468,118 @@ static void multiport_save6_v1(const void *ip_void, > > __multiport_save_v1(match, ip->proto); > > } > > > > +static int __multiport_xlate(const void *ip, const struct xt_entry_match *match, > > + struct xt_xlate *xl, int numeric) > > +{ > > + const struct xt_multiport *multiinfo > > + = (const struct xt_multiport *)match->data; > > + unsigned int i; > > + > > + switch (multiinfo->flags) { > > + case XT_MULTIPORT_SOURCE: > > + xt_xlate_add(xl, "sport "); > > + break; > > + case XT_MULTIPORT_DESTINATION: > > + xt_xlate_add(xl, "dport "); > > + break; > > + case XT_MULTIPORT_EITHER: > > + return 0; > > + } > > this case XT_MULTIPORT_EITHER seems unsupported in nftables. > > Is there anything established to do in case we find an impossible > translation? print a warning or something? I don't know right now. > I guess we should avoid printing an invalid nftables rule as if the > translation was 100% ok (which is not true in this case). > It was agreed to return 0 if the translation is not supported for nftables. Currently, it does. > Just wondering, I should check myself because I don't know this right now. > > > > + > > + if (multiinfo->count > 1) > > + xt_xlate_add(xl, "{ "); > > + > > + for (i = 0; i < multiinfo->count; i++) > > + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); > > + > > + if (multiinfo->count > 1) > > + xt_xlate_add(xl, "}"); > > + > > + xt_xlate_add(xl, " "); > > + > > + return 1; > > +} > > + > > +static int multiport_xlate(const void *ip, const struct xt_entry_match *match, > > + struct xt_xlate *xl, int numeric) > > +{ > > + uint8_t proto = ((const struct ipt_ip *)ip)->proto; > > + > > + xt_xlate_add(xl, "%s ", proto_to_name(proto)); > > + return __multiport_xlate(ip, match, xl, numeric); > > +} > > + > > +static int multiport_xlate6(const void *ip, const struct xt_entry_match *match, > > + struct xt_xlate *xl, int numeric) > > +{ > > + uint8_t proto = ((const struct ip6t_ip6 *)ip)->proto; > > + > > + xt_xlate_add(xl, "%s ", proto_to_name(proto)); > > + return __multiport_xlate(ip, match, xl, numeric); > > +} > > + > > +static int __multiport_xlate_v1(const void *ip, > > + const struct xt_entry_match *match, > > + struct xt_xlate *xl, int numeric) > > +{ > > + const struct xt_multiport_v1 *multiinfo > > + = (const struct xt_multiport_v1 *)match->data; > > + unsigned int i; > > + > > + switch (multiinfo->flags) { > > + case XT_MULTIPORT_SOURCE: > > + xt_xlate_add(xl, "sport "); > > + break; > > + case XT_MULTIPORT_DESTINATION: > > + xt_xlate_add(xl, "dport "); > > + break; > > + case XT_MULTIPORT_EITHER: > > + return 0; > > + } > > same XT_MULTIPORT_EITHER here. > Same as before. > > + > > + if (multiinfo->invert) { > > + xt_xlate_add(xl, "!= "); > > + } else { > > + if (multiinfo->count > 1) > > + xt_xlate_add(xl, "{ "); > > + } > > This if/else seems bogus. We only allow port sets if not inverting? > This should be now changed as we've to accept port ranges without {}. > > + > > + for (i = 0; i < multiinfo->count; i++) { > > + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); > > + if (i && multiinfo->invert) > > + return 0; > > This return here could mean that we build an incomplete nftables rule > (ie, missing '}') > Such condition cares that there is no translation in nftables for: tcp dport != { 80,88 } So the closed } doesn't even matter. > > + if (multiinfo->pflags[i]) > > + xt_xlate_add(xl, "-%u", multiinfo->ports[++i]); > > + } > > + If the rule needs such closed } , then it's controlled here: > > + if (multiinfo->count > 1 && !multiinfo->invert) > > + xt_xlate_add(xl, "}"); > > + > > + xt_xlate_add(xl, " "); > > + > > + return 1; > > +} > > > -- > Arturo Borrero González -- 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 31 May 2016 at 09:22, Laura Garcia <nevola@gmail.com> wrote: >> > + >> > + for (i = 0; i < multiinfo->count; i++) { >> > + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); >> > + if (i && multiinfo->invert) >> > + return 0; >> >> This return here could mean that we build an incomplete nftables rule >> (ie, missing '}') >> > > Such condition cares that there is no translation in nftables for: > > tcp dport != { 80,88 } > > So the closed } doesn't even matter. > Then, at the start of the xlate function, I would suggest to do an early return if we are about to translate an unsupported rule. No need to keep checking if we are in the inversion case >> > + if (multiinfo->count > 1 && !multiinfo->invert) >> > + xt_xlate_add(xl, "}"); >> > + like here
On 31 May 2016 at 00:41, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > This is not yet supported. This requires a small kernel patch to allow > inversions in the nft_lookup.c. Then, the little extra code for > libnftnl and nft. > > All tests for this usecase are disabled at the moment. What is your idea of the implementation? Perhaps adding NFTA_LOOKUP_FLAGS and also: enum nft_lookup_flags { NFT_LOOKUP_F_INV = (1 << 0), };
On Tue, May 31, 2016 at 10:23:31AM +0200, Arturo Borrero Gonzalez wrote: > On 31 May 2016 at 00:41, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > This is not yet supported. This requires a small kernel patch to allow > > inversions in the nft_lookup.c. Then, the little extra code for > > libnftnl and nft. > > > > All tests for this usecase are disabled at the moment. > > What is your idea of the implementation? > > Perhaps adding NFTA_LOOKUP_FLAGS and also: > enum nft_lookup_flags { > NFT_LOOKUP_F_INV = (1 << 0), > }; Something like that, yes. -- 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/libxt_multiport.c b/extensions/libxt_multiport.c index 03af5a9..25b5589 100644 --- a/extensions/libxt_multiport.c +++ b/extensions/libxt_multiport.c @@ -468,6 +468,118 @@ static void multiport_save6_v1(const void *ip_void, __multiport_save_v1(match, ip->proto); } +static int __multiport_xlate(const void *ip, const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + const struct xt_multiport *multiinfo + = (const struct xt_multiport *)match->data; + unsigned int i; + + switch (multiinfo->flags) { + case XT_MULTIPORT_SOURCE: + xt_xlate_add(xl, "sport "); + break; + case XT_MULTIPORT_DESTINATION: + xt_xlate_add(xl, "dport "); + break; + case XT_MULTIPORT_EITHER: + return 0; + } + + if (multiinfo->count > 1) + xt_xlate_add(xl, "{ "); + + for (i = 0; i < multiinfo->count; i++) + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); + + if (multiinfo->count > 1) + xt_xlate_add(xl, "}"); + + xt_xlate_add(xl, " "); + + return 1; +} + +static int multiport_xlate(const void *ip, const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + uint8_t proto = ((const struct ipt_ip *)ip)->proto; + + xt_xlate_add(xl, "%s ", proto_to_name(proto)); + return __multiport_xlate(ip, match, xl, numeric); +} + +static int multiport_xlate6(const void *ip, const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + uint8_t proto = ((const struct ip6t_ip6 *)ip)->proto; + + xt_xlate_add(xl, "%s ", proto_to_name(proto)); + return __multiport_xlate(ip, match, xl, numeric); +} + +static int __multiport_xlate_v1(const void *ip, + const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + const struct xt_multiport_v1 *multiinfo + = (const struct xt_multiport_v1 *)match->data; + unsigned int i; + + switch (multiinfo->flags) { + case XT_MULTIPORT_SOURCE: + xt_xlate_add(xl, "sport "); + break; + case XT_MULTIPORT_DESTINATION: + xt_xlate_add(xl, "dport "); + break; + case XT_MULTIPORT_EITHER: + return 0; + } + + if (multiinfo->invert) { + xt_xlate_add(xl, "!= "); + } else { + if (multiinfo->count > 1) + xt_xlate_add(xl, "{ "); + } + + for (i = 0; i < multiinfo->count; i++) { + xt_xlate_add(xl, "%s%u", i ? "," : "", multiinfo->ports[i]); + if (i && multiinfo->invert) + return 0; + if (multiinfo->pflags[i]) + xt_xlate_add(xl, "-%u", multiinfo->ports[++i]); + } + + if (multiinfo->count > 1 && !multiinfo->invert) + xt_xlate_add(xl, "}"); + + xt_xlate_add(xl, " "); + + return 1; +} + +static int multiport_xlate_v1(const void *ip, + const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + uint8_t proto = ((const struct ipt_ip *)ip)->proto; + + xt_xlate_add(xl, "%s ", proto_to_name(proto)); + return __multiport_xlate_v1(ip, match, xl, numeric); +} + +static int multiport_xlate6_v1(const void *ip, + const struct xt_entry_match *match, + struct xt_xlate *xl, int numeric) +{ + uint8_t proto = ((const struct ip6t_ip6 *)ip)->proto; + + xt_xlate_add(xl, "%s ", proto_to_name(proto)); + return __multiport_xlate_v1(ip, match, xl, numeric); +} + static struct xtables_match multiport_mt_reg[] = { { .family = NFPROTO_IPV4, @@ -482,6 +594,7 @@ static struct xtables_match multiport_mt_reg[] = { .print = multiport_print, .save = multiport_save, .x6_options = multiport_opts, + .xlate = multiport_xlate, }, { .family = NFPROTO_IPV6, @@ -496,6 +609,7 @@ static struct xtables_match multiport_mt_reg[] = { .print = multiport_print6, .save = multiport_save6, .x6_options = multiport_opts, + .xlate = multiport_xlate6, }, { .family = NFPROTO_IPV4, @@ -510,6 +624,7 @@ static struct xtables_match multiport_mt_reg[] = { .print = multiport_print_v1, .save = multiport_save_v1, .x6_options = multiport_opts, + .xlate = multiport_xlate_v1, }, { .family = NFPROTO_IPV6, @@ -524,6 +639,7 @@ static struct xtables_match multiport_mt_reg[] = { .print = multiport_print6_v1, .save = multiport_save6_v1, .x6_options = multiport_opts, + .xlate = multiport_xlate6_v1, }, };
Add translation for multiport to nftables, which it's supported natively. Examples: $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80,81 -j ACCEPT nft add rule ip filter INPUT ip protocol tcp tcp dport { 80,81} counter accept $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport --dports 80:88 -j ACCEPT nft add rule ip filter INPUT ip protocol tcp tcp dport { 80-88} counter accept $ sudo iptables-translate -t filter -A INPUT -p tcp -m multiport ! --dports 80:88 -j ACCEPT nft add rule ip filter INPUT ip protocol tcp tcp dport != 80-88 counter accept Signed-off-by: Laura Garcia Liebana <nevola@gmail.com> --- Changes in v2: - Add curley brackets to lists and range of ports. extensions/libxt_multiport.c | 116 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+)