Fix path traversal in imported hook-transform realignment - #117
Open
jay79-boop wants to merge 1 commit into
Open
Fix path traversal in imported hook-transform realignment#117jay79-boop wants to merge 1 commit into
jay79-boop wants to merge 1 commit into
Conversation
alignHookTransforms() derives filesystem paths from an imported
repo's own openclaw.json content -- match.path (hookPath) and
transform.module (actualModule) -- via normalizeHookPath() /
normalizeTransformModulePath(), which only strip a leading slash.
Neither rejects `../`. Those values then flow straight into
path.join(baseDir, "hooks/transforms", ...) with no containment
check, for both a read/move step (moving whatever exists at the
"actual" module path into a _backup dir) and an unconditional
fs.writeFileSync() of a generated shim at the "expected" module path.
Since this runs during onboarding's "import an existing OpenClaw
workspace" flow -- a first-class feature, not an edge case -- a
crafted openclaw.json in the imported repo could:
- point transform.module far enough up via ../ to reach an
existing host file outside the temp clone directory, which then
gets *moved* into the imported workspace's _backup dir (arbitrary
file read/exfiltration: it becomes browsable, and later
git-synced, as part of the new AlphaClaw config)
- point match.path via ../ so the shim's write path
(baseDir/hooks/transforms/<hookPath>/<hookPath>-transform.mjs)
lands outside the temp dir entirely (arbitrary file write,
unconditional -- no existence check gates the write side)
Both derived-path computations (actualAbsolutePath/expectedAbsolutePath,
and the backup-root pair built from them) now get checked against
baseDir before anything reads, moves, or writes -- mirroring the
containment check already used correctly elsewhere in this same file
(resolveExtractionTargetPath, used by applySecretExtraction and
canonicalizeConfigEnvRefs). A mapping whose paths escape baseDir is
now just skipped instead of processed.
Added two tests to tests/server/import-applier.test.js, verified to
fail without the fix (alignedCount: 1, and the read/write actually
happening) and pass with it:
- a transform.module escape reaching a real external "secret" file:
proves the file is never moved/read and nothing changes outside
baseDir
- a match.path escape targeting an external write location: proves
no shim is written outside baseDir and the legitimate in-workspace
transform is left untouched
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.
Summary
alignHookTransforms()(lib/server/onboarding/import/import-applier.js) derives filesystem paths from an imported repo's ownopenclaw.jsoncontent —match.path(hookPath) andtransform.module(actualModule) — vianormalizeHookPath()/normalizeTransformModulePath(), which only strip a leading slash:Neither rejects
../. Those values flow straight intopath.join(baseDir, "hooks/transforms", ...)with no containment check, for both:_backupdirectoryfs.writeFileSync()of a generated shim at the "expected" module pathThis runs during onboarding's "import an existing OpenClaw workspace" flow — a first-class, encouraged feature (e.g. restoring from a previous deployment's git-synced config repo), not an edge case. A crafted
openclaw.jsonin the imported repo can:transform.modulefar enough up via../to reach an existing host file outside the temp clone directory, which then gets moved into the imported workspace's_backupdir — arbitrary file read/exfiltration, since it becomes browsable (and later git-synced) as part of the new AlphaClaw configmatch.pathvia../so the shim's write path (baseDir/hooks/transforms/<hookPath>/<hookPath>-transform.mjs) lands outside the temp dir entirely — arbitrary file write, unconditional, no existence check gates the write sideFix
Both derived-path computations (
actualAbsolutePath/expectedAbsolutePath, and the backup-root pair built from them) are now checked againstbaseDirbefore anything reads, moves, or writes — mirroring the containment check already used correctly elsewhere in this same file (resolveExtractionTargetPath, used byapplySecretExtractionandcanonicalizeConfigEnvRefs). A mapping whose paths escapebaseDiris now just skipped instead of processed.Test plan
tests/server/import-applier.test.js, verified to fail without the fix (alignedCount: 1, and the read/write actually happening) and pass with it:transform.moduleescape reaching a real external "secret" file: proves the file is never moved/read and nothing changes outsidebaseDirmatch.pathescape targeting an external write location: proves no shim is written outsidebaseDirand the legitimate in-workspace transform is left untouchedimport-applier.test.jssuite passes (6/6), including the existing legitimate-realignment coverage.