Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The directory traversal logic fails to check directory ownership and fails open on `stat` error, allowing arbitrary code execution as root via an attacker-controlled ancestor directory. --
--- Patch [9]: [PATCH v6 9/9] selftests/ftrace: Walk up to find test.d/functions when a subdirectory is passed --- commit 71305ea3fc4b8a7294d098d4bcdd8624fd21ca70 Author: Wen Yang <[email protected]> selftests/ftrace: Walk up to find test.d/functions when a subdirectory is passed This commit modifies ftracetest to walk up the directory tree from a passed test directory until it finds a test.d/functions file. This allows monitor subdirectories to be used directly as the test root without needing dummy functions shims. > diff --git a/tools/testing/selftests/ftrace/ftracetest > b/tools/testing/selftests/ftrace/ftracetest > index 0a56bf209f6c0..8f9d9291bf4ce 100755 > --- a/tools/testing/selftests/ftrace/ftracetest > +++ b/tools/testing/selftests/ftrace/ftracetest [ ... ] > @@ -159,9 +159,29 @@ parse_opts() { # opts > if [ -n "$OPT_TEST_CASES" ]; then > TEST_CASES=$OPT_TEST_CASES > fi > - if [ -n "$OPT_TEST_DIR" -a -f "$OPT_TEST_DIR"/test.d/functions ]; then > - TOP_DIR=$OPT_TEST_DIR > - TEST_DIR=$TOP_DIR/test.d > + 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. 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 > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
