[PATCH next] iptables: on revision mismatch, do not call print/save

[PATCH next] iptables: on revision mismatch, do not call print/save

[Willem de Bruijn]

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(-)

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

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Pablo Neira Ayuso]

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.

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Jozsef Kadlecsik]

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

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Willem de Bruijn]

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.

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Willem de Bruijn]

>> 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.

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Willem de Bruijn]

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.

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Jozsef Kadlecsik]

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

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Willem de Bruijn]

>> > 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.

Re: [PATCH next] iptables: on revision mismatch, do not call print/save

[Jozsef Kadlecsik]

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