Skip to content

fix: prevent OS command injection in file manager (CWE-78) - #19

Open
poliakarmai wants to merge 1 commit into
BrowserlessAPI:mainfrom
poliakarmai:fix/command-injection-file-manager
Open

fix: prevent OS command injection in file manager (CWE-78)#19
poliakarmai wants to merge 1 commit into
BrowserlessAPI:mainfrom
poliakarmai:fix/command-injection-file-manager

Conversation

@poliakarmai

Copy link
Copy Markdown

🔍 Found by GSC — Git Security Checker

The file manager (panel/routes/files.py) builds shell commands with subprocess.run(..., shell=True) and interpolates user-controlled values directly into the command string. safe_path() only normalizes the path (os.path.normpath('/' + p)) — it does not escape shell metacharacters, and two endpoints (/api/files/search, /api/files/scan) skip it entirely.

Vulnerability (CWE-78 — OS Command Injection)

  • /api/files/search?q=<...> — the q parameter is interpolated into grep/find with no sanitization at all.
  • /api/files/compress, /api/files/extract, /api/files/lint, /api/files/size, /api/files/propertiessrc/dst/path/output pass through safe_path(), which does not neutralize ;, $(...), backticks, or quotes.
  • /api/files/scanpath is taken from the JSON body without safe_path().

An authenticated user can inject arbitrary shell commands (e.g. q="; id; #" on the search endpoint), leading to remote command execution on the panel host.

Fix

Replace shell=True string commands with argument lists (shell=False) and pass the working directory via cwd= instead of cd. This removes shell interpretation entirely, so no metacharacter can be injected.

Files Changed

  • panel/routes/files.py — all subprocess.run(shell=True, ...) calls converted to argument lists; scan_file now applies safe_path().

📚 Understanding This Vulnerability

What is it?

OS command injection (CWE-78) happens when user input is concatenated into a shell command without escaping. An attacker who controls any interpolated value can append their own commands.

Why does it matter?

It is OWASP A03:2021 (Injection). A single injectable parameter turns into arbitrary code execution on the server — the attacker inherits the panel's privileges (often root). This is how many server control panels get fully compromised.

When does it happen?

  1. subprocess.run(cmd, shell=True) with an f-string containing user input.
  2. No shlex.quote() / argument list.
  3. A "sanitizer" that only normalizes paths but leaves shell metacharacters intact.

How to fix it properly?

  1. Prefer shell=False with a list of arguments — no shell, no injection surface.
  2. Use cwd= for cd and stderr=subprocess.DEVNULL for 2>/dev/null.
  3. If shell=True is unavoidable, wrap every value in shlex.quote().

🧠 Analogy

Telling a waiter "write whatever the customer says on the order form" vs. handing them a pre-printed form where only the fields can be filled in. An argument list is the pre-printed form — the customer can't rewrite the whole order.

📖 Further reading


🐛 Found by GSC — понимай код, а не принимай.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant