Re: [iptables PATCH v2 1/2] nft: fix interface comparisons in `-C` commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-11-19, at 23:03:24 +0100, Phil Sutter wrote:
> From: Jeremy Sowden <jeremy@xxxxxxxxxx>
> 
> Commit 9ccae6397475 ("nft: Leave interface masks alone when parsing from
> kernel") removed code which explicitly set interface masks to all ones.  The
> result of this is that they are zero.  However, they are used to mask interfaces
> in `is_same_interfaces`.  Consequently, the masked values are alway zero, the
> comparisons are always true, and check commands which ought to fail succeed:
> 
>   # iptables -N test
>   # iptables -A test -i lo \! -o lo -j REJECT
>   # iptables -v -L test
>   Chain test (0 references)
>    pkts bytes target     prot opt in     out     source               destination
>       0     0 REJECT     all  --  lo     !lo     anywhere             anywhere             reject-with icmp-port-unreachable
>   # iptables -v -C test -i abcdefgh \! -o abcdefgh -j REJECT
>   REJECT  all opt -- in lo out !lo  0.0.0.0/0  -> 0.0.0.0/0   reject-with icmp-port-unreachable
> 
> Remove the mask parameters from `is_same_interfaces`.  Add a test-case.
> 
> Fixes: 9ccae6397475 ("nft: Leave interface masks alone when parsing from kernel")
> Signed-off-by: Jeremy Sowden <jeremy@xxxxxxxxxx>
> Signed-off-by: Phil Sutter <phil@xxxxxx>
> ---
> Changes since v1:
> - Replace the loop by strncmp() calls.

LGTM.

J.

> ---
>  iptables/nft-arp.c                            | 10 ++----
>  iptables/nft-ipv4.c                           |  4 +--
>  iptables/nft-ipv6.c                           |  6 +---
>  iptables/nft-shared.c                         | 36 +++++--------------
>  iptables/nft-shared.h                         |  6 +---
>  .../nft-only/0020-compare-interfaces_0        |  9 +++++
>  6 files changed, 22 insertions(+), 49 deletions(-)
>  create mode 100755 iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> 
> diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
> index 264864c3fb2b2..c11d64c368638 100644
> --- a/iptables/nft-arp.c
> +++ b/iptables/nft-arp.c
> @@ -385,14 +385,8 @@ static bool nft_arp_is_same(const struct iptables_command_state *cs_a,
>  		return false;
>  	}
>  
> -	return is_same_interfaces(a->arp.iniface,
> -				  a->arp.outiface,
> -				  (unsigned char *)a->arp.iniface_mask,
> -				  (unsigned char *)a->arp.outiface_mask,
> -				  b->arp.iniface,
> -				  b->arp.outiface,
> -				  (unsigned char *)b->arp.iniface_mask,
> -				  (unsigned char *)b->arp.outiface_mask);
> +	return is_same_interfaces(a->arp.iniface, a->arp.outiface,
> +				  b->arp.iniface, b->arp.outiface);
>  }
>  
>  static void nft_arp_save_chain(const struct nftnl_chain *c, const char *policy)
> diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
> index 740928757b7e2..0c8bd2911d105 100644
> --- a/iptables/nft-ipv4.c
> +++ b/iptables/nft-ipv4.c
> @@ -113,9 +113,7 @@ static bool nft_ipv4_is_same(const struct iptables_command_state *a,
>  	}
>  
>  	return is_same_interfaces(a->fw.ip.iniface, a->fw.ip.outiface,
> -				  a->fw.ip.iniface_mask, a->fw.ip.outiface_mask,
> -				  b->fw.ip.iniface, b->fw.ip.outiface,
> -				  b->fw.ip.iniface_mask, b->fw.ip.outiface_mask);
> +				  b->fw.ip.iniface, b->fw.ip.outiface);
>  }
>  
>  static void nft_ipv4_set_goto_flag(struct iptables_command_state *cs)
> diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
> index b184f8af3e6ed..4dbb2af206054 100644
> --- a/iptables/nft-ipv6.c
> +++ b/iptables/nft-ipv6.c
> @@ -99,11 +99,7 @@ static bool nft_ipv6_is_same(const struct iptables_command_state *a,
>  	}
>  
>  	return is_same_interfaces(a->fw6.ipv6.iniface, a->fw6.ipv6.outiface,
> -				  a->fw6.ipv6.iniface_mask,
> -				  a->fw6.ipv6.outiface_mask,
> -				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface,
> -				  b->fw6.ipv6.iniface_mask,
> -				  b->fw6.ipv6.outiface_mask);
> +				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface);
>  }
>  
>  static void nft_ipv6_set_goto_flag(struct iptables_command_state *cs)
> diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> index 6775578b1e36b..2c29e68f551df 100644
> --- a/iptables/nft-shared.c
> +++ b/iptables/nft-shared.c
> @@ -220,36 +220,16 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r,
>  }
>  
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask)
> +			const char *b_iniface, const char *b_outiface)
>  {
> -	int i;
> -
> -	for (i = 0; i < IFNAMSIZ; i++) {
> -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> -			DEBUGP("different iniface mask %x, %x (%d)\n",
> -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> -			return false;
> -		}
> -		if ((a_iniface[i] & a_iniface_mask[i])
> -		    != (b_iniface[i] & b_iniface_mask[i])) {
> -			DEBUGP("different iniface\n");
> -			return false;
> -		}
> -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> -			DEBUGP("different outiface mask\n");
> -			return false;
> -		}
> -		if ((a_outiface[i] & a_outiface_mask[i])
> -		    != (b_outiface[i] & b_outiface_mask[i])) {
> -			DEBUGP("different outiface\n");
> -			return false;
> -		}
> +	if (strncmp(a_iniface, b_iniface, IFNAMSIZ)) {
> +		DEBUGP("different iniface\n");
> +		return false;
> +	}
> +	if (strncmp(a_outiface, b_outiface, IFNAMSIZ)) {
> +		DEBUGP("different outiface\n");
> +		return false;
>  	}
> -
>  	return true;
>  }
>  
> diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> index 51d1e4609a3b6..b57aee1f84a87 100644
> --- a/iptables/nft-shared.h
> +++ b/iptables/nft-shared.h
> @@ -105,11 +105,7 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r, uint8_t proto, uint
>  void add_compat(struct nftnl_rule *r, uint32_t proto, bool inv);
>  
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask);
> +			const char *b_iniface, const char *b_outiface);
>  
>  void __get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, uint8_t *op);
>  void get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, bool *inv);
> diff --git a/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0 b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> new file mode 100755
> index 0000000000000..278cd648ebb78
> --- /dev/null
> +++ b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> @@ -0,0 +1,9 @@
> +#!/bin/bash
> +
> +[[ $XT_MULTI == *xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
> +
> +$XT_MULTI iptables -N test
> +$XT_MULTI iptables -A test -i lo \! -o lo -j REJECT
> +$XT_MULTI iptables -C test -i abcdefgh \! -o abcdefgh -j REJECT 2>/dev/null && exit 1
> +
> +exit 0
> -- 
> 2.47.0
> 

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux