T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/upscale_video.sh:134
- Finding
- Command Injection Through an Unsafe EXIT Trap<![CDATA[ ## Vulnerability Details **File Location**: `scripts/upscale_video.sh`, lines 14–19 and 134–135 **Vulnerability Type**: Shell command injection through dynamically constructed trap code **Risk Level**: High ### Vulnerable Code ```bash JOB_ID="${6:-}" # optional job ID # Generate job ID if not provided if [ -z "$JOB_ID" ]; then JOB_ID="job_$(date +%s)_$RANDOM" fi ``` ```bash TEMP_DIR=$(mktemp -d "/tmp/openclaw-upscale-${JOB_ID}-XXXXXX") trap "rm -rf $TEMP_DIR" EXIT ``` ### Technical Analysis The optional `JOB_ID` parameter is controlled by the caller and is embedded in the temporary-directory template without validation. Quoting the argument passed to `mktemp` prevents immediate shell expansion, but it does not make the resulting filename safe for later use as shell source. The generated path is interpolated into a double-quoted `trap` command. The trap body is parsed again by the shell when the script exits. Consequently, shell metacharacters contained in the directory name—such as semicolons or comment characters—can alter the trap command and introduce additional commands. This behavior is unnecessary for video upscaling and violates the principle that untrusted values must never be incorporated into dynamically evaluated shell code. ### Attack Path 1. An attacker or untrusted caller invokes the script with a malicious sixth argument, for example a `JOB_ID` containing syntax equivalent to: ```text x;id;# ``` 2. `mktemp` creates a directory whose path contains those characters because the supplied template is passed as one quoted filesystem argument. 3. The resulting path is inserted into the trap body: ```bash trap "rm -rf $TEMP_DIR" EXIT ``` 4. When the script exits normally or because of an error, Bash reparses the expanded trap body. 5. The semicolon terminates the intended `rm` command, the injected command executes, and the comment character suppresses the remaining generated filename suffix. 6. An attacker can r ...[truncated 615 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Do not construct trap handlers as strings containing expanded, caller-influenced values. Define a cleanup function and let it reference the variable when executed: ```bash TEMP_DIR="" cleanup() { if [ -n "${TEMP_DIR:-}" ] && [ -d "$TEMP_DIR" ]; then rm -rf -- "$TEMP_DIR" fi } trap cleanup EXIT TEMP_DIR=$(mktemp -d "/tmp/openclaw-upscale-XXXXXXXX") ``` Additionally: 1. Do not include `JOB_ID` in the temporary-directory template unless operationally necessary. 2. If it must be included, enforce a strict allowlist before use: ```bash case "$JOB_ID" in (*[!A-Za-z0-9._-]*|'') error_exit "Invalid job ID" ;; esac ``` 3. Use `rm -rf -- "$TEMP_DIR"` so the path remains one quoted argument and cannot be interpreted as an option. 4. Keep the temporary path under a fixed trusted parent and verify that it is nonempty before deletion. 5. Add regression tests using job IDs containing semicolons, spaces, quotes, command substitutions, newlines, and option-like strings. ]]>