Re: [PATCH net-next 11/15] netfilter: nft_compat: restrict raw table targets/matches

[Florian Westphal <fw@strlen.de>]

netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:

TL;DR, this patch is fine as-is, but a complementary patch to
validate nft_ct.c and handle vstate cache is missing from this PR.  This
does not cause any breakage, though.

> - [High] The new 'raw' check in nft_compat_chain_validate_dependency()
>   (net/netfilter/nft_compat.c:61) depends on basechain->ops.priority.
> - [Low] The new comment in nft_compat_chain_validate_dependency() says
>   the legacy raw table 'only ever has prerouting/output chains at this…
> > @@ -45,6 +46,20 @@ static int nft_compat_chain_validate_dependency(const struct nft_ctx *ctx,
> >  			type = NFT_CHAIN_T_NAT;
> >  		if (basechain->type->type != type)
> >  			return -EINVAL;
> > +	} else if (strcmp(tablename, "raw") == 0) {
> > +		/* iptables-nft emulates the legacy raw table, which only
> > +		 * ever has prerouting/output chains at this exact priority.
> > +		 */
> 
> [Severity: Low]
> Is this comment accurate?

Is this a trick question?

> The legacy raw table can also be registered at a
> second priority. In net/ipv4/netfilter/iptable_raw.c:

[..]

> The table is selected when the raw_before_defrag module parameter is set.
> ip6table_raw.c does the same with NF_IP6_PRI_RAW_BEFORE_DEFRAG.

iptables-nft doesn't use iptable_raw.c nor does it emulate this
module param.

> A CT/NOTRACK target at -450 still runs ahead of conntrack. The new check
> below accepts only NF_IP_PRI_RAW, though, so it rejects that priority with
> -EINVAL. That makes it stricter than "what xtables enforces", as the
> commit message puts it.

Why should I support -450 ? iptables-nft doesn't use/support that.

> Should the comment say that iptables-nft always uses -300?

*shrug*, thats what I intended to convey: iptables-nft only uses
-300, hence hard refusal.

> Alternatively,
> the check could also accept the before-defrag priority, or any priority
> below NF_IP_PRI_CONNTRACK.

"Yes" (but I don't see why it should)

> [Severity: High]
> Can the nf_tables chain validation cache let a jumped-to chain skip this
> priority check?

Yes, this patch only works with this applied as well:

[PATCH nf-next] netfilter: nft_ct: validate hook and priority for ct zone set
https://lore.kernel.org/netfilter-devel/atDHof0kFWIWIf6-@strlen.de/T/#t

> nft_chain_vstate_valid() treats a regular chain as already validated using
> only the calling base chain's type, hooknum and the depth:
> 
> net/netfilter/nf_tables_api.c:nft_chain_vstate_valid() {
>     ...
> 	if (chain->vstate.depth >= ctx->level &&
> 	    chain->vstate.hook_mask[type] & BIT(hooknum))
> 		return true;
>     ...
> }
> 
> nft_chain_vstate_update() records only hook_mask[type] and depth, never the
> base chain priority. nft_table_validate() clears vstate only after it has
> walked every base chain, so cached results carry over from one base chain
> to the next.

Yes, the above linked patch did add two prio fields to the vstate for this.

>   base chain A: type filter, hook prerouting, priority -300, rule "jump X"
>   base chain B: type filter, hook prerouting, priority 0, rule "jump X"
>   regular chain X: an nft_compat rule using the CT target
> 
> Chain A is created first. Validating A goes like this:
> 
> nft_table_validate()
>   nft_chain_validate(A)
>     nft_immediate_validate()            /* level 1 */
>       nft_chain_validate(X)
>         nft_target_validate()
>           nft_compat_chain_validate_dependency()  /* passes, A is -300 */
>         nft_chain_vstate_update()       /* FILTER/PRE_ROUTING, depth 1 */

With the vstate patch, this should now have vstate change set to -300
for min and max priorities.

> Then for B:
> 
>   nft_chain_validate(B)
>     nft_immediate_validate()            /* level 1 */
>       nft_chain_validate(X)
>         nft_chain_vstate_valid()        /* true */
>         return 0;
> 
> So the new check never runs with B as ctx->chain, and the ruleset is
> accepted.

With vstate patch, this would fail the vstate_valid test because prio 0
is larger than previous prio_max validation (-300).

> At runtime, nf_conntrack_in() can leave skb->_nfct == 0 for an invalid
> packet. It also clears any raw-table template before tracking, so an
> unconditional jump in A doesn't help. B then jumps to X after conntrack,
> and xt_ct_target() attaches the template:
> 
> 	if (skb->_nfct != 0)
> 		return XT_CONTINUE;
> 
> 	if (ct) {
> 		refcount_inc(&ct->ct_general.use);
> 		nf_ct_set(skb, ct, IP_CT_NEW);
> 
> Isn't this the same "after the real conntrack hook has already run" case
> that the commit message says this patch closes? It can be reached with
> CAP_NET_ADMIN in an unprivileged user namespace.

Yes.

> Could the vstate key be made priority-aware (or carry a pre-conntrack
> bit)?

Yes, patch is on ML.  But even if I had placed the vstate patch in this
one, its still incomplete because nft_ct allows template attachmnent as
well.

Both patches are needed.

> Other options would be to skip the cache for chains that hold compat
> expressions with a table dependency, or to run this check for every base
> chain that can reach the rule.

Would work if nft_ct.c would not exist.