| Message ID | 20141117123622.11349.43033.stgit@nfdev.cica.es |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Mon, Nov 17, 2014 at 01:36:23PM +0100, Arturo Borrero Gonzalez wrote: > The ebtables-compat tool doesn't support default policy in custom chains. > RETURN is the default policy in this case, and is mandatory (this is the > behaviour of nf_tables). > > While at it, fix the error message when trying to change the default policy to > RETURN in builtin chains to match the original ebtables message. > > Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> > --- > iptables/nft-bridge.c | 7 ++++++- > iptables/xtables-eb.c | 19 ++++++++++++------- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c > index b5aec00..dc26bfd 100644 > --- a/iptables/nft-bridge.c > +++ b/iptables/nft-bridge.c > @@ -356,7 +356,12 @@ static void nft_bridge_print_header(unsigned int format, const char *chain, > const struct xt_counters *counters, > bool basechain, uint32_t refs) > { > - printf("Bridge chain: %s, entries: %u, policy: %s\n", chain, refs, pol); > + if (basechain) > + printf("Bridge chain: %s, entries: %u, policy: %s\n", > + chain, refs, pol); > + else > + printf("Bridge chain: %s, entries: %u, policy: RETURN\n", > + chain, refs); > } I'd suggest: printf("Bridge chain: %s, entries: %u, policy: %s\n", chain, refs, basechain ? pol : "RETURN"); > static void nft_bridge_print_firewall(struct nft_rule *r, unsigned int num, > diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c > index 917bca2..bf9f264 100644 > --- a/iptables/xtables-eb.c > +++ b/iptables/xtables-eb.c > @@ -615,11 +615,10 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table) > case 'N': /* Make a user defined chain */ > case 'E': /* Rename chain */ > case 'X': /* Delete chain */ > - /* We allow -N chainname -P policy */ Better extend this comment: /* We allow -N chainname -P policy */ /* XXX: Not in ebtables-compat */ > if (command == 'N' && c == 'P') { > - command = c; > - optind--; /* No table specified */ > - goto handle_P; > + xtables_error(PARAMETER_PROBLEM, > + "The default policy in user-defined" > + " chains is RETURN (mandatory)"); > } I'd really prefer to avoid changes in the ebtables parser. Could you perform this checking from later on, when validating the configuration. if (strcmp(chain, "INPUT") != 0 && strcmp(chain, "FORWARD") != 0 && ... && strcmp(policy, "RETURN") != 0) xtables_error("...); > if (OPT_COMMANDS) > xtables_error(PARAMETER_PROBLEM, > @@ -663,7 +662,6 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table) > optind++; > } > } else if (c == 'P') { > -handle_P: > if (optind >= argc) > xtables_error(PARAMETER_PROBLEM, > "No policy specified"); > @@ -1146,9 +1144,16 @@ check_extension: */ > cs.fw.ethproto = htons(cs.fw.ethproto); > > if (command == 'P') { > - if (selected_chain < NF_BR_NUMHOOKS && strcmp(policy, "RETURN")==0) > + if (selected_chain < 0) { > xtables_error(PARAMETER_PROBLEM, > - "Policy RETURN only allowed for user defined chains"); > + "Default policy in user-defined chains " > + "is mandatory RETURN"); > + } > + if (strcmp(policy, "RETURN") == 0) { > + xtables_error(PARAMETER_PROBLEM, > + "Policy RETURN only allowed for user " > + "defined chains"); > + } Not sure why you need this change. -- 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 18 November 2014 19:43, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> if (command == 'N' && c == 'P') { >> - command = c; >> - optind--; /* No table specified */ >> - goto handle_P; >> + xtables_error(PARAMETER_PROBLEM, >> + "The default policy in user-defined" >> + " chains is RETURN (mandatory)"); >> } > > I'd really prefer to avoid changes in the ebtables parser. Could you > perform this checking from later on, when validating the > configuration. > > if (strcmp(chain, "INPUT") != 0 && > strcmp(chain, "FORWARD") != 0 && > ... && > strcmp(policy, "RETURN") != 0) > xtables_error("...); > In the user-defined chain creation path, the code jumps to several places (the goto handle_P; thing). I don't see clearly a zone where to put these validations. Could you please point to where would you like to put this code? regards.
On Wed, Nov 19, 2014 at 01:23:01PM +0100, Arturo Borrero Gonzalez wrote: > On 18 November 2014 19:43, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > >> if (command == 'N' && c == 'P') { > >> - command = c; > >> - optind--; /* No table specified */ > >> - goto handle_P; > >> + xtables_error(PARAMETER_PROBLEM, > >> + "The default policy in user-defined" > >> + " chains is RETURN (mandatory)"); > >> } > > > > I'd really prefer to avoid changes in the ebtables parser. Could you > > perform this checking from later on, when validating the > > configuration. > > > > if (strcmp(chain, "INPUT") != 0 && > > strcmp(chain, "FORWARD") != 0 && > > ... && > > strcmp(policy, "RETURN") != 0) > > xtables_error("...); > > > > In the user-defined chain creation path, the code jumps to several > places (the goto handle_P; thing). I don't see clearly a zone where to > put these validations. > > Could you please point to where would you like to put this code? This will leave the while ((c = getopt_long ...) loop at some point, right? So perform the validation after the parsing is done. -- 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/iptables/nft-bridge.c b/iptables/nft-bridge.c index b5aec00..dc26bfd 100644 --- a/iptables/nft-bridge.c +++ b/iptables/nft-bridge.c @@ -356,7 +356,12 @@ static void nft_bridge_print_header(unsigned int format, const char *chain, const struct xt_counters *counters, bool basechain, uint32_t refs) { - printf("Bridge chain: %s, entries: %u, policy: %s\n", chain, refs, pol); + if (basechain) + printf("Bridge chain: %s, entries: %u, policy: %s\n", + chain, refs, pol); + else + printf("Bridge chain: %s, entries: %u, policy: RETURN\n", + chain, refs); } static void nft_bridge_print_firewall(struct nft_rule *r, unsigned int num, diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c index 917bca2..bf9f264 100644 --- a/iptables/xtables-eb.c +++ b/iptables/xtables-eb.c @@ -615,11 +615,10 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table) case 'N': /* Make a user defined chain */ case 'E': /* Rename chain */ case 'X': /* Delete chain */ - /* We allow -N chainname -P policy */ if (command == 'N' && c == 'P') { - command = c; - optind--; /* No table specified */ - goto handle_P; + xtables_error(PARAMETER_PROBLEM, + "The default policy in user-defined" + " chains is RETURN (mandatory)"); } if (OPT_COMMANDS) xtables_error(PARAMETER_PROBLEM, @@ -663,7 +662,6 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table) optind++; } } else if (c == 'P') { -handle_P: if (optind >= argc) xtables_error(PARAMETER_PROBLEM, "No policy specified"); @@ -1146,9 +1144,16 @@ check_extension: */ cs.fw.ethproto = htons(cs.fw.ethproto); if (command == 'P') { - if (selected_chain < NF_BR_NUMHOOKS && strcmp(policy, "RETURN")==0) + if (selected_chain < 0) { xtables_error(PARAMETER_PROBLEM, - "Policy RETURN only allowed for user defined chains"); + "Default policy in user-defined chains " + "is mandatory RETURN"); + } + if (strcmp(policy, "RETURN") == 0) { + xtables_error(PARAMETER_PROBLEM, + "Policy RETURN only allowed for user " + "defined chains"); + } ret = nft_chain_set(h, *table, chain, policy, NULL); if (ret < 0) xtables_error(PARAMETER_PROBLEM, "Wrong policy");
The ebtables-compat tool doesn't support default policy in custom chains. RETURN is the default policy in this case, and is mandatory (this is the behaviour of nf_tables). While at it, fix the error message when trying to change the default policy to RETURN in builtin chains to match the original ebtables message. Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> --- iptables/nft-bridge.c | 7 ++++++- iptables/xtables-eb.c | 19 ++++++++++++------- 2 files changed, 18 insertions(+), 8 deletions(-) -- 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