Security hardening: env isolation, permissions, input validation, path safety - #391
Open
nobugpal wants to merge 1 commit into
Open
Security hardening: env isolation, permissions, input validation, path safety#391nobugpal wants to merge 1 commit into
nobugpal wants to merge 1 commit into
Conversation
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
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.
Security hardening: env isolation, permissions, input validation, path safety
Summary of Changes
This PR applies four independent security hardening improvements to the Shannon CLI.
1. Restrict forwarded environment variables (env.ts)
Problem: The
FORWARD_VARSlist included API keys and bearer tokens that were forwarded to Docker containers as environment variables. These were visible viadocker inspectand inside the container's/proc/*/environ.Fix: Removed credential variables (
ANTHROPIC_API_KEY,CLAUDE_CODE_OAUTH_TOKEN) fromFORWARD_VARS. Only infrastructure configuration variables are now forwarded. Credentials continue to reach the worker through the existingproviderConfigmechanism inclaude-executor.ts.2. Harden workspace directory permissions (start.ts)
Problem: Workspace directories and subdirectories were created with world-writable permissions (
0o777), allowing any local user to modify audit logs, exploitation queue files, and session state.Fix: Changed permissions from
0o777to0o755(owner can write, others read-only). The container user (UID-remapped via entrypoint) retains write access.3. Add input validation for workspace ID (logs.ts)
Problem: The
shannon logscommand accepted arbitraryworkspaceIdstrings that were joined directly into file system paths, allowing path traversal.Fix: Added a
WORKSPACE_ID_PATTERNvalidation that only allows alphanumeric characters, hyphens, underscores, and dots. Path separators (/,\) and traversal sequences (..) are rejected.4. Add path safety checks for
--repoand--config(paths.ts)Problem: The
resolveRepo()andresolveConfig()functions accepted arbitrary paths with no validation, allowing system directories (e.g.,/etc,/var/lib/docker) to be mounted into the worker container.Fix: Added a blocklist of system-critical directories and sensitive home subdirectories (
.ssh,.aws,.config, etc.) that cannot be mounted. A warning is also emitted when mounting paths outside the expected workspace directory.Testing
docker inspectoutput0o755(not world-writable)logscommand/etc,~/.ssh, etc.) are blocked from mount/tmp/my-repo,~/Projects/myapp) are allowed