[iptables PATCH] Unbreak xtables-translate

[iptables PATCH] Unbreak xtables-translate

[Phil Sutter]

Fixed commit broke xtables-translate which still relied upon do_parse()
to properly initialize the passed iptables_command_state reference. To
allow for callers to preset fields, this doesn't happen anymore so
do_command_xlate() has to initialize itself. Otherwise garbage from
stack is read leading to segfaults and program aborts.

Although init_cs callback is used by arptables only and
arptables-translate has not been implemented, do call it if set just to
avoid future issues.

Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-translate.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 086b85d2f9cef..e2948c5009dd6 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[],
 		.restore	= restore,
 		.xlate		= true,
 	};
-	struct iptables_command_state cs;
+	struct iptables_command_state cs = {
+		.jumpto = "",
+		.argv = argv,
+	};
+
 	struct xtables_args args = {
 		.family = h->family,
 	};
 
+	if (h->ops->init_cs)
+		h->ops->init_cs(&cs);
+
 	do_parse(h, argc, argv, &p, &cs, &args);
 
 	cs.restore = restore;
-- 
2.33.0

Re: [iptables PATCH] Unbreak xtables-translate

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On 2021-11-06, at 21:45:44 +0100, Phil Sutter wrote:
> Fixed commit broke xtables-translate which still relied upon
> do_parse() to properly initialize the passed iptables_command_state
> reference. To allow for callers to preset fields, this doesn't happen
> anymore so do_command_xlate() has to initialize itself. Otherwise
> garbage from stack is read leading to segfaults and program aborts.
>
> Although init_cs callback is used by arptables only and
> arptables-translate has not been implemented, do call it if set just
> to avoid future issues.
>
> Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/xtables-translate.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
> index 086b85d2f9cef..e2948c5009dd6 100644
> --- a/iptables/xtables-translate.c
> +++ b/iptables/xtables-translate.c
> @@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[],
>  		.restore	= restore,
>  		.xlate		= true,
>  	};
> -	struct iptables_command_state cs;
> +	struct iptables_command_state cs = {
> +		.jumpto = "",
> +		.argv = argv,
> +	};

No need to initialize .jumpto explicitly: initializing .argv will
zero-initialize all the other members.

> +
>  	struct xtables_args args = {
>  		.family = h->family,
>  	};
>
> +	if (h->ops->init_cs)
> +		h->ops->init_cs(&cs);
> +
>  	do_parse(h, argc, argv, &p, &cs, &args);
>
>  	cs.restore = restore;
> --
> 2.33.0
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [iptables PATCH] Unbreak xtables-translate

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On 2021-11-07, at 16:03:38 +0000, Jeremy Sowden wrote:
> On 2021-11-06, at 21:45:44 +0100, Phil Sutter wrote:
> > Fixed commit broke xtables-translate which still relied upon
> > do_parse() to properly initialize the passed iptables_command_state
> > reference. To allow for callers to preset fields, this doesn't
> > happen anymore so do_command_xlate() has to initialize itself.
> > Otherwise garbage from stack is read leading to segfaults and
> > program aborts.
> >
> > Although init_cs callback is used by arptables only and
> > arptables-translate has not been implemented, do call it if set just
> > to avoid future issues.
> >
> > Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  iptables/xtables-translate.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
> > index 086b85d2f9cef..e2948c5009dd6 100644
> > --- a/iptables/xtables-translate.c
> > +++ b/iptables/xtables-translate.c
> > @@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[],
> >  		.restore	= restore,
> >  		.xlate		= true,
> >  	};
> > -	struct iptables_command_state cs;
> > +	struct iptables_command_state cs = {
> > +		.jumpto = "",
> > +		.argv = argv,
> > +	};
>
> No need to initialize .jumpto explicitly: initializing .argv will
> zero-initialize all the other members.

Apologies, I'm talking nonsense: .jumpto is a pointer, not an array.
Ignore me. :)

> > +
> >  	struct xtables_args args = {
> >  		.family = h->family,
> >  	};
> >
> > +	if (h->ops->init_cs)
> > +		h->ops->init_cs(&cs);
> > +
> >  	do_parse(h, argc, argv, &p, &cs, &args);
> >
> >  	cs.restore = restore;
> > --
> > 2.33.0
> >
> >



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [iptables PATCH] Unbreak xtables-translate

[Pablo Neira Ayuso]

On Sat, Nov 06, 2021 at 09:45:44PM +0100, Phil Sutter wrote:
> Fixed commit broke xtables-translate which still relied upon do_parse()
> to properly initialize the passed iptables_command_state reference. To
> allow for callers to preset fields, this doesn't happen anymore so
> do_command_xlate() has to initialize itself. Otherwise garbage from
> stack is read leading to segfaults and program aborts.
> 
> Although init_cs callback is used by arptables only and
> arptables-translate has not been implemented, do call it if set just to
> avoid future issues.
> 
> Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Tested-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [iptables PATCH] Unbreak xtables-translate

[Phil Sutter]

Hey Jeremy,

On Sun, Nov 07, 2021 at 04:07:27PM +0000, Jeremy Sowden wrote:
[...]
> Apologies, I'm talking nonsense: .jumpto is a pointer, not an array.
> Ignore me. :)

I wondered at first, but indeed assigning an empty string to an array is
identical to setting all fields zero. :)

Thanks for the review!

Cheers, Phil