Re: [PATCH] netfilter: bpf: disallow conntrack kfuncs for token programs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Security restriction bypass in `nf_conntrack_kfunc_filter` due to unrelocated BTF ID comparison.

Pre-existing issues:
- [High] Token-loaded BPF programs can bypass namespace isolation via `bpf_xdp_get_xfrm_state`.
--

--- Patch [1]: [PATCH] netfilter: bpf: disallow conntrack kfuncs for token programs ---
commit 392473e378423f74e738047589faa01940a34fa2
Author: Jérémy Jean <[email protected]>

netfilter: bpf: disallow conntrack kfuncs for token programs

BPF tokens delegate BPF and network-admin capability checks to the token
owning user namespace. Conntrack kfuncs nevertheless accept a network
namespace ID relative to the program context without checking whether the
token has authority over the resolved namespace. This patch adds a filter
to conservatively reject the conntrack kfunc set for token-loaded programs.

> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index c2df7c9482812..4075ee1efa8ef 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -532,9 +532,24 @@ BTF_ID_FLAGS(func, bpf_ct_set_status)
>  BTF_ID_FLAGS(func, bpf_ct_change_status)
>  BTF_KFUNCS_END(nf_ct_kfunc_set)
>  
> +static int nf_conntrack_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> +	/*
> +	 * Conntrack kfuncs accept a netns ID relative to the program context.
> +	 * The verifier cannot determine which user namespace owns that target.
> +	 * Do not let a token delegate authority over arbitrary peer netns state.
> +	 */
> +	if (prog->aux->token &&
> +	    btf_id_set8_contains(&nf_ct_kfunc_set, kfunc_id))

[Severity: High]
Does btf_id_set8_contains() work correctly here when comparing the IDs?

If the kernel is built with distilled base BTF, the statically allocated
nf_ct_kfunc_set array remains unmodified and contains small, unrelocated
IDs. Since the kfunc_id provided by the verifier is a relocated runtime BTF
ID, will this comparison always fail and incorrectly return 0? If so, this
would bypass the intended namespace isolation security fix.

Also, is the btf_id_set8_contains() check necessary? It seems
__btf_kfunc_is_allowed() only invokes the filter for kfuncs that have
already been matched against the module's relocated registered set.

> +		return -EACCES;
> +
> +	return 0;
> +}

[Severity: High]
This is a pre-existing issue, but a similar vulnerability seems to exist in
net/xfrm/xfrm_state_bpf.c for the bpf_xdp_get_xfrm_state kfunc.

net/xfrm/xfrm_state_bpf.c:bpf_xdp_get_xfrm_state() {
    ...
    net = get_net_ns_by_id(net, opts->netns_id);
    ...
}

This resolves arbitrary peer network namespaces without validating if the BPF
token has authority over them. Since xfrm_state_xdp_kfunc_set lacks a .filter
hook to reject token-loaded programs, can an unprivileged token-loaded XDP
program pass a netns_id to look up and access sensitive struct xfrm_state
objects from the host init_net?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1