Skip to content

Fix command injection and import path traversal in compiler#1159

Open
marcauberer wants to merge 3 commits into
mainfrom
security/fix-cmd-injection-path-traversal
Open

Fix command injection and import path traversal in compiler#1159
marcauberer wants to merge 3 commits into
mainfrom
security/fix-cmd-injection-path-traversal

Conversation

@marcauberer

Copy link
Copy Markdown
Member

Summary

Fixes two security vulnerabilities in the C++ compiler's handling of untrusted input (malicious .spice source / projects compiled by a build service or CI).

HIGH — Command injection via shell-interpreted linker/archiver/run commands

The compiler built the linker, archiver and "run" commands as a single string and executed them through a shell (popen/_popen). File paths were concatenated unquoted, and object-file names derive directly from source/import filenames (outputDir / filePath.filename()). On Linux a filename may contain $, `, ;, spaces, etc., so a project shipping a file named e.g. x$(touch pwned).spice and importing it achieves arbitrary command execution at link time. The -o output path was a second injection point.

Fix: stop building shell strings in the compiler. Added a shell-free SystemUtil::exec(program, args, …) built on llvm::sys::findProgramByName + ExecuteAndWait, passing every argument verbatim via an argv vector (output captured through a temp file). Converted all compiler-side callers — ExternalLinkerInterface::link()/archive(), Driver::runBinary(), and the Graphviz dot invocation — and reimplemented isCommandAvailable() via findProgramByName (dropping std::system/which/where). The $(clang -print-resource-dir) shell substitution previously embedded in the MSAN/TYSAN linker flag is now resolved in C++ (getClangResourceDir()). The old string/shell exec is retained only for the test harness, with a doc comment warning it must never take untrusted input.

MEDIUM — Path traversal in import resolution

ImportCollector::visitImportDef concatenated the raw import-path string into a filesystem path with no validation, so import "../../../etc/x" (or the std/../../../etc/x bypass) could escape the project / std / bootstrap roots — a sandbox escape and file-existence oracle.

Fix: validate the import path before resolution — reject absolute/rooted paths (has_root_path() catches /etc, C:/…, C:\…) and any .. component — with a new INVALID_IMPORT_PATH semantic error. No existing .spice in the repo uses ../absolute imports, so nothing legitimate breaks. Added a reference test under test/test-files/typechecker/imports/error-import-path-traversal/.

Files

  • src/util/SystemUtil.{h,cpp} — shell-free argv exec; isCommandAvailable via findProgramByName
  • src/linker/ExternalLinkerInterface.cpp — argv-based link/archive; resolve clang resource dir
  • src/driver/Driver.cpp, src/SourceFile.cpp — argv-based run / dot
  • src/importcollector/ImportCollector.cpp — import-path validation
  • src/exception/SemanticError.{h,cpp}INVALID_IMPORT_PATH
  • test/test-files/typechecker/imports/error-import-path-traversal/ — new test

Testing / reviewer notes

  • ⚠️ This was developed in an environment that cannot build or run Spice (missing generated ANTLR headers; binary execution blocked). The new LLVM API usage was syntax-checked in isolation against the real LLVM 22 headers with g++ -std=c++23 -fsyntax-only, but the full build and tests were not run locally.
  • Please run spicetest for the new error-import-path-traversal case and regenerate its exception.out with --update-refs if the message text differs (it was hand-authored to match the existing convention).
  • Please confirm a normal build + link test, and an MSAN/TYSAN build if you use those, since the linker invocation and sanitizer resource-dir resolution changed.

🤖 Generated with Claude Code

@marcauberer marcauberer requested a review from a team as a code owner June 3, 2026 13:12
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added tests Contains changes to the test cases compiler labels Jun 3, 2026
marcauberer and others added 2 commits June 10, 2026 13:27
HIGH: The compiler built linker/archiver/run commands as shell strings and
executed them via popen/_popen, so attacker-influenceable file paths (object
files derived from source/import filenames, the -o output path) could inject
shell commands at compile/link time when building untrusted projects.

Replace shell-based execution in the compiler with a shell-free
SystemUtil::exec(program, args, ...) built on llvm::sys::findProgramByName +
ExecuteAndWait, passing every argument verbatim through an argv vector. Convert
all compiler-side callers (linker, archiver, runBinary, Graphviz dot) and
reimplement isCommandAvailable via findProgramByName (drops std::system). The
'$(clang -print-resource-dir)' sanitizer flag is now resolved in C++. The old
string/shell exec is retained only for the test harness, with a warning.

MEDIUM: ImportCollector resolved raw import-path strings against the project /
std / bootstrap roots without validation, allowing path traversal
(e.g. import "../../../etc/x" or "std/../../../etc/x") to escape the intended
tree. Reject absolute/rooted paths and any '..' component before resolution,
with a new INVALID_IMPORT_PATH semantic error. Add a reference test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@marcauberer marcauberer force-pushed the security/fix-cmd-injection-path-traversal branch from e668132 to 768ad94 Compare June 10, 2026 14:17
@marcauberer marcauberer added this to the 0.27.0 milestone Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler size/L tests Contains changes to the test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant