| Message ID | 1481235401-46043-1-git-send-email-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Thu, Dec 08, 2016 at 05:16:41PM -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Between revisions, the layout of xtables data may change completely. > Do not interpret the data in a revision M with a module of revision N. Applied, thanks Willem. -- 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
Hi, On Thu, 8 Dec 2016, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Between revisions, the layout of xtables data may change completely. > Do not interpret the data in a revision M with a module of revision N. > > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > iptables/ip6tables.c | 18 ++++++++++++++---- > iptables/iptables.c | 18 ++++++++++++++---- > 2 files changed, 28 insertions(+), 8 deletions(-) The patch breaks backward/forward compatibility in a match/target. When the list of the revisions of a given match/target of iptables is not exactly the same as for the kernel counter part (when the kernel module supports less revisions than iptables), then in spite of the supported match/target, " [unsupported revision]" is printed instead of the arguments. See https://bugzilla.netfilter.org/show_bug.cgi?id=1147. Please consider reverting the patch. Or we should not stop in xtables_find_match/xtables_find_target at revision checking when the revision does not match, until all possibilities is not exhausted. Best regards, Jozsef > diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c > index c8d34e2..0d09181 100644 > --- a/iptables/ip6tables.c > +++ b/iptables/ip6tables.c > @@ -76,6 +76,8 @@ static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z', > static const char optflags[] > = { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c'}; > > +static const char unsupported_rev[] = " [unsupported revision]"; > + > static struct option original_opts[] = { > {.name = "append", .has_arg = 1, .val = 'A'}, > {.name = "delete", .has_arg = 1, .val = 'D'}, > @@ -487,8 +489,10 @@ print_match(const struct xt_entry_match *m, > xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); > > if (match) { > - if (match->print) > + if (match->print && m->u.user.revision == match->revision) > match->print(ip, m, numeric); > + else if (match->print) > + printf("%s%s ", match->name, unsupported_rev); > else > printf("%s ", match->name); > } else { > @@ -614,9 +618,11 @@ print_firewall(const struct ip6t_entry *fw, > IP6T_MATCH_ITERATE(fw, print_match, &fw->ipv6, format & FMT_NUMERIC); > > if (target) { > - if (target->print) > + if (target->print && t->u.user.revision == target->revision) > /* Print the target information. */ > target->print(&fw->ipv6, t, format & FMT_NUMERIC); > + else if (target->print) > + printf(" %s%s", target->name, unsupported_rev); > } else if (t->u.target_size != sizeof(*t)) > printf("[%u bytes of unknown target data] ", > (unsigned int)(t->u.target_size - sizeof(*t))); > @@ -1004,8 +1010,10 @@ static int print_match_save(const struct xt_entry_match *e, > match->alias ? match->alias(e) : e->u.user.name); > > /* some matches don't provide a save function */ > - if (match->save) > + if (match->save && e->u.user.revision == match->revision) > match->save(ip, e); > + else if (match->save) > + printf(unsupported_rev); > } else { > if (e->u.match_size) { > fprintf(stderr, > @@ -1104,8 +1112,10 @@ void print_rule6(const struct ip6t_entry *e, > } > > printf(" -j %s", target->alias ? target->alias(t) : target_name); > - if (target->save) > + if (target->save && t->u.user.revision == target->revision) > target->save(&e->ipv6, t); > + else if (target->save) > + printf(unsupported_rev); > else { > /* If the target size is greater than xt_entry_target > * there is something to be saved, we just don't know > diff --git a/iptables/iptables.c b/iptables/iptables.c > index 79fa37b..1bdde27 100644 > --- a/iptables/iptables.c > +++ b/iptables/iptables.c > @@ -73,6 +73,8 @@ static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z', > static const char optflags[] > = { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c', 'f'}; > > +static const char unsupported_rev[] = " [unsupported revision]"; > + > static struct option original_opts[] = { > {.name = "append", .has_arg = 1, .val = 'A'}, > {.name = "delete", .has_arg = 1, .val = 'D'}, > @@ -472,8 +474,10 @@ print_match(const struct xt_entry_match *m, > xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); > > if (match) { > - if (match->print) > + if (match->print && m->u.user.revision == match->revision) > match->print(ip, m, numeric); > + else if (match->print) > + printf("%s%s ", match->name, unsupported_rev); > else > printf("%s ", match->name); > } else { > @@ -599,9 +603,11 @@ print_firewall(const struct ipt_entry *fw, > IPT_MATCH_ITERATE(fw, print_match, &fw->ip, format & FMT_NUMERIC); > > if (target) { > - if (target->print) > + if (target->print && t->u.user.revision == target->revision) > /* Print the target information. */ > target->print(&fw->ip, t, format & FMT_NUMERIC); > + else if (target->print) > + printf(" %s%s", target->name, unsupported_rev); > } else if (t->u.target_size != sizeof(*t)) > printf("[%u bytes of unknown target data] ", > (unsigned int)(t->u.target_size - sizeof(*t))); > @@ -995,8 +1001,10 @@ static int print_match_save(const struct xt_entry_match *e, > match->alias ? match->alias(e) : e->u.user.name); > > /* some matches don't provide a save function */ > - if (match->save) > + if (match->save && e->u.user.revision == match->revision) > match->save(ip, e); > + else if (match->save) > + printf(unsupported_rev); > } else { > if (e->u.match_size) { > fprintf(stderr, > @@ -1095,8 +1103,10 @@ void print_rule4(const struct ipt_entry *e, > } > > printf(" -j %s", target->alias ? target->alias(t) : target_name); > - if (target->save) > + if (target->save && t->u.user.revision == target->revision) > target->save(&e->ip, t); > + else if (target->save) > + printf(unsupported_rev); > else { > /* If the target size is greater than xt_entry_target > * there is something to be saved, we just don't know > -- > 2.8.0.rc3.226.g39d4020 > > -- > 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 > - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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, Apr 21, 2017 at 4:15 PM, Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > Hi, > > On Thu, 8 Dec 2016, Willem de Bruijn wrote: > >> From: Willem de Bruijn <willemb@google.com> >> >> Between revisions, the layout of xtables data may change completely. >> Do not interpret the data in a revision M with a module of revision N. >> >> Signed-off-by: Willem de Bruijn <willemb@google.com> >> --- >> iptables/ip6tables.c | 18 ++++++++++++++---- >> iptables/iptables.c | 18 ++++++++++++++---- >> 2 files changed, 28 insertions(+), 8 deletions(-) > > The patch breaks backward/forward compatibility in a match/target. > > When the list of the revisions of a given match/target of iptables is not > exactly the same as for the kernel counter part (when the kernel module > supports less revisions than iptables), then in spite of the supported > match/target, " [unsupported revision]" is printed instead of the > arguments. See https://bugzilla.netfilter.org/show_bug.cgi?id=1147. Thanks for the report. > Please consider reverting the patch. Or we should not stop in > xtables_find_match/xtables_find_target at revision checking when the > revision does not match, until all possibilities is not exhausted. This seems like the better solution to me. The patch fixes a real issue where garbage is printed by misinterpreting struct fields. Iptables should try to lookup the matching revision for a match or target, instead of returning the first one. I'll take a look. -- 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
>> The patch breaks backward/forward compatibility in a match/target. >> >> When the list of the revisions of a given match/target of iptables is not >> exactly the same as for the kernel counter part (when the kernel module >> supports less revisions than iptables), then in spite of the supported >> match/target, " [unsupported revision]" is printed instead of the >> arguments. See https://bugzilla.netfilter.org/show_bug.cgi?id=1147. > > Thanks for the report. > >> Please consider reverting the patch. Or we should not stop in >> xtables_find_match/xtables_find_target at revision checking when the >> revision does not match, until all possibilities is not exhausted. > > This seems like the better solution to me. The patch fixes a real issue > where garbage is printed by misinterpreting struct fields. Iptables should > try to lookup the matching revision for a match or target, instead of > returning the first one. I'll take a look. I have a draft patch, but am so far unable to reproduce the problem. It may be more subtle than what you describe. xtables_find_match can call xtables_fully_register_pending_match which calls compatible_match_revision to decide whether a match revision is supported and, if multiple revisions are supported, which to prefer. A simple setup where I add a new revision 2 of xt_mark to iptables does not cause this problem. iptables correctly negotiates with the kernel that the highest supported revision is 1. Initially iptables tries to register rev 2. This succeeds. Then, it tries to register rev 1. In xtables_fully_register_pending_match this triggers branch if (old). The existing entry rev 2 is tested for compatibility with the kernel in compatible_match_revision, which fails. Rev 2 is then deleted and rev 1 inserted. Finally, it tries to register rev 0. The branch is again entered, but now the call to compatible_match_revision for rev 1 will succeed. As this has a higher score, it is kept. As a result, when logging a rule with iptables -L, the binary will correctly identify the match of the highest level that the kernel supports and prints that. It does not rev 2, so does not hit the branch to print "[unsupported revision]" in this case. -- 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, Apr 26, 2017 at 5:15 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: >>> The patch breaks backward/forward compatibility in a match/target. >>> >>> When the list of the revisions of a given match/target of iptables is not >>> exactly the same as for the kernel counter part (when the kernel module >>> supports less revisions than iptables), then in spite of the supported >>> match/target, " [unsupported revision]" is printed instead of the >>> arguments. See https://bugzilla.netfilter.org/show_bug.cgi?id=1147. >> >> Thanks for the report. >> >>> Please consider reverting the patch. Or we should not stop in >>> xtables_find_match/xtables_find_target at revision checking when the >>> revision does not match, until all possibilities is not exhausted. >> >> This seems like the better solution to me. The patch fixes a real issue >> where garbage is printed by misinterpreting struct fields. Iptables should >> try to lookup the matching revision for a match or target, instead of >> returning the first one. I'll take a look. > > I have a draft patch, but am so far unable to reproduce the problem. > > It may be more subtle than what you describe. xtables_find_match > can call xtables_fully_register_pending_match which calls > compatible_match_revision to decide whether a match revision > is supported and, if multiple revisions are supported, which to prefer. The case reported in bugzilla is for match 'set', which has not had a new revision since v4 in 2014. Building the exact versions of iptables and ipset reported there and disabling v4 support in the kernel correctly reverts to using v3. -- 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
Hi Willem, On Wed, 26 Apr 2017, Willem de Bruijn wrote: > On Wed, Apr 26, 2017 at 5:15 PM, Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > >>> The patch breaks backward/forward compatibility in a match/target. > >>> > >>> When the list of the revisions of a given match/target of iptables is not > >>> exactly the same as for the kernel counter part (when the kernel module > >>> supports less revisions than iptables), then in spite of the supported > >>> match/target, " [unsupported revision]" is printed instead of the > >>> arguments. See https://bugzilla.netfilter.org/show_bug.cgi?id=1147. > >> > >> Thanks for the report. > >> > >>> Please consider reverting the patch. Or we should not stop in > >>> xtables_find_match/xtables_find_target at revision checking when the > >>> revision does not match, until all possibilities is not exhausted. > >> > >> This seems like the better solution to me. The patch fixes a real issue > >> where garbage is printed by misinterpreting struct fields. Iptables should > >> try to lookup the matching revision for a match or target, instead of > >> returning the first one. I'll take a look. > > > > I have a draft patch, but am so far unable to reproduce the problem. > > > > It may be more subtle than what you describe. xtables_find_match > > can call xtables_fully_register_pending_match which calls > > compatible_match_revision to decide whether a match revision > > is supported and, if multiple revisions are supported, which to prefer. > > The case reported in bugzilla is for match 'set', which has not had a > new revision since v4 in 2014. Building the exact versions of iptables > and ipset reported there and disabling v4 support in the kernel correctly > reverts to using v3. Maybe the case can be reproduced with the following steps, but I'm guessing: - rules inserted - iptables binary downgraded/upgraded - rules listed Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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
>> > It may be more subtle than what you describe. xtables_find_match >> > can call xtables_fully_register_pending_match which calls >> > compatible_match_revision to decide whether a match revision >> > is supported and, if multiple revisions are supported, which to prefer. >> >> The case reported in bugzilla is for match 'set', which has not had a >> new revision since v4 in 2014. Building the exact versions of iptables >> and ipset reported there and disabling v4 support in the kernel correctly >> reverts to using v3. > > Maybe the case can be reproduced with the following steps, but I'm > guessing: > > - rules inserted > - iptables binary downgraded/upgraded > - rules listed It will. This is largely what the patch protects against. But perhaps it currently unnecessarily restricts binaries that support both revisions N and N + 1 from printing structs of rev N, if those are inserted from another binary that only supported up to N. Let me try that. Before this patch, a rule inserted with a binary that only supports rev N for a given match, but read with another binary that supports rev N+1 will result in the new binary incorrectly parsing a struct of rev N as one of rev N + 1. And vice versa. See also the discussion in Re: [PATCH nf-next] netfilter: xt_bpf: support ebpf https://www.spinics.net/lists/netfilter-devel/msg45357.html Casting structs in this way is unsafe. The downgrade path cannot be fixed. But the upgrade path should. The way iptables negotiates with the kernel and only loads the highest revision for any given match or target may make this hard to 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 Thu, 27 Apr 2017, Willem de Bruijn wrote: > > Maybe the case can be reproduced with the following steps, but I'm > > guessing: > > > > - rules inserted > > - iptables binary downgraded/upgraded > > - rules listed > > It will. This is largely what the patch protects against. But perhaps > it currently unnecessarily restricts binaries that support both > revisions N and N + 1 from printing structs of rev N, if those > are inserted from another binary that only supported up to N. Let > me try that. > > Before this patch, a rule inserted with a binary that only supports > rev N for a given match, but read with another binary that supports > rev N+1 will result in the new binary incorrectly parsing a struct of > rev N as one of rev N + 1. And vice versa. See also the discussion in > > Re: [PATCH nf-next] netfilter: xt_bpf: support ebpf > https://www.spinics.net/lists/netfilter-devel/msg45357.html > > Casting structs in this way is unsafe. The downgrade path cannot > be fixed. But the upgrade path should. The way iptables negotiates > with the kernel and only loads the highest revision for any given > match or target may make this hard to change. The downgrade case is OK and it is all right that iptables won't try to interpret the blob of the not supported revision and prints " [unsupported revision]" instead. But in the upgrade case iptables is able to print the target/match so it should do so. Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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/ip6tables.c b/iptables/ip6tables.c index c8d34e2..0d09181 100644 --- a/iptables/ip6tables.c +++ b/iptables/ip6tables.c @@ -76,6 +76,8 @@ static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z', static const char optflags[] = { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c'}; +static const char unsupported_rev[] = " [unsupported revision]"; + static struct option original_opts[] = { {.name = "append", .has_arg = 1, .val = 'A'}, {.name = "delete", .has_arg = 1, .val = 'D'}, @@ -487,8 +489,10 @@ print_match(const struct xt_entry_match *m, xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); if (match) { - if (match->print) + if (match->print && m->u.user.revision == match->revision) match->print(ip, m, numeric); + else if (match->print) + printf("%s%s ", match->name, unsupported_rev); else printf("%s ", match->name); } else { @@ -614,9 +618,11 @@ print_firewall(const struct ip6t_entry *fw, IP6T_MATCH_ITERATE(fw, print_match, &fw->ipv6, format & FMT_NUMERIC); if (target) { - if (target->print) + if (target->print && t->u.user.revision == target->revision) /* Print the target information. */ target->print(&fw->ipv6, t, format & FMT_NUMERIC); + else if (target->print) + printf(" %s%s", target->name, unsupported_rev); } else if (t->u.target_size != sizeof(*t)) printf("[%u bytes of unknown target data] ", (unsigned int)(t->u.target_size - sizeof(*t))); @@ -1004,8 +1010,10 @@ static int print_match_save(const struct xt_entry_match *e, match->alias ? match->alias(e) : e->u.user.name); /* some matches don't provide a save function */ - if (match->save) + if (match->save && e->u.user.revision == match->revision) match->save(ip, e); + else if (match->save) + printf(unsupported_rev); } else { if (e->u.match_size) { fprintf(stderr, @@ -1104,8 +1112,10 @@ void print_rule6(const struct ip6t_entry *e, } printf(" -j %s", target->alias ? target->alias(t) : target_name); - if (target->save) + if (target->save && t->u.user.revision == target->revision) target->save(&e->ipv6, t); + else if (target->save) + printf(unsupported_rev); else { /* If the target size is greater than xt_entry_target * there is something to be saved, we just don't know diff --git a/iptables/iptables.c b/iptables/iptables.c index 79fa37b..1bdde27 100644 --- a/iptables/iptables.c +++ b/iptables/iptables.c @@ -73,6 +73,8 @@ static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z', static const char optflags[] = { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c', 'f'}; +static const char unsupported_rev[] = " [unsupported revision]"; + static struct option original_opts[] = { {.name = "append", .has_arg = 1, .val = 'A'}, {.name = "delete", .has_arg = 1, .val = 'D'}, @@ -472,8 +474,10 @@ print_match(const struct xt_entry_match *m, xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); if (match) { - if (match->print) + if (match->print && m->u.user.revision == match->revision) match->print(ip, m, numeric); + else if (match->print) + printf("%s%s ", match->name, unsupported_rev); else printf("%s ", match->name); } else { @@ -599,9 +603,11 @@ print_firewall(const struct ipt_entry *fw, IPT_MATCH_ITERATE(fw, print_match, &fw->ip, format & FMT_NUMERIC); if (target) { - if (target->print) + if (target->print && t->u.user.revision == target->revision) /* Print the target information. */ target->print(&fw->ip, t, format & FMT_NUMERIC); + else if (target->print) + printf(" %s%s", target->name, unsupported_rev); } else if (t->u.target_size != sizeof(*t)) printf("[%u bytes of unknown target data] ", (unsigned int)(t->u.target_size - sizeof(*t))); @@ -995,8 +1001,10 @@ static int print_match_save(const struct xt_entry_match *e, match->alias ? match->alias(e) : e->u.user.name); /* some matches don't provide a save function */ - if (match->save) + if (match->save && e->u.user.revision == match->revision) match->save(ip, e); + else if (match->save) + printf(unsupported_rev); } else { if (e->u.match_size) { fprintf(stderr, @@ -1095,8 +1103,10 @@ void print_rule4(const struct ipt_entry *e, } printf(" -j %s", target->alias ? target->alias(t) : target_name); - if (target->save) + if (target->save && t->u.user.revision == target->revision) target->save(&e->ip, t); + else if (target->save) + printf(unsupported_rev); else { /* If the target size is greater than xt_entry_target * there is something to be saved, we just don't know