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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 6 Jul 2024 09:42:00 +0800
From: Chengen Du <chengen.du@...onical.com>
To: Florian Westphal <fw@...len.de>
Cc: Michal Kubiak <michal.kubiak@...el.com>, jhs@...atatu.com, xiyou.wangcong@...il.com, 
	jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, ozsh@...dia.com, paulb@...dia.com, 
	marcelo.leitner@...il.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Gerald Yang <gerald.yang@...onical.com>
Subject: Re: [PATCH net v2] net/sched: Fix UAF when resolving a clash

On Fri, Jul 5, 2024 at 5:35 PM Florian Westphal <fw@...len.de> wrote:
>
> Michal Kubiak <michal.kubiak@...el.com> 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".