On Thu, 2026-08-20 at 16:58 +0000, [email protected] wrote: > > + if [ -n "$OPT_TEST_DIR" ]; then > > + # Walk up from OPT_TEST_DIR to find the nearest ancestor that contains > > + # test.d/functions. This allows a monitor subdirectory (e.g. > > + # verification/test.d/tlob/) to be passed directly without placing a > > + # dummy functions shim in each new subdirectory. > > + # Security: only walk within directories owned by root or the current > > user > > + # to prevent sourcing a functions file planted in a world-writable > > ancestor. > > + dir=$(realpath "$OPT_TEST_DIR" 2>/dev/null || echo "$OPT_TEST_DIR") > > + while [ "$dir" != "/" ] && [ "$dir" != "//" ]; do > > + if [ -f "$dir/test.d/functions" ]; then > > + # Verify the directory is not world-writable to prevent LPE. > > + dirperms=$(stat -c "%a" "$dir" 2>/dev/null || stat -f "%Lp" "$dir" > > 2>/dev/null) > > + case "$dirperms" in > > + *2|*3|*6|*7) ;; # world-writable: skip this candidate > > + *) > > + TOP_DIR=$dir > > [Severity: High] > Does this logic actually check directory ownership as mentioned in the > comments? > > It appears the code only checks if the directory is world-writable, but omits > validating that it is owned by root or the current user. An attacker could > potentially create a directory with standard 0755 permissions, plant a > malicious test.d/functions script, and if a privileged user or automated test > harness runs ftracetest against a subdirectory, the script would traverse up > and source the payload.
I'm wondering if this is a broad example of root can shoot themselves in the foot. This is a selftest run by a root user, I believe they are responsible to validate what's running isn't malicious. I cannot picture a case where a path is commonly world writable here. I would keep things simple and perhaps walk up at a maximum of 4-5 levels (tools/testing/selftests/verification/test.d), to kinda make sure we never go out of the repository. You cannot really rely on git (may run from a tarball) or other static methods, but I believe 4-5 levels up will cover us just fine. In my opinion all this stat/dirperms thing is rarely useful, you could just write the concern in a *brief* comment, saying we trust 4-5 level ups are still in the repo and we trust the repo, just to make AIs happy. What do you think? Gabriele > Additionally, if the stat command fails for any reason, dirperms will be > empty. > Would an empty string match the *) fallback case and cause the script to > silently > fail open, accepting even a world-writable directory? > > > + TEST_DIR=$TOP_DIR/test.d > > + break > > + ;; > > + esac > > + fi > > + dir=$(dirname "$dir") > > + done > > fi > > }
