Skip to content

fix(security): validate trashDir to prevent SSH command injection (fixes #35)#39

Open
ThankNIXlater wants to merge 1 commit into
iamlukethedev:mainfrom
ThankNIXlater:fix/issue-35-trash-dir-injection
Open

fix(security): validate trashDir to prevent SSH command injection (fixes #35)#39
ThankNIXlater wants to merge 1 commit into
iamlukethedev:mainfrom
ThankNIXlater:fix/issue-35-trash-dir-injection

Conversation

@ThankNIXlater

Copy link
Copy Markdown
Contributor

Adds regex validation for the trashDir parameter in the PUT handler of /api/gateway/agent-state/route.ts. The value is now checked against ^[a-zA-Z0-9_.~\/-]+$ before being passed to SSH commands, preventing shell metacharacter injection.

Fixes #35.

ThankNIXlater added a commit to ThankNIXlater/Claw3D that referenced this pull request Mar 23, 2026
iamlukethedev#53)

Adds regex validation for baseDir, workspaceDir, and managedSkillsDir
in the skill removal endpoint. These values are passed as SSH positional
arguments and should only contain safe filesystem characters.

Uses the same safe-path pattern established in PR iamlukethedev#39 for trashDir
validation, ensuring consistency across all SSH-facing inputs.

@iamlukethedev iamlukethedev left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR should be merged as-is.

The new trashDir regex is too strict and can reject valid restore paths, especially when the OpenClaw state directory is overridden to a path containing spaces. Both restore implementations already do the important safety check by resolving the path and verifying that it stays under the OpenClaw state directory, so this API-layer validation can introduce false negatives for legitimate restores.

Please revise the fix so it protects the SSH execution path without rejecting valid filesystem paths, and add a test that covers restoring from a valid trashDir under a state directory whose path includes spaces.

@iamlukethedev

Copy link
Copy Markdown
Owner

Suggested direction for a safer fix:

  1. Remove the new trashDir regex from the route.
  2. Keep the existing path safety checks in the local/SSH restore implementations (exists + resolve + verify under the OpenClaw state dir).
  3. Change the SSH restore path so trashDir is not passed as a remote command argument via ssh ... bash -s -- ..., since ssh host command arg1 arg2 is flattened and reparsed by the remote shell.
  4. Instead, serialize { agentId, trashDir } as JSON, base64-encode it, and embed that payload inside the script sent on stdin. Then decode it inside Python before doing the existing validation.
  5. Add a test that restores from a valid trashDir under a state directory whose path contains spaces.

I’d also audit the other runSshJson call sites that pass user-controlled paths as remote args, especially the gateway media and skill-removal flows, because they appear to use the same SSH invocation pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Unsanitized trashDir parameter in SSH agent-state restore allows remote command injection

3 participants