T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/trackyard.sh:151
- Finding
- Caller-Controlled Download Path Allows Arbitrary File Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `scripts/trackyard.sh`, lines 151–202 **Vulnerability Type**: Unrestricted file write and overwrite **Risk Level**: Medium ### Vulnerable Code ```bash cmd_download() { local track_id="" local duration="" local hit_point="" local output="" while [[ $# -gt 0 ]]; do case "$1" in --duration) duration="$2"; shift 2 ;; --hit-point) hit_point="$2"; shift 2 ;; --output) output="$2"; shift 2 ;; -*) echo "Unknown option: $1" >&2; exit 1 ;; *) track_id="$1"; shift ;; esac done if [[ -z "$track_id" ]]; then echo "Error: track ID required" >&2 exit 1 fi # Build request body local body body=$(jq -n --arg id "$track_id" '{track_id: $id}') if [[ -n "$duration" ]]; then body=$(echo "$body" | jq --argjson d "$duration" '. + {duration_seconds: $d}') fi if [[ -n "$hit_point" ]]; then body=$(echo "$body" | jq --argjson h "$hit_point" '. + {hit_point_seconds: $h}') fi # Determine output filename if [[ -z "$output" ]]; then # Get track title for filename local title title=$(curl -sS -X POST "$BASE_URL/search" \ -H "Authorization: Bearer $TRACKYARD_API_KEY" \ -H "Content-Type: application/json" \ -d "{\"query\":\"$track_id\",\"limit\":1}" | jq -r '.tracks[0].title // "track"' 2>/dev/null || echo "track") # Sanitize filename output=$(echo "$title" | tr ' ' '_' | tr -cd '[:alnum:]_-').mp3 fi echo "Downloading to: $output" >&2 curl -sS -X POST "$BASE_URL/download-track" \ -H "Authorization: Bearer $TRACKYARD_API_KEY" \ -H "Content-Type: application/json" \ -d "$body" \ --output "$output" echo "Downloaded: $output" } ``` ### Technical Analysis The `--output` argument is accepted as an unrestricted caller-controlled filesystem path. Although automatically generated filenames are sanitized, explicitly supplied output values receive no equivalent validation. The value is pas ...[truncated 2068 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Confine downloads to a dedicated directory under the project or user data directory. 2. Treat `--output` as a basename rather than a general path: - Reject absolute paths. - Reject `/`, `\`, `..`, control characters, and empty names. - Enforce an expected extension such as `.mp3`. 3. Canonicalize the destination directory and verify that the resolved path remains beneath the approved download root. 4. Refuse to overwrite existing files by default. Require an explicit, clearly documented `--force` option if replacement is necessary. 5. Reject symbolic-link destinations and validate the parent directory before writing. 6. Download to a securely created temporary file in the approved directory, validate HTTP status and expected content type, and then atomically rename it to the final destination. 7. Add `curl --fail-with-body` or equivalent HTTP error handling so error responses are not silently saved as successful media files. A safe design should resemble: ```bash download_dir="${TRACKYARD_DOWNLOAD_DIR:-$PWD/downloads}" mkdir -p -- "$download_dir" name="${output:-track.mp3}" if [[ "$name" == /* || "$name" == *"/"* || "$name" == *"\\"* || "$name" == *".."* || "$name" != *.mp3 ]]; then echo "Error: output must be a safe .mp3 filename" >&2 exit 1 fi destination="$download_dir/$name" if [[ -e "$destination" || -L "$destination" ]]; then echo "Error: destination already exists" >&2 exit 1 fi temporary=$(mktemp "$download_dir/.trackyard.XXXXXX") trap 'rm -f -- "$temporary"' EXIT curl --fail-with-body -sS -X POST "$BASE_URL/download-track" \ -H "Authorization: Bearer $TRACKYARD_API_KEY" \ -H "Content-Type: application/json" \ -d "$body" \ --output "$temporary" mv -- "$temporary" "$destination" trap - EXIT ``` ]]>