diff mbox series

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

Message ID 20230615152918.3484699-1-revest@chromium.org
State New
Headers
Series [nf] netfilter: conntrack: Avoid nf_ct_helper_hash uses after free |

Commit Message

Florent Revest June 15, 2023, 3:29 p.m. UTC 
  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.

Further netfilter modules like netfilter_conntrack_ftp don't check
whether nf_conntrack initialized correctly and call
nf_conntrack_helpers_register() which accesses the freed
nf_ct_helper_hash and causes a uaf.

This patch guards nf_conntrack_helper_register() from accessing
freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
use-after-free.

Cc: stable@vger.kernel.org
Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
Signed-off-by: Florent Revest <revest@chromium.org>
---
 net/netfilter/nf_conntrack_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Pablo Neira Ayuso June 20, 2023, 6:35 a.m. UTC | #1 
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.
> 
> Further netfilter modules like netfilter_conntrack_ftp don't check
> whether nf_conntrack initialized correctly and call
> nf_conntrack_helpers_register() which accesses the freed
> nf_ct_helper_hash and causes a uaf.
> 
> This patch guards nf_conntrack_helper_register() from accessing
> freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
> use-after-free.

How could this possibly happen?

nf_conntrack_ftp depends on nf_conntrack.

If nf_conntrack fails to load, how can nf_conntrack_ftp be loaded?

> Cc: stable@vger.kernel.org
> Fixes: 12f7a505331e ("netfilter: add user-space connection tracking helper infrastructure")
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  net/netfilter/nf_conntrack_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 0c4db2f2ac43..f22691f83853 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -360,6 +360,9 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
>  	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
>  	BUG_ON(strlen(me->name) > NF_CT_HELPER_NAME_LEN - 1);
>  
> +	if (!nf_ct_helper_hash)
> +		return -ENOENT;
> +
>  	if (me->expect_policy->max_expected > NF_CT_EXPECT_MAX_CNT)
>  		return -EINVAL;
>  
> @@ -515,4 +518,5 @@ int nf_conntrack_helper_init(void)
>  void nf_conntrack_helper_fini(void)
>  {
>  	kvfree(nf_ct_helper_hash);
> +	nf_ct_helper_hash = NULL;
>  }
> -- 
> 2.41.0.162.gfafddb0af9-goog
>
Florent Revest June 21, 2023, 10:20 a.m. UTC | #2 
On Tue, Jun 20, 2023 at 8:35 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> 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.
> >
> > Further netfilter modules like netfilter_conntrack_ftp don't check
> > whether nf_conntrack initialized correctly and call
> > nf_conntrack_helpers_register() which accesses the freed
> > nf_ct_helper_hash and causes a uaf.
> >
> > This patch guards nf_conntrack_helper_register() from accessing
> > freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
> > use-after-free.
>
> How could this possibly happen?

Here is one way to reproduce this bug:

  # Use nf/main
  git clone git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git
  cd nf

  # Start from a minimal config
  make LLVM=1 LLVM_IAS=0 defconfig

  # Enable KASAN, BTF and nf_conntrack_ftp
  scripts/config -e KASAN -e BPF_SYSCALL -e DEBUG_INFO -e
DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -e DEBUG_INFO_BTF -e
NF_CONNTRACK_FTP
  make LLVM=1 LLVM_IAS=0 olddefconfig

  # Build without the LLVM integrated assembler
  make LLVM=1 LLVM_IAS=0 -j `nproc`

(Note that the use of LLVM_IAS=0, KASAN and BTF is just to trigger a
bug in BTF that will be fixed by
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=9724160b3942b0a967b91a59f81da5593f28b8ba
Independently of that specific BTF bug, it shows how an error in
nf_conntrack_bpf can cause a boot-time uaf in netfilter)

Then, booting gives me:

[    4.624666] BPF: [13893] FUNC asan.module_ctor
[    4.625611] BPF: type_id=1
[    4.626176] BPF:
[    4.626601] BPF: Invalid name
[    4.627208] BPF:
[    4.627723] ==================================================================
[    4.628610] BUG: KASAN: slab-use-after-free in
nf_conntrack_helper_register+0x129/0x2f0
[    4.628610] Read of size 8 at addr ffff888102d24000 by task swapper/0/1
[    4.628610]
[    4.628610] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
6.4.0-rc4-00244-gab39b113e747 #47
[    4.628610] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[    4.628610] Call Trace:
[    4.628610]  <TASK>
[    4.636584] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
[    4.628610]  dump_stack_lvl+0x97/0xd0
[    4.638738] i2c i2c-0: 1/1 memory slots populated (from DMI)
[    4.628610]  print_report+0x17e/0x570
[    4.640118] i2c i2c-0: Memory type 0x07 not supported yet, not
instantiating SPD
[    4.628610]  ? __virt_addr_valid+0xe4/0x160
[    4.628610]  kasan_report+0x169/0x1a0
[    4.628610]  ? nf_conntrack_helper_register+0x129/0x2f0
[    4.628610]  nf_conntrack_helper_register+0x129/0x2f0
[    4.628610]  nf_conntrack_helpers_register+0x24/0x60
[    4.628610]  nf_conntrack_ftp_init+0x114/0x140
[    4.628610]  ? __pfx_nf_conntrack_ftp_init+0x10/0x10
[    4.628610]  do_one_initcall+0xe6/0x310
[    4.628610]  ? kasan_set_track+0x61/0x80
[    4.628610]  ? kasan_set_track+0x4f/0x80
[    4.628610]  ? __kasan_kmalloc+0x72/0x90
[    4.628610]  ? __kmalloc+0xa7/0x1a0
[    4.628610]  ? do_initcalls+0x1b/0x70
[    4.628610]  ? kernel_init_freeable+0x174/0x1e0
[    4.628610]  ? kernel_init+0x18/0x1b0
[    4.628610]  ? ret_from_fork+0x29/0x50
[    4.628610]  ? sysvec_apic_timer_interrupt+0xe/0x80
[    4.628610]  ? asm_sysvec_apic_timer_interrupt+0x1a/0x20
[    4.628610]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
[    4.628610]  ? next_arg+0x20b/0x250
[    4.628610]  ? strlen+0x21/0x40
[    4.628610]  ? parse_args+0xc7/0x5f0
[    4.628610]  do_initcall_level+0xa6/0x140
[    4.628610]  do_initcalls+0x3e/0x70
[    4.628610]  kernel_init_freeable+0x174/0x1e0
[    4.628610]  ? __pfx_kernel_init+0x10/0x10
[    4.628610]  kernel_init+0x18/0x1b0
[    4.628610]  ? __pfx_kernel_init+0x10/0x10
[    4.628610]  ret_from_fork+0x29/0x50
[    4.628610]  </TASK>
[    4.628610]
[    4.628610] Allocated by task 1:
[    4.628610]  kasan_set_track+0x4f/0x80
[    4.628610]  __kasan_kmalloc+0x72/0x90
[    4.628610]  __kmalloc_node+0xa7/0x190
[    4.628610]  kvmalloc_node+0x44/0x120
[    4.628610]  nf_ct_alloc_hashtable+0x5b/0xe0
[    4.628610]  nf_conntrack_helper_init+0x1f/0x60
[    4.628610]  nf_conntrack_init_start+0x1c9/0x2d0
[    4.628610]  nf_conntrack_standalone_init+0xb/0xa0
[    4.628610]  do_one_initcall+0xe6/0x310
[    4.628610]  do_initcall_level+0xa6/0x140
[    4.628610]  do_initcalls+0x3e/0x70
[    4.628610]  kernel_init_freeable+0x174/0x1e0
[    4.628610]  kernel_init+0x18/0x1b0
[    4.628610]  ret_from_fork+0x29/0x50
[    4.628610]
[    4.628610] Freed by task 1:
[    4.628610]  kasan_set_track+0x4f/0x80
[    4.628610]  kasan_save_free_info+0x2b/0x50
[    4.628610]  ____kasan_slab_free+0x116/0x1a0
[    4.628610]  __kmem_cache_free+0xc4/0x200
[    4.628610]  nf_conntrack_init_start+0x29c/0x2d0
[    4.628610]  nf_conntrack_standalone_init+0xb/0xa0
[    4.628610]  do_one_initcall+0xe6/0x310
[    4.628610]  do_initcall_level+0xa6/0x140
[    4.628610]  do_initcalls+0x3e/0x70
[    4.628610]  kernel_init_freeable+0x174/0x1e0
[    4.628610]  kernel_init+0x18/0x1b0
[    4.628610]  ret_from_fork+0x29/0x50
[    4.628610]
[    4.628610] The buggy address belongs to the object at ffff888102d24000
[    4.628610]  which belongs to the cache kmalloc-4k of size 4096
[    4.628610] The buggy address is located 0 bytes inside of
[    4.628610]  freed 4096-byte region [ffff888102d24000, ffff888102d25000)
[    4.628610]
[    4.628610] The buggy address belongs to the physical page:
[    4.628610] page:000000001eb64ba1 refcount:1 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x102d20
[    4.628610] head:000000001eb64ba1 order:3 entire_mapcount:0
nr_pages_mapped:0 pincount:0
[    4.628610] flags: 0x200000000010200(slab|head|node=0|zone=2)
[    4.628610] page_type: 0xffffffff()
[    4.628610] raw: 0200000000010200 ffff888100043040 dead000000000122
0000000000000000
[    4.628610] raw: 0000000000000000 0000000000040004 00000001ffffffff
0000000000000000
[    4.628610] page dumped because: kasan: bad access detected
...

> 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 ?
Pablo Neira Ayuso June 21, 2023, 11:12 a.m. UTC | #3 
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:
> >
> > 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.
> > >
> > > Further netfilter modules like netfilter_conntrack_ftp don't check
> > > whether nf_conntrack initialized correctly and call
> > > nf_conntrack_helpers_register() which accesses the freed
> > > nf_ct_helper_hash and causes a uaf.
> > >
> > > This patch guards nf_conntrack_helper_register() from accessing
> > > freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
> > > use-after-free.
> >
> > How could this possibly happen?
> 
> Here is one way to reproduce this bug:
> 
>   # Use nf/main
>   git clone git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git
>   cd nf
> 
>   # Start from a minimal config
>   make LLVM=1 LLVM_IAS=0 defconfig
> 
>   # Enable KASAN, BTF and nf_conntrack_ftp
>   scripts/config -e KASAN -e BPF_SYSCALL -e DEBUG_INFO -e
> DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -e DEBUG_INFO_BTF -e
> NF_CONNTRACK_FTP
>   make LLVM=1 LLVM_IAS=0 olddefconfig
> 
>   # Build without the LLVM integrated assembler
>   make LLVM=1 LLVM_IAS=0 -j `nproc`
> 
> (Note that the use of LLVM_IAS=0, KASAN and BTF is just to trigger a
> bug in BTF that will be fixed by
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=9724160b3942b0a967b91a59f81da5593f28b8ba
> Independently of that specific BTF bug, it shows how an error in
> nf_conntrack_bpf can cause a boot-time uaf in netfilter)
> 
> Then, booting gives me:
> 
> [    4.624666] BPF: [13893] FUNC asan.module_ctor
> [    4.625611] BPF: type_id=1
> [    4.626176] BPF:
> [    4.626601] BPF: Invalid name
> [    4.627208] BPF:
> [    4.627723] ==================================================================
> [    4.628610] BUG: KASAN: slab-use-after-free in
> nf_conntrack_helper_register+0x129/0x2f0
> [    4.628610] Read of size 8 at addr ffff888102d24000 by task swapper/0/1
> [    4.628610]
> [    4.628610] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 6.4.0-rc4-00244-gab39b113e747 #47
> [    4.628610] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS 1.16.0-debian-1.16.0-5 04/01/2014
> [    4.628610] Call Trace:
> [    4.628610]  <TASK>
> [    4.636584] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
> [    4.628610]  dump_stack_lvl+0x97/0xd0
> [    4.638738] i2c i2c-0: 1/1 memory slots populated (from DMI)
> [    4.628610]  print_report+0x17e/0x570
> [    4.640118] i2c i2c-0: Memory type 0x07 not supported yet, not
> instantiating SPD
> [    4.628610]  ? __virt_addr_valid+0xe4/0x160
> [    4.628610]  kasan_report+0x169/0x1a0
> [    4.628610]  ? nf_conntrack_helper_register+0x129/0x2f0
> [    4.628610]  nf_conntrack_helper_register+0x129/0x2f0
> [    4.628610]  nf_conntrack_helpers_register+0x24/0x60
> [    4.628610]  nf_conntrack_ftp_init+0x114/0x140
> [    4.628610]  ? __pfx_nf_conntrack_ftp_init+0x10/0x10
> [    4.628610]  do_one_initcall+0xe6/0x310
> [    4.628610]  ? kasan_set_track+0x61/0x80
> [    4.628610]  ? kasan_set_track+0x4f/0x80
> [    4.628610]  ? __kasan_kmalloc+0x72/0x90
> [    4.628610]  ? __kmalloc+0xa7/0x1a0
> [    4.628610]  ? do_initcalls+0x1b/0x70
> [    4.628610]  ? kernel_init_freeable+0x174/0x1e0
> [    4.628610]  ? kernel_init+0x18/0x1b0
> [    4.628610]  ? ret_from_fork+0x29/0x50
> [    4.628610]  ? sysvec_apic_timer_interrupt+0xe/0x80
> [    4.628610]  ? asm_sysvec_apic_timer_interrupt+0x1a/0x20
> [    4.628610]  ? __pfx_ignore_unknown_bootoption+0x10/0x10
> [    4.628610]  ? next_arg+0x20b/0x250
> [    4.628610]  ? strlen+0x21/0x40
> [    4.628610]  ? parse_args+0xc7/0x5f0
> [    4.628610]  do_initcall_level+0xa6/0x140
> [    4.628610]  do_initcalls+0x3e/0x70
> [    4.628610]  kernel_init_freeable+0x174/0x1e0
> [    4.628610]  ? __pfx_kernel_init+0x10/0x10
> [    4.628610]  kernel_init+0x18/0x1b0
> [    4.628610]  ? __pfx_kernel_init+0x10/0x10
> [    4.628610]  ret_from_fork+0x29/0x50
> [    4.628610]  </TASK>
> [    4.628610]
> [    4.628610] Allocated by task 1:
> [    4.628610]  kasan_set_track+0x4f/0x80
> [    4.628610]  __kasan_kmalloc+0x72/0x90
> [    4.628610]  __kmalloc_node+0xa7/0x190
> [    4.628610]  kvmalloc_node+0x44/0x120
> [    4.628610]  nf_ct_alloc_hashtable+0x5b/0xe0
> [    4.628610]  nf_conntrack_helper_init+0x1f/0x60
> [    4.628610]  nf_conntrack_init_start+0x1c9/0x2d0
> [    4.628610]  nf_conntrack_standalone_init+0xb/0xa0
> [    4.628610]  do_one_initcall+0xe6/0x310
> [    4.628610]  do_initcall_level+0xa6/0x140
> [    4.628610]  do_initcalls+0x3e/0x70
> [    4.628610]  kernel_init_freeable+0x174/0x1e0
> [    4.628610]  kernel_init+0x18/0x1b0
> [    4.628610]  ret_from_fork+0x29/0x50
> [    4.628610]
> [    4.628610] Freed by task 1:
> [    4.628610]  kasan_set_track+0x4f/0x80
> [    4.628610]  kasan_save_free_info+0x2b/0x50
> [    4.628610]  ____kasan_slab_free+0x116/0x1a0
> [    4.628610]  __kmem_cache_free+0xc4/0x200
> [    4.628610]  nf_conntrack_init_start+0x29c/0x2d0
> [    4.628610]  nf_conntrack_standalone_init+0xb/0xa0
> [    4.628610]  do_one_initcall+0xe6/0x310
> [    4.628610]  do_initcall_level+0xa6/0x140
> [    4.628610]  do_initcalls+0x3e/0x70
> [    4.628610]  kernel_init_freeable+0x174/0x1e0
> [    4.628610]  kernel_init+0x18/0x1b0
> [    4.628610]  ret_from_fork+0x29/0x50
> [    4.628610]
> [    4.628610] The buggy address belongs to the object at ffff888102d24000
> [    4.628610]  which belongs to the cache kmalloc-4k of size 4096
> [    4.628610] The buggy address is located 0 bytes inside of
> [    4.628610]  freed 4096-byte region [ffff888102d24000, ffff888102d25000)
> [    4.628610]
> [    4.628610] The buggy address belongs to the physical page:
> [    4.628610] page:000000001eb64ba1 refcount:1 mapcount:0
> mapping:0000000000000000 index:0x0 pfn:0x102d20
> [    4.628610] head:000000001eb64ba1 order:3 entire_mapcount:0
> nr_pages_mapped:0 pincount:0
> [    4.628610] flags: 0x200000000010200(slab|head|node=0|zone=2)
> [    4.628610] page_type: 0xffffffff()
> [    4.628610] raw: 0200000000010200 ffff888100043040 dead000000000122
> 0000000000000000
> [    4.628610] raw: 0000000000000000 0000000000040004 00000001ffffffff
> 0000000000000000
> [    4.628610] page dumped because: kasan: bad access detected
> ...
> 
> > 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.

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.
Florian Westphal June 21, 2023, 11:14 a.m. UTC | #4 
Florent Revest <revest@chromium.org> wrote:
> On Tue, Jun 20, 2023 at 8:35 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > 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.
> > >
> > > Further netfilter modules like netfilter_conntrack_ftp don't check
> > > whether nf_conntrack initialized correctly and call
> > > nf_conntrack_helpers_register() which accesses the freed
> > > nf_ct_helper_hash and causes a uaf.
> > >
> > > This patch guards nf_conntrack_helper_register() from accessing
> > > freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
> > > use-after-free.
> >
> > How could this possibly happen?
> 
> Here is one way to reproduce this bug:
> 
>   # Use nf/main
>   git clone git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git
>   cd nf
> 
>   # Start from a minimal config
>   make LLVM=1 LLVM_IAS=0 defconfig
> 
>   # Enable KASAN, BTF and nf_conntrack_ftp
>   scripts/config -e KASAN -e BPF_SYSCALL -e DEBUG_INFO -e
> DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -e DEBUG_INFO_BTF -e
> NF_CONNTRACK_FTP
>   make LLVM=1 LLVM_IAS=0 olddefconfig
> 
>   # Build without the LLVM integrated assembler
>   make LLVM=1 LLVM_IAS=0 -j `nproc`
> 
> (Note that the use of LLVM_IAS=0, KASAN and BTF is just to trigger a
> bug in BTF that will be fixed by
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=9724160b3942b0a967b91a59f81da5593f28b8ba
> Independently of that specific BTF bug, it shows how an error in
> nf_conntrack_bpf can cause a boot-time uaf in netfilter)
> 
> Then, booting gives me:
> 
> [    4.624666] BPF: [13893] FUNC asan.module_ctor
> [    4.625611] BPF: type_id=1
> [    4.626176] BPF:
> [    4.626601] BPF: Invalid name
> [    4.627208] BPF:
> [    4.627723] ==================================================================
> [    4.628610] BUG: KASAN: slab-use-after-free in
> nf_conntrack_helper_register+0x129/0x2f0
> [    4.628610] Read of size 8 at addr ffff888102d24000 by task swapper/0/1
> [    4.628610]

Isn't that better than limping along?

in this case an initcall is failing and I think panic is preferrable
to a kernel that behaves like NF_CONNTRACK_FTP=n.

AFAICS this problem is specific to NF_CONNTRACK_FTP=y
(or any other helper module, for that matter).

If you disagree please resend with a commit message that
makes it clear that this is only relevant for the 'builtin' case.
Florent Revest June 21, 2023, 12:41 p.m. UTC | #5 
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.
Florent Revest June 21, 2023, 1:07 p.m. UTC | #6 
On Wed, Jun 21, 2023 at 1:14 PM Florian Westphal <fw@strlen.de> wrote:
>
> Florent Revest <revest@chromium.org> wrote:
> > On Tue, Jun 20, 2023 at 8:35 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> > > 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.
> > > >
> > > > Further netfilter modules like netfilter_conntrack_ftp don't check
> > > > whether nf_conntrack initialized correctly and call
> > > > nf_conntrack_helpers_register() which accesses the freed
> > > > nf_ct_helper_hash and causes a uaf.
> > > >
> > > > This patch guards nf_conntrack_helper_register() from accessing
> > > > freed/uninitialized nf_ct_helper_hash maps and fixes a boot-time
> > > > use-after-free.
> > >
> > > How could this possibly happen?
> >
> > Here is one way to reproduce this bug:
> >
> >   # Use nf/main
> >   git clone git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git
> >   cd nf
> >
> >   # Start from a minimal config
> >   make LLVM=1 LLVM_IAS=0 defconfig
> >
> >   # Enable KASAN, BTF and nf_conntrack_ftp
> >   scripts/config -e KASAN -e BPF_SYSCALL -e DEBUG_INFO -e
> > DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -e DEBUG_INFO_BTF -e
> > NF_CONNTRACK_FTP
> >   make LLVM=1 LLVM_IAS=0 olddefconfig
> >
> >   # Build without the LLVM integrated assembler
> >   make LLVM=1 LLVM_IAS=0 -j `nproc`
> >
> > (Note that the use of LLVM_IAS=0, KASAN and BTF is just to trigger a
> > bug in BTF that will be fixed by
> > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=9724160b3942b0a967b91a59f81da5593f28b8ba
> > Independently of that specific BTF bug, it shows how an error in
> > nf_conntrack_bpf can cause a boot-time uaf in netfilter)
> >
> > Then, booting gives me:
> >
> > [    4.624666] BPF: [13893] FUNC asan.module_ctor
> > [    4.625611] BPF: type_id=1
> > [    4.626176] BPF:
> > [    4.626601] BPF: Invalid name
> > [    4.627208] BPF:
> > [    4.627723] ==================================================================
> > [    4.628610] BUG: KASAN: slab-use-after-free in
> > nf_conntrack_helper_register+0x129/0x2f0
> > [    4.628610] Read of size 8 at addr ffff888102d24000 by task swapper/0/1
> > [    4.628610]
>
> Isn't that better than limping along?

Note that this only panics because KASAN instrumentation notices the
use-after-free and makes a lot of noise about it. In a non-debug boot,
this would just silently corrupt random memory instead.

> in this case an initcall is failing and I think panic is preferrable
> to a kernel that behaves like NF_CONNTRACK_FTP=n.

In that case, it seems like what you'd want is
nf_conntrack_standalone_init() to BUG() instead of returning an error
then ? (so you'd never get to NF_CONNTRACK_FTP or any other if
nf_conntrack failed to initialize) If this is the prefered behavior,
then sure, why not.

> AFAICS this problem is specific to NF_CONNTRACK_FTP=y
> (or any other helper module, for that matter).

Even with NF_CONNTRACK_FTP=m, the initialization failure in
nf_conntrack_standalone_init() still happens. Therefore, the helper
hashtable gets freed and when the nf_conntrack_ftp.ko module gets
insmod-ed, it calls nf_conntrack_helpers_register() and this still
causes a use-after-free.

> If you disagree please resend with a commit message that
> makes it clear that this is only relevant for the 'builtin' case.
Florian Westphal June 21, 2023, 6:47 p.m. UTC | #7 
Florent Revest <revest@chromium.org> wrote:
> > in this case an initcall is failing and I think panic is preferrable
> > to a kernel that behaves like NF_CONNTRACK_FTP=n.
> 
> In that case, it seems like what you'd want is
> nf_conntrack_standalone_init() to BUG() instead of returning an error
> then ? (so you'd never get to NF_CONNTRACK_FTP or any other if
> nf_conntrack failed to initialize) If this is the prefered behavior,
> then sure, why not.
> 
> > AFAICS this problem is specific to NF_CONNTRACK_FTP=y
> > (or any other helper module, for that matter).
> 
> Even with NF_CONNTRACK_FTP=m, the initialization failure in
> nf_conntrack_standalone_init() still happens. Therefore, the helper
> hashtable gets freed and when the nf_conntrack_ftp.ko module gets
> insmod-ed, it calls nf_conntrack_helpers_register() and this still
> causes a use-after-free.

Can you send a v2 with a slightly reworded changelog?

It should mention that one needs NF_CONNTRACK=y, so that when
the failure happens during the initcall (as oposed to module insertion),
nf_conntrack_helpers_register() can fail cleanly without followup splat?

Thanks.
Florent Revest July 3, 2023, 2:42 p.m. UTC | #8 
On Wed, Jun 21, 2023 at 8:47 PM Florian Westphal <fw@strlen.de> wrote:
>
> Florent Revest <revest@chromium.org> wrote:
> > > in this case an initcall is failing and I think panic is preferrable
> > > to a kernel that behaves like NF_CONNTRACK_FTP=n.
> >
> > In that case, it seems like what you'd want is
> > nf_conntrack_standalone_init() to BUG() instead of returning an error
> > then ? (so you'd never get to NF_CONNTRACK_FTP or any other if
> > nf_conntrack failed to initialize) If this is the prefered behavior,
> > then sure, why not.
> >
> > > AFAICS this problem is specific to NF_CONNTRACK_FTP=y
> > > (or any other helper module, for that matter).
> >
> > Even with NF_CONNTRACK_FTP=m, the initialization failure in
> > nf_conntrack_standalone_init() still happens. Therefore, the helper
> > hashtable gets freed and when the nf_conntrack_ftp.ko module gets
> > insmod-ed, it calls nf_conntrack_helpers_register() and this still
> > causes a use-after-free.
>
> Can you send a v2 with a slightly reworded changelog?
>
> It should mention that one needs NF_CONNTRACK=y, so that when
> the failure happens during the initcall (as oposed to module insertion),
> nf_conntrack_helpers_register() can fail cleanly without followup splat?

Sure! :) On it.
diff mbox series

Patch

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 0c4db2f2ac43..f22691f83853 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -360,6 +360,9 @@  int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
 	BUG_ON(strlen(me->name) > NF_CT_HELPER_NAME_LEN - 1);
 
+	if (!nf_ct_helper_hash)
+		return -ENOENT;
+
 	if (me->expect_policy->max_expected > NF_CT_EXPECT_MAX_CNT)
 		return -EINVAL;
 
@@ -515,4 +518,5 @@  int nf_conntrack_helper_init(void)
 void nf_conntrack_helper_fini(void)
 {
 	kvfree(nf_ct_helper_hash);
+	nf_ct_helper_hash = NULL;
 }