[iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

While translating a conntrack state match in old syntax, matches are
looked up by name, only. This returned the revision 0 entry since
matches are registered in reverse order of appearance in the array
passed to xtables_register_matches(). The problem is that revision 0
doesn't define an xlate callback.

Fix this by reordering the matches in conntrack_mt_reg so that the
highest revision one is found first.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
The strange thing here is that I'm pretty sure this has been working
once. My logs from playing with iptables-restore-translate from November
2016 indicate that. Yet I have not been able to find a point in iptables
git history in which it works.
---
 extensions/libxt_conntrack.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/extensions/libxt_conntrack.c b/extensions/libxt_conntrack.c
index 72c522004a7ea..60ce9d1dc0a2e 100644
--- a/extensions/libxt_conntrack.c
+++ b/extensions/libxt_conntrack.c
@@ -1507,6 +1507,19 @@ static struct xtables_match conntrack_mt_reg[] = {
 	{
 		.family        = NFPROTO_UNSPEC,
 		.name          = "state",
+		.revision      = 0,
+		.version       = XTABLES_VERSION,
+		.size          = XT_ALIGN(sizeof(struct xt_state_info)),
+		.userspacesize = XT_ALIGN(sizeof(struct xt_state_info)),
+		.help          = state_help,
+		.print         = state_print,
+		.save          = state_save,
+		.x6_parse      = state_parse,
+		.x6_options    = state_opts,
+	},
+	{
+		.family        = NFPROTO_UNSPEC,
+		.name          = "state",
 		.real_name     = "conntrack",
 		.revision      = 1,
 		.ext_flags     = XTABLES_EXT_ALIAS,
@@ -1550,19 +1563,6 @@ static struct xtables_match conntrack_mt_reg[] = {
 		.x6_options    = state_opts,
 		.xlate         = state_xlate,
 	},
-	{
-		.family        = NFPROTO_UNSPEC,
-		.name          = "state",
-		.revision      = 0,
-		.version       = XTABLES_VERSION,
-		.size          = XT_ALIGN(sizeof(struct xt_state_info)),
-		.userspacesize = XT_ALIGN(sizeof(struct xt_state_info)),
-		.help          = state_help,
-		.print         = state_print,
-		.save          = state_save,
-		.x6_parse      = state_parse,
-		.x6_options    = state_opts,
-	},
 };
 
 void _init(void)
-- 
2.11.0

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Pablo Neira Ayuso]

On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> While translating a conntrack state match in old syntax, matches are
> looked up by name, only. This returned the revision 0 entry since
> matches are registered in reverse order of appearance in the array
> passed to xtables_register_matches(). The problem is that revision 0
> doesn't define an xlate callback.
> 
> Fix this by reordering the matches in conntrack_mt_reg so that the
> highest revision one is found first.

Applied, thanks Phil.

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Pablo Neira Ayuso]

On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > While translating a conntrack state match in old syntax, matches are
> > looked up by name, only. This returned the revision 0 entry since
> > matches are registered in reverse order of appearance in the array
> > passed to xtables_register_matches(). The problem is that revision 0
> > doesn't define an xlate callback.
> > 
> > Fix this by reordering the matches in conntrack_mt_reg so that the
> > highest revision one is found first.
> 
> Applied, thanks Phil.

Wait.

Do you mean this case?

# iptables-translate -I INPUT -m state --state NEW
nft insert rule ip filter INPUT ct state new  counter

Hm, this works here.

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > While translating a conntrack state match in old syntax, matches are
> > > looked up by name, only. This returned the revision 0 entry since
> > > matches are registered in reverse order of appearance in the array
> > > passed to xtables_register_matches(). The problem is that revision 0
> > > doesn't define an xlate callback.
> > > 
> > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > highest revision one is found first.
> > 
> > Applied, thanks Phil.
> 
> Wait.
> 
> Do you mean this case?
> 
> # iptables-translate -I INPUT -m state --state NEW
> nft insert rule ip filter INPUT ct state new  counter
> 
> Hm, this works here.

Yes, that's it. And it working for you just emphasizes something's fishy
here. I just reverted my patch, then it's like this:

| $ ./configure --prefix=$PWD/install && make && make install
| [...]
| $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
| nft # -I INPUT -m state --state NEW

Using 'strace -eopen' I see that libxt_state.so from the install
destination is used, not the system one.

Also, I did this change for debugging:

| --- a/iptables/xtables-translate.c
| +++ b/iptables/xtables-translate.c
| @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
|                         .escape_quotes  = !cs->restore,
|                 };
|  
| +               printf("found match %s, rev %u, xlate is %p\n",
| +                               matchp->match->name,
| +                               matchp->match->revision,
| +                               matchp->match->xlate);
|                 if (!matchp->match->xlate)
|                         return 0;

The output is then:

| $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
| nft found match state, rev 0, xlate is (nil)
| # -I INPUT -m state --state NEW

Am I missing something??

Thanks, Phil

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Pablo Neira Ayuso]

On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > While translating a conntrack state match in old syntax, matches are
> > > > looked up by name, only. This returned the revision 0 entry since
> > > > matches are registered in reverse order of appearance in the array
> > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > doesn't define an xlate callback.
> > > > 
> > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > highest revision one is found first.
> > > 
> > > Applied, thanks Phil.
> > 
> > Wait.
> > 
> > Do you mean this case?
> > 
> > # iptables-translate -I INPUT -m state --state NEW
> > nft insert rule ip filter INPUT ct state new  counter
> > 
> > Hm, this works here.
> 
> Yes, that's it. And it working for you just emphasizes something's fishy
> here. I just reverted my patch, then it's like this:
> 
> | $ ./configure --prefix=$PWD/install && make && make install
> | [...]
> | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> | nft # -I INPUT -m state --state NEW
> 
> Using 'strace -eopen' I see that libxt_state.so from the install
> destination is used, not the system one.
> 
> Also, I did this change for debugging:
> 
> | --- a/iptables/xtables-translate.c
> | +++ b/iptables/xtables-translate.c
> | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> |                         .escape_quotes  = !cs->restore,
> |                 };
> |  
> | +               printf("found match %s, rev %u, xlate is %p\n",
> | +                               matchp->match->name,
> | +                               matchp->match->revision,
> | +                               matchp->match->xlate);
> |                 if (!matchp->match->xlate)
> |                         return 0;
> 
> The output is then:
> 
> | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> | nft found match state, rev 0, xlate is (nil)
> | # -I INPUT -m state --state NEW
> 
> Am I missing something??

Via nft_compatible_revision() I'm getting revision 3. Are you using
latest kernel from git?

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > While translating a conntrack state match in old syntax, matches are
> > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > matches are registered in reverse order of appearance in the array
> > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > doesn't define an xlate callback.
> > > > > 
> > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > highest revision one is found first.
> > > > 
> > > > Applied, thanks Phil.
> > > 
> > > Wait.
> > > 
> > > Do you mean this case?
> > > 
> > > # iptables-translate -I INPUT -m state --state NEW
> > > nft insert rule ip filter INPUT ct state new  counter
> > > 
> > > Hm, this works here.
> > 
> > Yes, that's it. And it working for you just emphasizes something's fishy
> > here. I just reverted my patch, then it's like this:
> > 
> > | $ ./configure --prefix=$PWD/install && make && make install
> > | [...]
> > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > | nft # -I INPUT -m state --state NEW
> > 
> > Using 'strace -eopen' I see that libxt_state.so from the install
> > destination is used, not the system one.
> > 
> > Also, I did this change for debugging:
> > 
> > | --- a/iptables/xtables-translate.c
> > | +++ b/iptables/xtables-translate.c
> > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > |                         .escape_quotes  = !cs->restore,
> > |                 };
> > |  
> > | +               printf("found match %s, rev %u, xlate is %p\n",
> > | +                               matchp->match->name,
> > | +                               matchp->match->revision,
> > | +                               matchp->match->xlate);
> > |                 if (!matchp->match->xlate)
> > |                         return 0;
> > 
> > The output is then:
> > 
> > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > | nft found match state, rev 0, xlate is (nil)
> > | # -I INPUT -m state --state NEW
> > 
> > Am I missing something??
> 
> Via nft_compatible_revision() I'm getting revision 3. Are you using
> latest kernel from git?

No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
installed, is that relevant?

Thanks, Phil

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Pablo Neira Ayuso]

On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > matches are registered in reverse order of appearance in the array
> > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > doesn't define an xlate callback.
> > > > > > 
> > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > highest revision one is found first.
> > > > > 
> > > > > Applied, thanks Phil.
> > > > 
> > > > Wait.
> > > > 
> > > > Do you mean this case?
> > > > 
> > > > # iptables-translate -I INPUT -m state --state NEW
> > > > nft insert rule ip filter INPUT ct state new  counter
> > > > 
> > > > Hm, this works here.
> > > 
> > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > here. I just reverted my patch, then it's like this:
> > > 
> > > | $ ./configure --prefix=$PWD/install && make && make install
> > > | [...]
> > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > | nft # -I INPUT -m state --state NEW
> > > 
> > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > destination is used, not the system one.
> > > 
> > > Also, I did this change for debugging:
> > > 
> > > | --- a/iptables/xtables-translate.c
> > > | +++ b/iptables/xtables-translate.c
> > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > |                         .escape_quotes  = !cs->restore,
> > > |                 };
> > > |  
> > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > | +                               matchp->match->name,
> > > | +                               matchp->match->revision,
> > > | +                               matchp->match->xlate);
> > > |                 if (!matchp->match->xlate)
> > > |                         return 0;
> > > 
> > > The output is then:
> > > 
> > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > | nft found match state, rev 0, xlate is (nil)
> > > | # -I INPUT -m state --state NEW
> > > 
> > > Am I missing something??
> > 
> > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > latest kernel from git?
> 
> No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> installed, is that relevant?

I have a way to trigger this here, but not sure if that is the problem
there. If the kernel comes with no nft_compat support, then
nft_compatible_revision() fails, and
xtables_fully_register_pending_match() ends up selecting revision 0.

In such case, I can reproduce your problem.

# iptables-translate -A INPUT -m state --state NEW
nft # -A INPUT -m state --state NEW 

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > doesn't define an xlate callback.
> > > > > > > 
> > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > highest revision one is found first.
> > > > > > 
> > > > > > Applied, thanks Phil.
> > > > > 
> > > > > Wait.
> > > > > 
> > > > > Do you mean this case?
> > > > > 
> > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > 
> > > > > Hm, this works here.
> > > > 
> > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > here. I just reverted my patch, then it's like this:
> > > > 
> > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > | [...]
> > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > | nft # -I INPUT -m state --state NEW
> > > > 
> > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > destination is used, not the system one.
> > > > 
> > > > Also, I did this change for debugging:
> > > > 
> > > > | --- a/iptables/xtables-translate.c
> > > > | +++ b/iptables/xtables-translate.c
> > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > |                         .escape_quotes  = !cs->restore,
> > > > |                 };
> > > > |  
> > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > | +                               matchp->match->name,
> > > > | +                               matchp->match->revision,
> > > > | +                               matchp->match->xlate);
> > > > |                 if (!matchp->match->xlate)
> > > > |                         return 0;
> > > > 
> > > > The output is then:
> > > > 
> > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > | nft found match state, rev 0, xlate is (nil)
> > > > | # -I INPUT -m state --state NEW
> > > > 
> > > > Am I missing something??
> > > 
> > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > latest kernel from git?
> > 
> > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > installed, is that relevant?
> 
> I have a way to trigger this here, but not sure if that is the problem
> there. If the kernel comes with no nft_compat support, then
> nft_compatible_revision() fails, and
> xtables_fully_register_pending_match() ends up selecting revision 0.
> 
> In such case, I can reproduce your problem.
> 
> # iptables-translate -A INPUT -m state --state NEW
> nft # -A INPUT -m state --state NEW 

Ah! On my system, nft_compat is built as module and it didn't matter
whether I have it loaded or not, nft_compatible_revision() would never
succeed.

Oh man, I just found the cause: I was running iptables-translate as
unprivileged user. Calling it with sudo magically makes everything work.

I'll have a look whether it's possible to communicate the received
-EPERM back to the user.

Cheers, Phil

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > > doesn't define an xlate callback.
> > > > > > > > 
> > > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > > highest revision one is found first.
> > > > > > > 
> > > > > > > Applied, thanks Phil.
> > > > > > 
> > > > > > Wait.
> > > > > > 
> > > > > > Do you mean this case?
> > > > > > 
> > > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > > 
> > > > > > Hm, this works here.
> > > > > 
> > > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > > here. I just reverted my patch, then it's like this:
> > > > > 
> > > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > > | [...]
> > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > | nft # -I INPUT -m state --state NEW
> > > > > 
> > > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > > destination is used, not the system one.
> > > > > 
> > > > > Also, I did this change for debugging:
> > > > > 
> > > > > | --- a/iptables/xtables-translate.c
> > > > > | +++ b/iptables/xtables-translate.c
> > > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > > |                         .escape_quotes  = !cs->restore,
> > > > > |                 };
> > > > > |  
> > > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > > | +                               matchp->match->name,
> > > > > | +                               matchp->match->revision,
> > > > > | +                               matchp->match->xlate);
> > > > > |                 if (!matchp->match->xlate)
> > > > > |                         return 0;
> > > > > 
> > > > > The output is then:
> > > > > 
> > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > | nft found match state, rev 0, xlate is (nil)
> > > > > | # -I INPUT -m state --state NEW
> > > > > 
> > > > > Am I missing something??
> > > > 
> > > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > > latest kernel from git?
> > > 
> > > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > > installed, is that relevant?
> > 
> > I have a way to trigger this here, but not sure if that is the problem
> > there. If the kernel comes with no nft_compat support, then
> > nft_compatible_revision() fails, and
> > xtables_fully_register_pending_match() ends up selecting revision 0.
> > 
> > In such case, I can reproduce your problem.
> > 
> > # iptables-translate -A INPUT -m state --state NEW
> > nft # -A INPUT -m state --state NEW 
> 
> Ah! On my system, nft_compat is built as module and it didn't matter
> whether I have it loaded or not, nft_compatible_revision() would never
> succeed.
> 
> Oh man, I just found the cause: I was running iptables-translate as
> unprivileged user. Calling it with sudo magically makes everything work.
> 
> I'll have a look whether it's possible to communicate the received
> -EPERM back to the user.

I wonder if we just update xtables_globals for xtables-translate.c so
we don't require any kernel intervention, just add a dummy
nft_compatible_revision() that simply says: Yes, this revision is
good. So we just let libxtables select the largest revision number all
the time and we skip traffic and dependencies with the kernel.

It would be great if you can have a look into this. Thanks!

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> Oh man, I just found the cause: I was running iptables-translate as
> unprivileged user. Calling it with sudo magically makes everything work.
> 
> I'll have a look whether it's possible to communicate the received
> -EPERM back to the user.

I wonder how iptables should deal with this situation - in a regular
use-case, I guess the program will abort eventually anyway but for
iptables-translate it shouldn't really matter. So do you think it's OK
to make nft_compatible_revision() return 0 if it made it past
mnl_cb_run() and errno is EPERM?

Ideally it should warn as well, of course.

Cheers, Phil

Re: [iptables PATCH] extensions: libxt_conntrack: Fix 'state' translation to nft

[Phil Sutter]

On Wed, Mar 08, 2017 at 02:38:42PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> > On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > > > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > > > doesn't define an xlate callback.
> > > > > > > > > 
> > > > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > > > highest revision one is found first.
> > > > > > > > 
> > > > > > > > Applied, thanks Phil.
> > > > > > > 
> > > > > > > Wait.
> > > > > > > 
> > > > > > > Do you mean this case?
> > > > > > > 
> > > > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > > > 
> > > > > > > Hm, this works here.
> > > > > > 
> > > > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > > > here. I just reverted my patch, then it's like this:
> > > > > > 
> > > > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > > > | [...]
> > > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > > | nft # -I INPUT -m state --state NEW
> > > > > > 
> > > > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > > > destination is used, not the system one.
> > > > > > 
> > > > > > Also, I did this change for debugging:
> > > > > > 
> > > > > > | --- a/iptables/xtables-translate.c
> > > > > > | +++ b/iptables/xtables-translate.c
> > > > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > > > |                         .escape_quotes  = !cs->restore,
> > > > > > |                 };
> > > > > > |  
> > > > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > > > | +                               matchp->match->name,
> > > > > > | +                               matchp->match->revision,
> > > > > > | +                               matchp->match->xlate);
> > > > > > |                 if (!matchp->match->xlate)
> > > > > > |                         return 0;
> > > > > > 
> > > > > > The output is then:
> > > > > > 
> > > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > > | nft found match state, rev 0, xlate is (nil)
> > > > > > | # -I INPUT -m state --state NEW
> > > > > > 
> > > > > > Am I missing something??
> > > > > 
> > > > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > > > latest kernel from git?
> > > > 
> > > > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > > > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > > > installed, is that relevant?
> > > 
> > > I have a way to trigger this here, but not sure if that is the problem
> > > there. If the kernel comes with no nft_compat support, then
> > > nft_compatible_revision() fails, and
> > > xtables_fully_register_pending_match() ends up selecting revision 0.
> > > 
> > > In such case, I can reproduce your problem.
> > > 
> > > # iptables-translate -A INPUT -m state --state NEW
> > > nft # -A INPUT -m state --state NEW 
> > 
> > Ah! On my system, nft_compat is built as module and it didn't matter
> > whether I have it loaded or not, nft_compatible_revision() would never
> > succeed.
> > 
> > Oh man, I just found the cause: I was running iptables-translate as
> > unprivileged user. Calling it with sudo magically makes everything work.
> > 
> > I'll have a look whether it's possible to communicate the received
> > -EPERM back to the user.
> 
> I wonder if we just update xtables_globals for xtables-translate.c so
> we don't require any kernel intervention, just add a dummy
> nft_compatible_revision() that simply says: Yes, this revision is
> good. So we just let libxtables select the largest revision number all
> the time and we skip traffic and dependencies with the kernel.

Oh, that sounds like a great solution! I'll have a look.

Thanks, Phil

[iptables PATCH] xtables-translate: Avoid querying the kernel

[Phil Sutter]

This originally came up when accidentally calling iptables-translate as
unprivileged user - nft_compatible_revision() then fails every time,
making the translator fall back to using revision 0 only which often
leads to failed translations (due to missing xlate callback).

The bottom line is there is no need to check what revision of a given
iptables match the kernel supports when it is only to be translated into
an nftables equivalent. So just assign a dummy callback returning good
for any revision being asked for.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-translate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 153bd6503c59b..76ca666b79f96 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -379,6 +379,14 @@ static int xlate_chain_set(struct nft_handle *h, const char *table,
 	return 1;
 }
 
+static int dummy_compat_rev(const char *name, uint8_t rev, int opt)
+{
+	/* Avoid querying the kernel - it's not needed when just translating
+	 * rules and not even possible when running as unprivileged user.
+	 */
+	return 1;
+}
+
 static struct nft_xt_restore_cb cb_xlate = {
 	.table_new	= xlate_table_new,
 	.chain_set	= xlate_chain_set,
@@ -398,6 +406,7 @@ static int xtables_xlate_main(int family, const char *progname, int argc,
 	};
 
 	xtables_globals.program_name = progname;
+	xtables_globals.compat_rev = dummy_compat_rev;
 	ret = xtables_init_all(&xtables_globals, family);
 	if (ret < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
@@ -440,6 +449,7 @@ static int xtables_restore_xlate_main(int family, const char *progname,
 	int c;
 
 	xtables_globals.program_name = progname;
+	xtables_globals.compat_rev = dummy_compat_rev;
 	ret = xtables_init_all(&xtables_globals, family);
 	if (ret < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-- 
2.11.0

Re: [iptables PATCH] xtables-translate: Avoid querying the kernel

[Pablo Neira Ayuso]

On Wed, Mar 08, 2017 at 04:43:25PM +0100, Phil Sutter wrote:
> This originally came up when accidentally calling iptables-translate as
> unprivileged user - nft_compatible_revision() then fails every time,
> making the translator fall back to using revision 0 only which often
> leads to failed translations (due to missing xlate callback).
> 
> The bottom line is there is no need to check what revision of a given
> iptables match the kernel supports when it is only to be translated into
> an nftables equivalent. So just assign a dummy callback returning good
> for any revision being asked for.

Applied, thanks a lot Phil.