[v2,net] netfilter: conntrack: reject nested ctnetlink master chains

Conntracks created through ctnetlink can reference another conntrack as
their master.  Userspace can repeat this to build an unbounded chain whose
recursive destruction exhausts the kernel stack.

Stop the repeatable userspace paths.  Reject a direct master that already
has a master, and reject NFQUEUE-attached expectations for such conntracks.
Keep regular ctnetlink and kernel helper expectations unchanged.

Fixes: 5faa1f4cb5a1 ("[NETFILTER]: nf_conntrack_netlink: add support to related connections")
Cc: stable@vger.kernel.org
Suggested-by: Florian Westphal <fw@strlen.de>
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Assisted-by: LLM
Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>
---
Changes in v2:
- Replace iterative destruction with the maintainer-requested creation-time
  restrictions.
- Reject nested NFQUEUE-attached expectations while preserving regular
  ctnetlink and kernel helper expectations.

Tested on net 71a77ab76e74 and exact v6.12.105.  In both userns runs a
one-level master was accepted, 5,998 nested direct attempts returned
EOPNOTSUPP, and cleanup ended with nf_conntrack_count=0 without a crash.
A policy control also confirmed that terminal IPCTNL_MSG_EXP_NEW remains
accepted on a related master.  A real iptables NFQUEUE/NFQA_EXP control
created one expectation before the patch and none after it.  The related
object is W=1 warning-free.  The netdev allyesconfig and allmodconfig W=1
full builds were not run.

 net/netfilter/nf_conntrack_netlink.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Daehyeon Ko <4ncienth@gmail.com> wrote:
> Conntracks created through ctnetlink can reference another conntrack as
> their master.  Userspace can repeat this to build an unbounded chain whose
> recursive destruction exhausts the kernel stack.
> 
> Stop the repeatable userspace paths.  Reject a direct master that already
> has a master, and reject NFQUEUE-attached expectations for such conntracks.
> Keep regular ctnetlink and kernel helper expectations unchanged.
> 
> Fixes: 5faa1f4cb5a1 ("[NETFILTER]: nf_conntrack_netlink: add support to related connections")
> Cc: stable@vger.kernel.org
> Suggested-by: Florian Westphal <fw@strlen.de>
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Assisted-by: LLM
> Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>

LGTM.  FWIW I think we should also apply the other patch to
destroy iteratively instead of recursing.

Reviewed-by: Florian Westphal <fw@strlen.de>

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix does not stop userspace from building an unbounded
  ct->master chain.
- [High] The commit keeps "kernel helper expectations unchanged", but the
  H.323 Q.931 helper nests itself without limit.
- [Medium] The new check in ctnetlink_create_conntrack()
  (nf_conntrack_netlink.c:2362) makes IPCTNL_MSG_CT_NEW fail with…

--

netfilter: conntrack: reject nested ctnetlink master chains

The patch rejects ctnetlink conntrack creation when the CTA_TUPLE_MASTER
conntrack already has a master. It also rejects NFQUEUE-attached
expectations (NFQA_EXP) for conntracks that have a master. The aim is to
stop userspace from building unbounded master chains, whose recursive
release in nf_ct_destroy() exhausts the kernel stack.

> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 4e5d7c7014368..c68e79d1ac87b 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2359,6 +2359,11 @@ ctnetlink_create_conntrack(struct net *net,
>  			goto err2;
>  		}
>  		master_ct = nf_ct_tuplehash_to_ctrack(master_h);
> +		if (master_ct->master) {
> +			nf_ct_put(master_ct);
> +			err = -EOPNOTSUPP;
> +			goto err2;
> +		}

[Severity: Medium]
Does this check reject state that the kernel itself creates? In H.323, a
RAS expectation creates the Q.931 conntrack. That Q.931 conntrack then
sets up the H.245 expectation:

net/netfilter/nf_conntrack_h323_main.c:expect_h245() {
    ...
	rcu_assign_pointer(exp->assign_helper, nf_conntrack_helper_h245_ptr);
    ...
}

So the resulting H.245 and RTP conntracks have a master that itself has
a master.

A state-sync daemon such as conntrackd may recreate those flows with
IPCTNL_MSG_CT_NEW and a CTA_TUPLE_MASTER that points at the related Q.931
conntrack. That request used to be accepted and now gets -EOPNOTSUPP, so
these flows would not be replicated after a failover.

Since this is tagged for stable, could the commit message mention this
change in what CT_NEW accepts?

>  		__set_bit(IPS_EXPECTED_BIT, &ct->status);
>  		ct->master = master_ct;
>  	}
> @@ -2864,6 +2869,9 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
>  	struct nf_conntrack_expect *exp;
>  	int err;
>  
> +	if (ct->master)
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Can nf_tables still reach the same stack exhaustion? The commit message
says "Stop the repeatable userspace paths", but the patch only covers
CTA_TUPLE_MASTER and NFQA_EXP.

The first path uses an nft ct expectation object alone.
nft_ct_expect_obj_eval() only skips conntracks that are confirmed,
templates, or that already have a help extension:

net/netfilter/nft_ct.c:nft_ct_expect_obj_eval() {
    ...
	ct = nf_ct_get(pkt->skb, &ctinfo);
	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
		regs->verdict.code = NFT_BREAK;
		return;
	}

	help = nfct_help(ct);
	if (help) {
    ...
}

A RELATED child created from an expectation with a NULL assign_helper has
no help extension, so it also gets the ct_expect_help helper.

ct_expect_help() then calls nf_ct_expect_related() with the child as
exp->master, and it never checks ct->master. Over loopback, each new flow
from a fresh source port consumes the previous expectation and arms the
next one, so every flow adds a level.

The second path combines an nft ct helper with ctnetlink.
IPCTNL_MSG_EXP_NEW only stops the chain because ctnetlink_alloc_expect()
requires help->helper.

nft_ct_helper_obj_eval() assigns a helper to any unconfirmed conntrack,
related ones included. After that, ctnetlink_create_expect() succeeds
with the child as master.

A kernel helper attached this way, such as FTP, could also extend the
chain from traffic alone.

In every case init_conntrack() sets ct->master = exp->master with no
depth limit. Freeing the leaf then recurses once per level:

nf_ct_put()
  nf_ct_destroy()
    nf_ct_put(ct->master)
      nf_ct_destroy()
        ...

Both nft paths need only CAP_NET_ADMIN in a user/net namespace, the same
privilege ctnetlink needs.

Would a central bound in nf_ct_expect_related() or init_conntrack() be
needed to close this, or an iterative release of the master chain in
nf_ct_destroy()? Gating each entry point separately leaves these paths
open.

[Severity: High]
The commit message says "Keep regular ctnetlink and kernel helper
expectations unchanged". Can the H.323 Q.931 helper build an unbounded
chain on its own?

For a Facility message with reason callForwarded, process_facility()
calls expect_callforwarding(). That function gives the expected child the
Q.931 helper again:

net/netfilter/nf_conntrack_h323_main.c:expect_callforwarding() {
    ...
	rcu_assign_pointer(exp->assign_helper, nf_conntrack_helper_q931_ptr[0]);
    ...
}

Neither function checks IPS_EXPECTED or ct->master. Each new expected
Q.931 connection can therefore send another Facility(callForwarded) and
create the next level.

The default callforward_filter does not seem to prevent this.
callforward_do_filter() only filters when the nexthop and device are the
same, and two distinct local addresses have different nexthops.

This works over loopback from an unprivileged user namespace once the
helper is attached. It also works for remote peers wherever the Q.931
helper is enabled. Either way, the same recursion in nf_ct_destroy()
exhausts the stack.

SIP REGISTER does not seem to have this problem, because
process_register_request() returns early when IPS_EXPECTED is set.

>  	err = nla_parse_nested_deprecated(cda, CTA_EXPECT_MAX, attr,
>  					  exp_nla_policy, NULL);
>  	if (err < 0)

Thanks for the review.  All three points are valid.

The nft ct expectation object can attach its internal helper to an
unconfirmed expected child, then arm the next expectation with that child
as master.  I confirmed a bounded four-entry chain through this path, and
current net retains the same unchecked transition.  An nft helper followed
by IPCTNL_MSG_EXP_NEW can repeat in the same way.  The H.323 Q.931
call-forwarding path also propagates its helper without a depth bound.

The CT_NEW compatibility concern is valid as well.  CTA_TUPLE_MASTER was
introduced for conntrackd replication, so rejecting nested helper-created
state changes that interface.

V2 is therefore incomplete and should not be applied.  I have reworked the
replacement to make master destruction stack-safe while preserving nested
expectation semantics.

pw-bot: cr