[security][high] Avoid shell-interpreted execution of config-derived commands (command-injection risk)

[upupwrite]

Summary

Path Keeper constructs and executes shell command strings that include data from configuration files and other runtime sources and uses system/popen in multiple places (notably src/search.cpp and src/loadfile.cpp). This creates a command injection vulnerability when any part of the executed string can be influenced by an attacker (for example, a writable config file, a crafted alias, or untrusted environment variables). The same pattern also makes it easy to accidentally execute malicious binaries if PATH is tampered with.

Reproduction / evidence

• src/search.cpp: interactiveSearch() builds a command string "fzf --delimiter='\t' --with-nth=2,3 < " + tmpname and passes it to popen(). The list written to tmpname is derived from config (generateList()).
• src/loadfile.cpp / include/loadfile.h: commands are read from config["path"] and later matched/executed by the application (getCommandIndex, Editor classes). The code uses popen("which ...") and other shell invocations.

Risks

• If an attacker can write or modify the user config file (or supply an alias/command through import or other feature), they can cause arbitrary shell execution on the host with the privileges of the running user.
• If PATH or other environment variables are maliciously controlled, the app may execute trojanized binaries.
• High severity: arbitrary command execution leads to full compromise of the user account running Path Keeper.

Suggested fixes

1. Eliminate use of shell-based execution for config-derived commands.

   • Do not pass a single shell-interpreted string to system/popen//bin/sh -c when the command or arguments contain untrusted data.
   • Use execv()/execvp()/posix_spawn() with an argv array, or use QProcess (Qt) with a QStringList of arguments, so that command arguments are not interpreted by a shell. Example: QProcess::startDetached(program, arguments);
   • When you must use a shell, strictly validate and escape any untrusted inputs (prefer avoiding this entirely).

2. Validate and sanitize configuration input.

   • Treat all config-provided command strings as untrusted. If features require literal shell snippets, mark them explicitly and require user confirmation and documentation.
   • Provide an option to store commands as arrays (program + args) rather than raw shell lines; prefer structured command storage.

3. Restrict config write access and verify ownership.

   • Ensure config files are created with owner-only write permissions (e.g., 0600) and check ownership before executing commands read from them.

4. Add unit tests and CI checks.

   • Add tests that attempt to put metacharacters into config entries and verify they do not cause shell execution.
   • Add a security-focused CI scan (shellcheck for scripts, static analysis for C++ callsites that use system/popen).

Files/places to review first

• src/search.cpp (interactiveSearch and any follow-up execution paths for selected commands)
• src/loadfile.cpp (getCommandIndex, any functions that actually run commands)
• Any other callsites of system(), popen(), execl*/execv* and similar.

References

• POSIX exec* usage patterns; QProcess::startDetached (Qt docs)
• OWASP Command Injection guidelines