Re: [PATCH nf] netfilter: conntrack: Avoid nf_ct_helper_hash uses after free

[Florent Revest <revest@chromium.org>]

On Wed, Jun 21, 2023 at 1:12 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Jun 21, 2023 at 12:20:44PM +0200, Florent Revest wrote:
> > On Tue, Jun 20, 2023 at 8:35 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > nf_conntrack_ftp depends on nf_conntrack.
> > >
> > > If nf_conntrack fails to load, how can nf_conntrack_ftp be loaded?
> >
> > Is this maybe only true of dynamically loaded kmods ? With
> > CONFIG_NF_CONNTRACK_FTP=y, it seems to me that nf_conntrack_ftp_init()
> > will be called as an __init function, independently of whether
> > nf_conntrack_init_start() succeeded or not. Am I missing something ?
>
> No idea, nf_conntrack init path invokes nf_conntrack_helper_init()
> which initializes the helper hashtable.

Yes

> How is it that you can nf_conntrack_helpers_register() call before the
> initialization path of nf_conntrack is run, that I don't know.

No, this is not what happens. I tried to describe the problem in the
following paragraph (I'm open to suggestions on how to explain this
better!)

> > > On Thu, Jun 15, 2023 at 05:29:18PM +0200, Florent Revest wrote:
> > > > If register_nf_conntrack_bpf() fails (for example, if the .BTF section
> > > > contains an invalid entry), nf_conntrack_init_start() calls
> > > > nf_conntrack_helper_fini() as part of its cleanup path and
> > > > nf_ct_helper_hash gets freed.

Said differently (chronologically, I hope that helps)

First, nf_conntrack_init_start() runs:
- it calls nf_conntrack_helper_init() (this succeeds and initializes
the hashmap)
- it calls register_nf_conntrack_bpf() (this fails)
- goto err_kfunc
- it calls nf_conntrack_helper_fini() (this frees the hashmap and
leaves a dangling nf_ct_helper_hash pointer)
- it returns back an error such that nf_conntrack_standalone_init() fails

At this point, the builtin nf_conntrack module failed to load.

But now, nf_conntrack_ftp_init() also runs:
- it calls nf_conntrack_helpers_register()
  - this calls nf_conntrack_helper_register()
    - this accesses the hashmap pointer even though the hashmap has
been freed already. That's where the use-after-free is.

I proposed we fix this by not accessing a freed hashmap (using NULL as
a clear indication that this is now an invalid pointer) but I suppose
there are other ways one could go about it such as checking if
nf_conntrack initialized successfully early in nf_conntrack_ftp_init()
etc... I'm open to suggestions.