Re: [PATCH net v2 12/15] selftests: mptcp: lib: get counters for the right test

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Insecure use of predictable temporary file paths in /tmp allows local privilege escalation via a symlink attack (TOCTOU).
--

commit 706f0812739080817c63b8f13d5f3a60fbeddf0a
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

selftests: mptcp: lib: get counters for the right test

This commit updates the mptcp_lib_get_counter function to use nstat history
instead of absolute counters when extracting MIB values. It renames the
previous variable to cache and introduces a new history file variable.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index da1da414c30f2..b9d14647f4019 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh

[ ... ]

> @@ -416,19 +416,21 @@ mptcp_lib_nstat_get() {
>  }
>  
>  # $1: ns, $2: MIB counter
> -# Get the counter from the history (mptcp_lib_nstat_{init,get}()) if available.
> -# If not, get the counter from nstat ignoring any history.
> +# Get the counter from the cache (mptcp_lib_nstat_{init,get}()) if available.
> +# If not, get the counter from nstat ignoring any cache, but using the history.
>  mptcp_lib_get_counter() {
>  	local ns="${1}"
>  	local counter="${2}"
> -	local hist="/tmp/${ns}.out"
> +	local cache="/tmp/${ns}.out"
> +	local hist="/tmp/${ns}.nstat"

[Severity: High]
This isn't a bug introduced by this patch, but was already present in the
script for .out and .nstat files. Is there a risk of a local privilege
escalation via a symlink attack here?

Because the file path is constructed in mptcp_lib_get_counter() using the
globally visible namespace name, an unprivileged attacker could monitor
/var/run/netns/ for new network namespace names and preemptively create a
symlink at /tmp/${ns}.nstat pointing to a sensitive file like /etc/shadow.

>  	local count
>  
> -	if [[ -s "${hist}" && "${counter}" == *"Tcp"* ]]; then
> -		count=$(awk "/^${counter} / {print \$2; exit}" "${hist}")
> +	if [[ -s "${cache}" && "${counter}" == *"Tcp"* ]]; then
> +		count=$(awk "/^${counter} / {print \$2; exit}" "${cache}")
>  	else
> -		count=$(ip netns exec "${ns}" nstat -asz "${counter}" |
> -			awk 'NR==1 {next} {print $2}')
> +		count=$(NSTAT_HISTORY="${hist}" ip netns exec "${ns}" \
> +			nstat -sz "${counter}" |
> +				awk 'NR==1 {next} {print $2}')

[Severity: High]
When nstat is executed here as root with NSTAT_HISTORY pointing to the
predictable /tmp/ path, could it follow the attacker's symlink and
overwrite the target file?

>  	fi
>  	if [ -z "${count}" ]; then
>  		mptcp_lib_fail_if_expected_feature "${counter} counter"

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-net-mptcp-misc-fixes-7-3-rc1-v2-0-df1de70348b6@kernel.org?part=12