Mount .devcontainer/ read-only to prevent container escape on rebuild#12
Mount .devcontainer/ read-only to prevent container escape on rebuild#12RyanJarv wants to merge 1 commit intotrailofbits:mainfrom
Conversation
A process inside the container could modify .devcontainer/devcontainer.json to inject malicious mounts or initializeCommand entries that execute on the host during the next rebuild. Bind-mounting .devcontainer/ as read-only blocks this privilege escalation vector. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
|
@dguido fyi |
dguido
left a comment
dguido
left a comment
There was a problem hiding this comment.
Nice find — the threat model is solid and the fix is well-scoped. Three small suggestions:
SYS_ADMIN guard (suggestion 3): The read-only bind mount only holds if the container can't remount it read-write, which requires SYS_ADMIN. The current runArgs only grant NET_ADMIN/NET_RAW so this is fine today, but there's no guard against someone adding SYS_ADMIN later and silently breaking the protection. Consider adding a comment above extract_mounts_to_file() (lines 83-84) like:
# Security: .devcontainer/ is mounted read-only inside the container to prevent
# a compromised process from injecting malicious mounts or commands into
# devcontainer.json that execute on the host during rebuild. This protection
# requires that SYS_ADMIN is never added to runArgs (it would allow remounting
# read-write).(Can't put it in devcontainer.json directly since install.sh processes it with jq, which doesn't handle JSONC comments.)
| (startswith("source=claude-code-gh-") | not) and | ||
| (startswith("source=${localEnv:HOME}/.gitconfig,") | not) | ||
| (startswith("source=${localEnv:HOME}/.gitconfig,") | not) and | ||
| (contains("target=/workspace/.devcontainer,") | not) |
There was a problem hiding this comment.
Suggestions 1+2: contains() is slightly loose — it would also match a mount targeting e.g. /workspace/.devcontainer-backup. Since the source is a known literal in the JSON (${localWorkspaceFolder} isn't resolved until the devcontainer CLI processes it), startswith() on the source is more precise and consistent with the other filters. Added an inline comment explaining the security rationale.
| (contains("target=/workspace/.devcontainer,") | not) | |
| # Security: read-only .devcontainer mount prevents container escape on rebuild | |
| (startswith("source=${localWorkspaceFolder}/.devcontainer,") | not) |
A process inside the container could modify .devcontainer/devcontainer.json to inject malicious mounts or initializeCommand entries that execute on the host during the next rebuild. Bind-mounting .devcontainer/ as read-only blocks this privilege escalation vector. Uses startswith() for the jq filter to be precise and consistent with other mount filters, and documents the SYS_ADMIN guard requirement. Based on PR #12 with review feedback from @dguido addressed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…#13) A process inside the container could modify .devcontainer/devcontainer.json to inject malicious mounts or initializeCommand entries that execute on the host during the next rebuild. Bind-mounting .devcontainer/ as read-only blocks this privilege escalation vector. Uses startswith() for the jq filter to be precise and consistent with other mount filters, and documents the SYS_ADMIN guard requirement. Based on PR #12 with review feedback from @dguido addressed. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
A process inside the container could modify .devcontainer/devcontainer.json to inject malicious mounts or initializeCommand entries that execute on the host during the next rebuild. Bind-mounting .devcontainer/ as read-only blocks this privilege escalation vector.