Skip to content

[SECURITY] Path sanitization hardening for plugin traversal protection - #1

Open
leighstillard wants to merge 1 commit into
jdforsythe:masterfrom
leighstillard:security/path-sanitization-hardening
Open

[SECURITY] Path sanitization hardening for plugin traversal protection#1
leighstillard wants to merge 1 commit into
jdforsythe:masterfrom
leighstillard:security/path-sanitization-hardening

Conversation

@leighstillard

Copy link
Copy Markdown

Summary

This PR addresses three security vulnerabilities related to path traversal and arbitrary file write attacks in jig's plugin and hook script handling.

Vulnerabilities Fixed

1. Arbitrary file write via unvalidated hook_scripts.dest

File: internal/claude/plugin.go
Severity: High
Attack vector: A malicious project .jig.yaml or profile sets hook_scripts[*].dest to a path like ../../../etc/cron.d/backdoor. Since filepath.Join was called without containment checks, this would write the hook script to an arbitrary location on disk with the user's privileges.

Fix: Introduced safeJoinWithin(baseDir, rel string) which uses filepath.Clean + filepath.Rel to verify the resolved destination is strictly within the plugin temp directory before performing the join.

2. Arbitrary symlink creation via unsanitized plugin component names

File: internal/claude/plugin.go
Severity: High
Attack vector: A malicious plugin definition exposes components with names like ../../../home/user/.ssh/authorized_keys. These names were used directly in filepath.Join for symlink destinations, allowing symlinks to be created at arbitrary paths.

Fix: Introduced safeComponentName(name string) which rejects empty strings, ., .., absolute paths, and any name containing path separators. safeBaseName wraps this for symlink source path handling.

3. Validation gap at profile load time

File: internal/config/validate.go
Severity: Medium
Attack vector: Malicious path values could pass Validate() and only fail (or not fail) at runtime during plugin directory generation, with no user-visible error.

Fix: Added validateComponentName helper to validation layer; Validate() now enforces absolute-path and traversal rejection for hook_scripts[*].dest and all plugin_components[*].agents/skills/commands entries at profile load time.

Changes

File Change
internal/claude/plugin.go Added safeComponentName, safeBaseName, safeJoinWithin helpers; integrated into 6 symlink loops + 1 hook script copy
internal/config/validate.go Added validateComponentName helper; added validation checks for hook script dest and plugin component names
internal/claude/plugin_extra_test.go Added 11 tests for new safety functions
internal/config/config_test.go Added 11 tests for validation-layer safety checks

Testing

All existing and new tests pass:

ok  github.com/jdforsythe/jig/internal/claude   0.034s
ok  github.com/jdforsythe/jig/internal/config   0.037s
ok  github.com/jdforsythe/jig/internal/plugin   0.033s
ok  github.com/jdforsythe/jig/internal/scanner  0.021s
ok  github.com/jdforsythe/jig/internal/tui/screens 0.008s

Usability Impact

None. Legitimate paths pass all checks:

  • Component names like my-agent, code-analyzer, validate
  • Hook destinations like hooks/script.sh, validate.sh
  • Only traversal and absolute path attacks are rejected ✗

Deferred (Behavior-Changing)

Two additional hardening opportunities were identified but not implemented as they would change existing behavior and require user discussion:

  1. Project config trust gatesystem_prompt and extra_flags from project-local profiles are currently applied without approval. An interactive prompt on first use of a project profile could be added.
  2. MCP server override approval — Project .mcp.json definitions currently override global ~/.claude/.mcp.json without confirmation. A read-only append policy (project can add new servers but not override existing globals) could be considered.

Prevent directory traversal and absolute path abuse in plugin component
operations and hook script destinations.

Changes:
- internal/claude/plugin.go: add safeComponentName, safeBaseName, and
  safeJoinWithin helpers; validate all plugin component names and hook
  script destinations before use in filepath operations

- internal/config/validate.go: add validateComponentName helper; enforce
  absolute-path and traversal rejection for hook_scripts.dest and all
  plugin_component names at profile-validation time

- internal/claude/plugin_extra_test.go: add 11 tests covering all new
  safety functions (accept, reject empty, reject dot/dotdot, reject
  absolute paths, reject path separators, correct path computation)

- internal/config/config_test.go: add 11 tests covering validateComponentName
  and Validate-layer checks for hook script dest and plugin component names

Vulnerabilities addressed:
- Arbitrary file write via unvalidated hook_scripts.dest (jdforsythe#1)
- Arbitrary symlink creation via unsanitized plugin component names (jdforsythe#2)
- Validation gap allowing path traversal to reach profile load time (jdforsythe#3)

Zero usability impact: legitimate paths (e.g. 'my-agent', 'hooks/script.sh')
pass cleanly; only traversal and absolute path attacks are rejected.
@leighstillard

Copy link
Copy Markdown
Author

Deferred Hardening Items

Two additional security improvements were identified during the audit but not included in this PR because they would change existing user-facing behaviour. Documenting here for tracking:


Decision 1: Project config trust gate

Issue: When a user runs jig run <profile> inside a project directory, any project-local profile (e.g. .jig/profiles/my-profile.yaml or .jig.yaml) is automatically loaded and its system_prompt, append_system_prompt, and extra_flags are passed directly to Claude — without the user being prompted to review them first.

Risk: Opening an untrusted repo and running jig could silently execute repo-injected prompts or flags.

Potential mitigations (ranked by usability impact):

  1. (Minimal) Schema-level validation on these fields to reject known injection patterns — least disruptive
  2. (Medium) Interactive approval prompt on first use of a project profile: "Project defines system_prompt. Allow? [Y/n]"
  3. (High) GPG-signed project configs — most secure but requires significant infrastructure

Decision 2: MCP server override approval

Issue: Project-local .mcp.json is loaded after the global ~/.claude/.mcp.json and its definitions silently overwrite global ones. A malicious project could redefine an existing MCP server (e.g. "github") to point to an attacker-controlled binary.

Risk: Supply-chain style attack where a trusted server name is hijacked by a project config.

Potential mitigations (ranked by usability impact):

  1. (Low) Read-only append policy: project .mcp.json can add new server names but cannot override existing global entries — log a warning if an override is attempted. This is the recommended starting point.
  2. (Medium) Interactive approval per project-defined server on first run
  3. (High) Allowlist of permitted project-level MCP server names

Happy to implement any of the above if the maintainer wants to explore further hardening.

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.

1 participant