T09 · Insecure Skill Coding Practices
Warning
- Location
SKILL.md:128- Finding
Configurable Scratch Directory Allows Deletion of Unrelated Files
- Content
View full analysis
/dev/null # start clean trap 'rm -f "$TMP"/*.pdf 2>/dev/null' EXIT # belt-and-braces cleanup (also done explicitly in Step 6) ``` The final cleanup expands the set of affected files: ```bash rm -f "$TMP"/*.pdf "$TMP"/*.txt "$TMP"/discover.json "$TMP"/matches.tsv "$TMP"/selected.tsv "$TMP"/http_code 2>/dev/null ``` The directory originates from user-editable configuration: ```bash TMP=$(jq -r '.tmp_dir' "$SRC") ``` ### Technical Analysis The Skill treats `tmp_dir` as a dedicated scratch directory but does not enforce that assumption. It neither creates a unique per-run subdirectory nor verifies that the configured path is reserved exclusively for this Skill. At startup, every file matching `*.pdf` in the configured directory is removed. At normal exit, every matching `*.pdf` and `*.txt` file is removed. These wildcard operations are not restricted to files created by the current execution. Although shell quoting prevents ordinary whitespace expansion of the directory value, it does not prevent destructive behavior when the quoted value identifies an existing shared directory. A legitimate configuration error, such as setting `tmp_dir` to `/tmp`, a workspace, or a document directory, is sufficient to trigger data loss. The exit trap only removes PDFs, so it does not fully implement the stated cleanup policy on failure. Conversely, the explicit successful cleanup is overly broad because it removes all text and PDF files in t ...[truncated 1491 chars]- Remediation
View remediation
&2 exit 1 ;; esac mkdir -p -- "$TMP_PARENT" || exit 1 chmod 700 -- "$TMP_PARENT" || exit 1 RUN_TMP=$(mktemp -d "$TMP_PARENT/run.XXXXXXXX") || { echo "ERROR: unable to create private run directory" >&2 exit 1 } cleanup() { case "$RUN_TMP" in "$TMP_PARENT"/run.*) rm -rf -- "$RUN_TMP" ;; *) echo "ERROR: refusing unsafe cleanup path" >&2 ;; esac } trap cleanup EXIT HUP INT TERM ``` Additional hardening measures: 1. Use `RUN_TMP` for every downloaded or generated file. 2. Refuse empty paths, `/`, and paths that are not absolute. 3. Resolve canonical paths and reject known shared or sensitive directories unless a private child directory is created within them. 4. Set restrictive permissions with `umask 077`. 5. Never remove `"$TMP"/*.pdf` or `"$TMP"/*.txt` from a user-selected parent directory. 6. If recursive deletion is used, guard it with strict prefix and non-empty-path validation. 7. Track exact generated filenames if a unique run directory cannot be used. ]]>