T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/init_sync.sh:449
- Finding
- Arbitrary Shell Command Injection Through eval-Based Command Execution<![CDATA[ ## Vulnerability Details **File Location**: `scripts/init_sync.sh:449-461` **Vulnerability Type**: Shell command injection **Risk Level**: Critical ### Vulnerable Code ```bash # Dry-run helper do_cmd() { if [ "$DRY_RUN" = true ]; then echo " [DRY-RUN] $*" else eval "$@" fi } # Initialize version files MEMORY_DIR="$MASTER_WS/memory" msg creating_dirs do_cmd "mkdir -p \"$MEMORY_DIR\"" ``` Additional attacker-influenced commands are evaluated at: ```bash do_cmd "echo \"\$SYNC_TEMPLATE\" > \"$agent_ws/SYNC.md\"" do_cmd "mkdir -p \"$local_agent_memory\"" do_cmd "_atomic_write \"$local_agent_memory/.agent_sync_version\" '$START_VERSION'" do_cmd "mkdir -p \"$local_agent_memory/.sync_snapshots\"" ``` ### Technical Analysis The `do_cmd` helper constructs shell commands as strings and executes them through `eval`. `eval` reparses its arguments as shell syntax, so quoting performed while creating the string does not provide a reliable security boundary. `MASTER_WS` may come directly from a positional command-line argument: ```bash elif [ -z "$MASTER_WS" ] && [ "${arg#--}" = "$arg" ]; then MASTER_WS="$arg" fi ``` It may also be derived from the editable Agent registry. No character validation, canonical path validation, or shell metacharacter rejection is performed before the value is interpolated into a command passed to `eval`. A value containing a quotation mark followed by shell syntax can terminate the intended quoted argument. Command separators, command substitutions, redirections, or pipelines can then be interpreted by the second shell parsing pass. The same unsafe execution pattern is used for writes and directory creation in downstream Agent workspaces. ### Attack Path 1. An attacker modifies `references/agent-registry.json`, influences a registry-generation source, or convinces a user to supply a crafted master-workspace argument. 2. The crafted path contains shell syntax that escapes the quoted argument used in a `do_c ...[truncated 1150 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove `eval` completely. 2. Replace string-based execution with direct function calls and commands whose arguments remain distinct shell words: ```bash run_or_preview() { if [ "$DRY_RUN" = true ]; then printf ' [DRY-RUN]' printf ' %q' "$@" printf '\n' else "$@" fi } run_or_preview mkdir -p -- "$MEMORY_DIR" run_or_preview _atomic_write \ "$local_agent_memory/.agent_sync_version" \ "$START_VERSION" ``` 3. Handle multiline file creation through dedicated functions rather than constructing redirection expressions: ```bash write_file() { local destination="$1" local content="$2" if [ "$DRY_RUN" = true ]; then printf '[DRY-RUN] Would write %s\n' "$destination" else printf '%s\n' "$content" > "$destination" fi } ``` 4. Reject control characters and unexpected shell metacharacters in identifiers and paths even after removing `eval`. 5. Validate all paths canonically before performing a write. 6. Add regression tests with spaces, quotes, semicolons, command substitutions, newlines, and redirection characters in all registry and command-line inputs. ]]>