[PATCH nf] netfilter: xt_devgroup: reject unsupported families in checkentry

[PATCH nf] netfilter: xt_devgroup: reject unsupported families in checkentry

[bestswngs]

From: Weiming Shi <bestswngs@gmail.com>

devgroup_mt_checkentry() validates hook_mask using NF_INET_* constants,
but the match is registered with NFPROTO_UNSPEC, which allows it to be
used from any protocol family through nft_compat.

On an ARP nftables output chain, nft_compat passes
hook_mask = 1 << NF_ARP_OUT. Because NF_ARP_OUT == 1 == NF_INET_LOCAL_IN,
the source-group hook validation incorrectly accepts the rule. At runtime
arp_xmit() invokes the chain with state->in == NULL, and devgroup_mt()
dereferences xt_in(par)->group, crashing the kernel:

 Oops: general protection fault, probably for non-canonical address 0xdffffc0000000044: 0000 [#1] SMP KASAN NOPTI
 KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
 RIP: 0010:devgroup_mt+0xff/0x350
 Call Trace:
  <TASK>
  nft_match_eval (net/netfilter/nft_compat.c:407)
  nft_do_chain (net/netfilter/nf_tables_core.c:285)
  nft_do_chain_arp (net/netfilter/nft_chain_filter.c:61)
  nf_hook_slow (net/netfilter/core.c:623)
  arp_xmit (net/ipv4/arp.c:666)
  arp_solicit (net/ipv4/arp.c:393)
  neigh_probe (net/core/neighbour.c:1098)
  __neigh_event_send (net/core/neighbour.c:1277)
  neigh_resolve_output (net/core/neighbour.c:1604)
  ip_finish_output2 (net/ipv4/ip_output.c:237)
  </TASK>
 Kernel panic - not syncing: Fatal exception in interrupt

Reject families whose hook numbering differs from the NF_INET_* scheme
early in checkentry. NFPROTO_INET and NFPROTO_BRIDGE share the same
five-hook layout (PRE_ROUTING ... POST_ROUTING) and the same
state->in/state->out semantics as IPv4/IPv6, so they are safe.
ARP only has three hooks (IN=0, OUT=1, FORWARD=2) with different
semantics, causing the numbering collision that triggers this bug.

The match is intentionally registered as NFPROTO_UNSPEC (it carries
MODULE_ALIAS entries for both ipt_devgroup and ip6t_devgroup), but
accepting it on ARP chains was never intended and is unsafe.

Trigger conditions:
- Required CONFIG: CONFIG_NF_TABLES=y, CONFIG_NFT_COMPAT=y,
  CONFIG_NF_TABLES_ARP=y, CONFIG_NETFILTER_XT_MATCH_DEVGROUP=y
  (all enabled by default on Ubuntu 24.04)
- Required privilege: CAP_NET_ADMIN (namespace-reachable via user+net
  namespace on systems with unprivileged user namespaces)
- Attack vector: local, via nftables ARP output chain + xt-compat match

Fixes: 9291747f118d ("netfilter: xtables: add device group match")
Cc: stable@vger.kernel.org
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/netfilter/xt_devgroup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/xt_devgroup.c b/net/netfilter/xt_devgroup.c
index 9520dd00070b2..86eb07d63274e 100644
--- a/net/netfilter/xt_devgroup.c
+++ b/net/netfilter/xt_devgroup.c
@@ -37,6 +37,12 @@ static int devgroup_mt_checkentry(const struct xt_mtchk_param *par)
 {
 	const struct xt_devgroup_info *info = par->matchinfo;

+	if (par->family != NFPROTO_IPV4 &&
+	    par->family != NFPROTO_IPV6 &&
+	    par->family != NFPROTO_INET &&
+	    par->family != NFPROTO_BRIDGE)
+		return -EINVAL;
+
 	if (info->flags & ~(XT_DEVGROUP_MATCH_SRC | XT_DEVGROUP_INVERT_SRC |
 			    XT_DEVGROUP_MATCH_DST | XT_DEVGROUP_INVERT_DST))
 		return -EINVAL;
--
2.43.0

Re: [PATCH nf] netfilter: xt_devgroup: reject unsupported families in checkentry

[Florian Westphal]

bestswngs@gmail.com <bestswngs@gmail.com> wrote:
> From: Weiming Shi <bestswngs@gmail.com>
> 
> devgroup_mt_checkentry() validates hook_mask using NF_INET_* constants,
> but the match is registered with NFPROTO_UNSPEC, which allows it to be
> used from any protocol family through nft_compat.
>
> On an ARP nftables output chain, nft_compat passes
> hook_mask = 1 << NF_ARP_OUT. Because NF_ARP_OUT == 1 == NF_INET_LOCAL_IN,
> the source-group hook validation incorrectly accepts the rule. At runtime
> arp_xmit() invokes the chain with state->in == NULL, and devgroup_mt()
> dereferences xt_in(par)->group, crashing the kernel:
> 
>  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000044: 0000 [#1] SMP KASAN NOPTI
>  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
>  RIP: 0010:devgroup_mt+0xff/0x350
>  Call Trace:
>   <TASK>
>   nft_match_eval (net/netfilter/nft_compat.c:407)
>   nft_do_chain (net/netfilter/nf_tables_core.c:285)
>   nft_do_chain_arp (net/netfilter/nft_chain_filter.c:61)
>   nf_hook_slow (net/netfilter/core.c:623)
>   arp_xmit (net/ipv4/arp.c:666)
>   arp_solicit (net/ipv4/arp.c:393)
>   neigh_probe (net/core/neighbour.c:1098)
>   __neigh_event_send (net/core/neighbour.c:1277)
>   neigh_resolve_output (net/core/neighbour.c:1604)
>   ip_finish_output2 (net/ipv4/ip_output.c:237)
>   </TASK>
>  Kernel panic - not syncing: Fatal exception in interrupt
> 
> Reject families whose hook numbering differs from the NF_INET_* scheme
> early in checkentry. NFPROTO_INET and NFPROTO_BRIDGE share the same
> five-hook layout (PRE_ROUTING ... POST_ROUTING) and the same
> state->in/state->out semantics as IPv4/IPv6, so they are safe.
> ARP only has three hooks (IN=0, OUT=1, FORWARD=2) with different
> semantics, causing the numbering collision that triggers this bug.

I think we should solve this in x_tables.c so we don't have to ensure
all the .checkentry functions provide for this.
While this patch solves the specific module at hand, it begs
the question if the same bug pattern exist exlsewhere.

xt_check_match and xt_check_target should call a common
helper, this helper checks that if the match/target is UNSPEC
and has .hooks != 0, then the calling family is IPV4, IPV6 or BRIDGE.

All others can be rejected.

Would you make such a patch?

Thanks!

Re: [PATCH nf] netfilter: xt_devgroup: reject unsupported families in checkentry

[Weiming Shi]

On 26-03-22 09:39, Florian Westphal wrote:
> bestswngs@gmail.com <bestswngs@gmail.com> wrote:
> > From: Weiming Shi <bestswngs@gmail.com>
> > 
> > devgroup_mt_checkentry() validates hook_mask using NF_INET_* constants,
> > but the match is registered with NFPROTO_UNSPEC, which allows it to be
> > used from any protocol family through nft_compat.
> >
> > On an ARP nftables output chain, nft_compat passes
> > hook_mask = 1 << NF_ARP_OUT. Because NF_ARP_OUT == 1 == NF_INET_LOCAL_IN,
> > the source-group hook validation incorrectly accepts the rule. At runtime
> > arp_xmit() invokes the chain with state->in == NULL, and devgroup_mt()
> > dereferences xt_in(par)->group, crashing the kernel:
> > 
> >  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000044: 0000 [#1] SMP KASAN NOPTI
> >  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
> >  RIP: 0010:devgroup_mt+0xff/0x350
> >  Call Trace:
> >   <TASK>
> >   nft_match_eval (net/netfilter/nft_compat.c:407)
> >   nft_do_chain (net/netfilter/nf_tables_core.c:285)
> >   nft_do_chain_arp (net/netfilter/nft_chain_filter.c:61)
> >   nf_hook_slow (net/netfilter/core.c:623)
> >   arp_xmit (net/ipv4/arp.c:666)
> >   arp_solicit (net/ipv4/arp.c:393)
> >   neigh_probe (net/core/neighbour.c:1098)
> >   __neigh_event_send (net/core/neighbour.c:1277)
> >   neigh_resolve_output (net/core/neighbour.c:1604)
> >   ip_finish_output2 (net/ipv4/ip_output.c:237)
> >   </TASK>
> >  Kernel panic - not syncing: Fatal exception in interrupt
> > 
> > Reject families whose hook numbering differs from the NF_INET_* scheme
> > early in checkentry. NFPROTO_INET and NFPROTO_BRIDGE share the same
> > five-hook layout (PRE_ROUTING ... POST_ROUTING) and the same
> > state->in/state->out semantics as IPv4/IPv6, so they are safe.
> > ARP only has three hooks (IN=0, OUT=1, FORWARD=2) with different
> > semantics, causing the numbering collision that triggers this bug.
> 
> I think we should solve this in x_tables.c so we don't have to ensure
> all the .checkentry functions provide for this.
> While this patch solves the specific module at hand, it begs
> the question if the same bug pattern exist exlsewhere.
> 
> xt_check_match and xt_check_target should call a common
> helper, this helper checks that if the match/target is UNSPEC
> and has .hooks != 0, then the calling family is IPV4, IPV6 or BRIDGE.
> 
> All others can be rejected.
> 
> Would you make such a patch?
> 
> Thanks!

Hi,

Thanks for the review! I'll prepare a v2 patch that adds the family
check in xt_check_match/xt_check_target.

Weiming Shi