Command injection in filesystem.py: nine shell commands interpolate user paths, zero shlex.quote #154

Summary

clustrix/filesystem.py builds nine remote shell commands by f-string interpolation of caller-supplied paths and glob patterns, and contains no shlex.quote at all:

$ grep -c shlex clustrix/filesystem.py
0

423:  cmd = f"ls -1 {full_path} 2>/dev/null || true"
437:  cmd = f"cd {full_path} && find . -name '{pattern}' -type f | sed 's|^\\./||' | sort"
452:  cmd = f"stat -c '%s %Y %f' {full_path} 2>/dev/null"
483:  cmd = f"test -e {full_path} && echo 'EXISTS' || echo 'NOT_EXISTS'"
494:  cmd = f"test -d {full_path} && echo 'DIR' || echo 'NOT_DIR'"
505:  cmd = f"test -f {full_path} && echo 'FILE' || echo 'NOT_FILE'"
518:  cmd = f"cd {full_path} && ls -d {pattern} 2>/dev/null | sort || true"
553:  cmd = f"find {full_path} -type f 2>/dev/null | wc -l"
556:  cmd = f"find {full_path} -name '{pattern}' -type f 2>/dev/null | wc -l"

Nothing sanitizes the input on the way in. _get_full_path (filesystem.py:232) only joins the caller's string to a base directory and returns it:

if os.path.isabs(path):
    return path
return os.path.join(base_dir, path)

This is public API

Every one of these is reachable through a documented, exported convenience function: cluster_ls, cluster_find, cluster_stat, cluster_exists, cluster_isdir, cluster_isfile, cluster_glob, cluster_du, cluster_count_files — all in clustrix.__all__ and all documented in docs/source/api/filesystem.rst.

A path or pattern carrying shell metacharacters runs as a command on the cluster, as the user, with their credentials. The single-quoted -name '{pattern}' sites are not protection: a pattern containing ' closes the quote.

Why it survived

This is precisely the defect class PR #139 fixed — its description says "the job script had no shlex.quote anywhere, so remote_work_dir, module loads and environment variables reached the shell unquoted." That work hardened clustrix/utils.py, which now carries 56 shlex.quote calls and a validate_shell_fragment allowlist, and it added tests/unit/test_script_injection.py to hold the line.

filesystem.py was never audited in that pass. The same author-supplied-string-to-shell pattern sat one module over the whole time.

Two more defects in the same function

While confirming the above:

1. _remote_stat is not portable, and its comment says it is. Line 451 reads # Use stat command with portable format; line 452 uses stat -c, which is GNU coreutils only. BSD and macOS stat take -f and reject -c. On such a host the command fails, 2>/dev/null swallows the error, output is empty, and the caller gets FileNotFoundError: File not found: <path> for a file that exists. The 2>/dev/null turns every failure mode of this command into a false "not found".

2. SFTP would remove the whole problem. paramiko's SFTPClient.stat() returns size, mtime and mode over the protocol — no shell, no quoting, no GNU-vs-BSD difference. ConnectionManager already opens SFTP channels (executor_connections.py:152). _remote_stat, _remote_exists, isdir and isfile are all better served by SFTP than by test/stat.

Definition of done

• No shell command in filesystem.py interpolates an unquoted caller-supplied value
• Where SFTP can replace a shell command outright, it does — that is a removal of the attack surface rather than an escape of it
• 2>/dev/null no longer converts a command failure into a false negative; a failure says what failed
• _remote_stat works on a non-GNU remote host, or refuses with a message naming the reason
• Injection tests in the style of tests/unit/test_script_injection.py, exercised against the real in-process SSH server in tests/ssh_server.py — hostile paths must be treated as filenames, never executed
• A guard that fails if a new unquoted interpolation appears in this module