hooks: harden path resolution against symlink components#69
Merged
Conversation
Enables upcoming tests that verify hook helpers refuse to follow symlink components planted in a rootfs. The helper places a symlink at rootfs/linkPath pointing to an arbitrary target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hook previously used os.MkdirAll + os.WriteFile to place authorized_keys under a caller-configured home path inside the rootfs. pathutil.Contains guarded against lexical traversal but did not inspect the resolved filesystem — a symlink component planted inside the rootfs (e.g. by a malicious OCI layer) caused the write to follow the link out of the rootfs. Route the directory creation through image.MkdirAllNoSymlink (Lstat each component, refuse symlinks), gate the leaf with image.ValidateNoSymlinkLeaf, and open the final file with O_NOFOLLOW to close the TOCTOU window between validation and write. Covered by three new sub-tests: an absolute symlink at the home component, a relative escaping symlink at .ssh, and a symlink leaf at authorized_keys. All assert the out-of-rootfs target is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Route parent-directory creation through image.MkdirAllNoSymlink, gate the leaf with image.ValidateNoSymlinkLeaf, and open with O_NOFOLLOW via writeFileNoFollow. InjectBinary delegates to InjectFile so both are covered by the same code path. Tests cover the two realistic rootfs shapes: a parent directory replaced by a symlink and a leaf replaced by a symlink pointing at a host file. Both assert the out-of-rootfs target is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Last of the Inject* file-writing helpers. Same treatment as InjectAuthorizedKeys and InjectFile: MkdirAllNoSymlink for parent dirs, ValidateNoSymlinkLeaf for the target, O_NOFOLLOW on open. Tests cover parent-as-symlink and leaf-as-symlink variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
InjectVMConfig writes /etc/go-microvm.json by delegating to InjectFile, which now refuses symlink components. A regression test locks the guarantee in at the delegation boundary so a future refactor that inlines or bypasses InjectFile cannot silently lose the protection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, any non-permission error from os.Lchown was logged at warn level and then silently dropped — a failure mode where the file's owner silently ended up wrong while the caller saw no error. Tighten the contract so only permission-denied errors (the expected non-root case) are absorbed; everything else (ENOENT, EROFS, EIO, ...) propagates to the caller. Tests cover both paths: a non-existent path returning ENOENT surfaces an error; a non-root chown to a foreign UID returning EPERM returns nil. The EPERM test is skipped when running as root since root can chown to any UID. Co-Authored-By: Claude Opus 4.7 (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
1 participant
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
Phase 1 of a multi-phase hardening pass on
hooks/. EveryInject*helperthat writes into the rootfs now routes parent-directory creation through
image.MkdirAllNoSymlink, gates the target path withimage.ValidateNoSymlinkLeaf, and opens the final file withO_NOFOLLOWtoclose the TOCTOU window. The helpers previously relied only on
pathutil.Contains, which is lexical — a rootfs whose target path containeda symlink component had that symlink followed by the subsequent
os.MkdirAll/os.WriteFile.Commits land one-per-change so the series is bisectable:
stageSymlinktest helper for hook fixturesInjectAuthorizedKeysInjectFileandInjectBinaryInjectEnvFileInjectVMConfigsymlink-rejection via delegation chainBestEffortLchownto only swallow permission errorsBestEffortLchownpreviously logged then swallowed any non-permissionerror; it now propagates
ENOENT,EROFS,EIO, etc. so silentownership drift no longer hides behind a
nilreturn.No API break:
writeFileNoFollowis unexported and used only internally.The only observable change for callers is error propagation in
BestEffortLchown, which previously masked non-permission errors.Test plan
task verifygreen at every commit in the seriesrelative escaping symlink at
.ssh, symlink leaf at the file target,parent directory as a symlink
TestInjectAuthorizedKeys_*,TestInjectFile_*,TestInjectEnvFile_*,TestInjectVMConfig) continue to passbrood-boxwith a localreplacedirective:a full VM lifecycle (snapshot → boot → SSH via injected key →
headless
claude-codewriting/workspace/hello.txt→ flush back tohost) completed cleanly
🤖 Generated with Claude Code