Re: [PATCH nf-next 2/3] netfilter: core: only allow one nat hook per hook point

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Dec 08, 2017 at 05:01:54PM +0100, Florian Westphal wrote:
> The netfilter NAT core cannot deal with more than one NAT hook per hook
> location (prerouting, input ...), because the NAT hooks install a NAT null
> binding in case the iptables nat table (iptable_nat hooks) or the
> corresponding nftables chain (nft nat hooks) doesn't specify a nat
> transformation.
> 
> Null bindings are needed to detect port collsisions between NAT-ed and
> non-NAT-ed connections.
> 
> This causes nftables NAT rules to not work when iptable_nat module is
> loaded, and vice versa because nat binding has already been attached
> when the second nat hook is consulted.
> 
> The netfilter core is not really the correct location to handle this
> (hooks are just hooks, the core has no notion of what kinds of side
>  effects a hook implements), but its the only place where we can check
> for conflicts between both iptables hooks and nftables hooks without
> adding dependencies.
> 
> So add nat annotation to hook_ops to describe those hooks that will
> add NAT bindings and then make core reject if such a hook already exists.
> The annotation fills a padding hole, in case further restrictions appar
> we might change this to a 'u8 type' instead of bool.
> 
> iptables error if nft nat hook active:
> iptables -t nat -A POSTROUTING -j MASQUERADE
> iptables v1.4.21: can't initialize iptables table `nat': File exists
> Perhaps iptables or your kernel needs to be upgraded.
> 
> nftables error if iptables nat table present:
> nft -f /etc/nftables/ipv4-nat
> /usr/etc/nftables/ipv4-nat:3:1-2: Error: Could not process rule: File exists
> table nat {
> ^^

This reminds me we have to fix error reporting on chains, it seems
location is unset by the time, there's a bugreport on the wiki on
this.

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/linux/netfilter.h         | 1 +
>  net/ipv4/netfilter/iptable_nat.c  | 4 ++++
>  net/ipv6/netfilter/ip6table_nat.c | 4 ++++
>  net/netfilter/core.c              | 6 ++++++
>  net/netfilter/nf_tables_api.c     | 2 ++
>  5 files changed, 17 insertions(+)
> 
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index 9dcbcdfa3b82..579b1ba86ee6 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -67,6 +67,7 @@ struct nf_hook_ops {
>  	struct net_device	*dev;
>  	void			*priv;
>  	u_int8_t		pf;
> +	bool			nat_hook;
>  	unsigned int		hooknum;
>  	/* Hooks are ordered in ascending priority. */
>  	int			priority;
> diff --git a/net/ipv4/netfilter/iptable_nat.c b/net/ipv4/netfilter/iptable_nat.c
> index a1a07b338ccf..0f7255cc65ee 100644
> --- a/net/ipv4/netfilter/iptable_nat.c
> +++ b/net/ipv4/netfilter/iptable_nat.c
> @@ -72,6 +72,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
>  	{
>  		.hook		= iptable_nat_ipv4_in,
>  		.pf		= NFPROTO_IPV4,
> +		.nat_hook	= true,

Just a suggestion: This nat_hook basically means that we only allow
this hook to be a singleton in this spot. So I would call it like
this, ie. singleton, given we have no NAT semantics in the netfilter
core.

>  		.hooknum	= NF_INET_PRE_ROUTING,
>  		.priority	= NF_IP_PRI_NAT_DST,
>  	},
> @@ -79,6 +80,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
>  	{
>  		.hook		= iptable_nat_ipv4_out,
>  		.pf		= NFPROTO_IPV4,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_POST_ROUTING,
>  		.priority	= NF_IP_PRI_NAT_SRC,
>  	},
> @@ -86,6 +88,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
>  	{
>  		.hook		= iptable_nat_ipv4_local_fn,
>  		.pf		= NFPROTO_IPV4,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_LOCAL_OUT,
>  		.priority	= NF_IP_PRI_NAT_DST,
>  	},
> @@ -93,6 +96,7 @@ static const struct nf_hook_ops nf_nat_ipv4_ops[] = {
>  	{
>  		.hook		= iptable_nat_ipv4_fn,
>  		.pf		= NFPROTO_IPV4,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_LOCAL_IN,
>  		.priority	= NF_IP_PRI_NAT_SRC,
>  	},
> diff --git a/net/ipv6/netfilter/ip6table_nat.c b/net/ipv6/netfilter/ip6table_nat.c
> index 991512576c8c..47306e45a80a 100644
> --- a/net/ipv6/netfilter/ip6table_nat.c
> +++ b/net/ipv6/netfilter/ip6table_nat.c
> @@ -74,6 +74,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
>  	{
>  		.hook		= ip6table_nat_in,
>  		.pf		= NFPROTO_IPV6,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_PRE_ROUTING,
>  		.priority	= NF_IP6_PRI_NAT_DST,
>  	},
> @@ -81,6 +82,7 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
>  	{
>  		.hook		= ip6table_nat_out,
>  		.pf		= NFPROTO_IPV6,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_POST_ROUTING,
>  		.priority	= NF_IP6_PRI_NAT_SRC,
>  	},
> @@ -88,12 +90,14 @@ static const struct nf_hook_ops nf_nat_ipv6_ops[] = {
>  	{
>  		.hook		= ip6table_nat_local_fn,
>  		.pf		= NFPROTO_IPV6,
> +		.nat_hook	= true,
>  		.hooknum	= NF_INET_LOCAL_OUT,
>  		.priority	= NF_IP6_PRI_NAT_DST,
>  	},
>  	/* After packet filtering, change source */
>  	{
>  		.hook		= ip6table_nat_fn,
> +		.nat_hook	= true,
>  		.pf		= NFPROTO_IPV6,
>  		.hooknum	= NF_INET_LOCAL_IN,
>  		.priority	= NF_IP6_PRI_NAT_SRC,
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index a6eaaf303be8..6c263efed2b6 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -160,6 +160,12 @@ nf_hook_entries_grow(const struct nf_hook_entries *old,
>  			++i;
>  			continue;
>  		}
> +
> +		if (reg->nat_hook && orig_ops[i]->nat_hook) {
> +			kvfree(new);
> +			return ERR_PTR(-EEXIST);
> +		}
> +
>  		if (inserted || reg->priority > orig_ops[i]->priority) {
>  			new_ops[nhooks] = (void *)orig_ops[i];
>  			new->hooks[nhooks] = old->hooks[i];
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index d8327b43e4dc..f000d4399c7a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1400,6 +1400,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
>  				ops->hook = hookfn;
>  			if (afi->hook_ops_init)
>  				afi->hook_ops_init(ops, i);
> +			if (basechain->type->type == NFT_CHAIN_T_NAT)
> +				ops->nat_hook = true;
>  		}
>  
>  		chain->flags |= NFT_BASE_CHAIN;
> -- 
> 2.13.6
> 
> --
> 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