Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> Florian Westphal says:
>
> "Problem is that after the helper hook was merged back into the confirm
> one, the queueing itself occurs from the confirm hook, i.e. we queue
> from the last netfilter callback in the hook-list.
>
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
>
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
>
> To fix this there are several ways:
> 1. revert the 'Fixes' commit and have a extra helper hook again.
> Works, but has the drawback of adding another indirect call for
> everyone.
>
> 2. Special case this: split the hooks only when userspace helper
> gets added, so queueing occurs at a lower priority again,
> and normal nqueue reinject would eventually call the last hook.
>
> 3. Extend the existing nf_queue ct update hook to allow a forced
> confirmation (plus run the seqadj code).
>
> This goes for 3)."
>
> Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> + * processing after the helper invocation in nf_confirm().
> + */
> +static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
> + enum ip_conntrack_info ctinfo)
> +{
> + const struct nf_conntrack_helper *helper;
> + const struct nf_conn_help *help;
> + unsigned int protoff;
> +
> + help = nfct_help(ct);
> + if (!help)
> + return 0;
> +
> + helper = rcu_dereference(help->helper);
> + if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
> + return 0;
Relying on this check means that in case of
... -j CT (assign userspace helper)
... -j NFQUEUE
> + /* We've seen it coming out the other side: confirm it */
> + return nf_conntrack_confirm(skb) == NF_DROP ? - 1 : 0;
> +}
> +static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
> +{
[..]
> + err = nf_confirm_cthelper(skb, ct, ctinfo);
> + if (err < 0)
> + return err;
> +
> + if (nf_ct_is_confirmed(ct))
> + return 0;
This means that in case of userspace helper, we return here
any bypass the __nf_conntrack_update logic.
I don't think thats a problem either given the userspace
helper presence, so
Acked-by: Florian Westphal <fw@xxxxxxxxx>
