Re: [PATCH net v2] net/sched: Fix UAF when resolving a clash

[Chengen Du]

On Fri, Jul 5, 2024 at 5:35 PM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Michal Kubiak <michal.kubiak@xxxxxxxxx> wrote:
> > On Fri, Jul 05, 2024 at 10:50:56AM +0800, Chengen Du wrote:
> > The ct may be dropped if a clash has been resolved but is still passed to
> > > the tcf_ct_flow_table_process_conn function for further usage. This issue
> > > can be fixed by retrieving ct from skb again after confirming conntrack.
>
> Right, ct can be stale after confirm.
>
> > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > > index 2a96d9c1db65..6f41796115e3 100644
> > > --- a/net/sched/act_ct.c
> > > +++ b/net/sched/act_ct.c
> > > @@ -1077,6 +1077,14 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
> > >              */
> > >             if (nf_conntrack_confirm(skb) != NF_ACCEPT)
> > >                     goto drop;
> > > +
> > > +           /* The ct may be dropped if a clash has been resolved,
> > > +            * so it's necessary to retrieve it from skb again to
> > > +            * prevent UAF.
> > > +            */
> > > +           ct = nf_ct_get(skb, &ctinfo);
> > > +           if (!ct)
> > > +                   goto drop;
> >
> > After taking a closer look at this change, I have a question: Why do we
> > need to change an action returned by "nf_conntrack_confirm()"
> > (NF_ACCEPT) and actually perform the flow for NF_DROP?
> >
> > From the commit message I understand that you only want to prevent
> > calling "tcf_ct_flow_table_process_conn()". But for such reason we have
> > a bool variable: "skip_add".
> > Shouldn't we just set "skip_add" to true to prevent the UAF?
> > Would the following example code make sense in this case?
> >
> >       ct = nf_ct_get(skb, &ctinfo);
> >       if (!ct)
> >               skip_add = true;

The fix is followed by the KASAN analysis. The ct is freed while
resolving a clash in the __nf_ct_resolve_clash function, but it is
still accessed in the tcf_ct_flow_table_process_conn function. If I
understand correctly, the original logic still adds the ct to the flow
table after resolving a clash once the skip_add is false. The chance
of encountering a drop case is rare because the skb's ct is already
substituted into the hashes one. However, if we still encounter a NULL
ct, the situation is unusual and might warrant dropping it as a
precaution. I am not an expert in this area and might have some
misunderstandings. Please share your opinions if you have any
concerns.

>
> It depends on what tc wants do to here.
>
> For netfilter, the skb is not dropped and continues passing
> through the stack. Its up to user to decide what to do with it,
> e.g. doing "ct state invalid drop".