Fix path traversal in imported config's $include handling (raw secret disclosure) - #118
Open
jay79-boop wants to merge 1 commit into
Open
Conversation
A crafted $include value in an imported repo's openclaw.json is untrusted content, but it flowed into filesystem paths in two places with no containment check: 1. import-scanner.js's scanWorkspace(): walks $include values found in the imported openclaw.json and, if the resolved path exists, adds the *raw, unresolved* include string to gatewayConfig.files. A `../../../real/host/file.json` include that happens to exist on the host gets added verbatim -- no ../ rejection, no containment check. 2. import-config.js's resolveImportedConfigPaths(): same pattern against the *live* OPENCLAW_DIR (post-promotion), used by normalizeImportedConfig() -- which both reads AND writes these paths back if their content needs normalizing. The real impact is downstream: gatewayConfig.files (from chrysb#1) is passed directly into secret-detector.js's detectSecrets()/extractPreFillValues(), which read each entry via plain path.join(baseDir, cfgFile) with no containment check of their own, and detectSecrets()'s result -- including raw, unmasked secret values -- is returned directly in the POST /api/onboard/import/scan HTTP response body. Chained together: importing a malicious repo whose openclaw.json contains something like `"$include": "../../../etc/some-real-config.json"` lets AlphaClaw read an arbitrary host JSON file that happens to exist, run it through the same heuristics used to detect real secrets (key names like apiKey/token/password, known value prefixes like sk-/ghp_/ AKIA), and hand back matching values -- unmasked -- in the scan response shown to the admin during onboarding. Verified locally: a `sk-ant-...`-shaped value in a file outside the temp clone directory was fully exfiltrated via detectSecrets()/extractPreFillValues() before this fix. Fixed at all three points in the chain (belt and suspenders, since any one of these closes the demonstrated exploit but each is independently worth guarding): - import-scanner.js: reject include paths that resolve outside baseDir before adding them to gatewayConfig.files - import-config.js: same check in resolveImportedConfigPaths against openclawDir - secret-detector.js: detectSecrets()/extractPreFillValues() now resolve configFiles/envFiles entries within baseDir themselves (mirroring the resolveExtractionTargetPath helper already used correctly elsewhere in import-applier.js) rather than trusting that every caller already validated its input Added tests for all three fix points, each verified locally to fail without the fix (proving the traversal reaches a real external file / discloses a real secret value) and pass with it.
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
A crafted
$includevalue in an imported repo'sopenclaw.jsonis untrusted content, but it flowed into filesystem paths in two places with no containment check:import-scanner.js'sscanWorkspace(): walks$includevalues found in the importedopenclaw.jsonand, if the resolved path exists, adds the raw, unresolved include string togatewayConfig.files. A../../../real/host/file.jsoninclude that happens to exist on the host gets added verbatim — no../rejection, no containment check.import-config.js'sresolveImportedConfigPaths(): same pattern, but against the liveOPENCLAW_DIR(post-promotion) — used bynormalizeImportedConfig(), which both reads and writes these paths back if their content needs normalizing.The real impact is downstream:
gatewayConfig.files(from #1) is passed directly intosecret-detector.js'sdetectSecrets()/extractPreFillValues(), which read each entry via plainpath.join(baseDir, cfgFile)with no containment check of their own — anddetectSecrets()'s result, including raw, unmasked secret values, is returned directly in thePOST /api/onboard/import/scanHTTP response body.Chained together: importing a malicious repo whose
openclaw.jsoncontains something like:{ "auth": { "$include": "../../../etc/some-real-config.json" } }lets AlphaClaw read an arbitrary host JSON file that happens to exist, run it through the same heuristics used to detect real secrets (key names like
apiKey/token/password, known value prefixes likesk-/ghp_/AKIA), and hand back matching values — unmasked — in the scan response shown to the admin during onboarding.Verified locally: a
sk-ant-...-shaped value in a file outside the temp clone directory was fully exfiltrated viadetectSecrets()/extractPreFillValues()before this fix — confirmed by reverting each guard individually and watching the raw value come back in the result.Fix
Fixed at all three points in the chain (belt and suspenders — any one of these closes the demonstrated exploit, but each is independently worth guarding):
import-scanner.js: reject include paths that resolve outsidebaseDirbefore adding them togatewayConfig.filesimport-config.js: same check inresolveImportedConfigPathsagainstopenclawDirsecret-detector.js:detectSecrets()/extractPreFillValues()now resolveconfigFiles/envFilesentries withinbaseDirthemselves (mirroring theresolveExtractionTargetPathhelper already used correctly elsewhere inimport-applier.js) rather than trusting that every caller already validated its inputTest plan
tests/server/import-scanner.test.js,tests/server/import-config.test.js(new file), andtests/server/secret-detector.test.js.