T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/quick-log.sh:13- Finding
Path Traversal Through Unvalidated Skill Name Permits File Append Outside the Log Directory
- Content
View full analysis
> ".learnings/skills/${SKILL_NAME}.md" << EOF ``` The unsafe construction is also reproduced in the documentation at `SKILL.md`, lines 188-196: ```bash #!/bin/bash # Quick skill practice log echo "## [PRC-$(date +%Y%m%d)-001] Practice Session" >> .learnings/skills/$1.md echo "**Logged**: $(date -Iseconds)Z" >> .learnings/skills/$1.md echo "**Duration**: $2 minutes" >> .learnings/skills/$1.md echo "**Quality Score**: $3/10" >> .learnings/skills/$1.md echo "" >> .learnings/skills/$1.md echo "### What I Practiced" >> .learnings/skills/$1.md echo "- " >> .learnings/skills/$1.md ``` ### Technical Analysis The first positional argument is accepted as `SKILL_NAME` without validation and interpolated directly into the destination path: ```bash ".learnings/skills/${SKILL_NAME}.md" ``` Quoting prevents shell word splitting and wildcard expansion in the executable script, but it does not prevent filesystem path traversal. Values containing `../` can escape `.learnings/skills` after normal path resolution. Because the redirection operator is `>>`, the script creates a missing destination or appends generated Markdown to an existing destination. The forced `.md` suffix limits directly selectable filenames, but an attacker can still modify any writable file whose path ends in `.md`. The operation also follows filesystem symbolic links when the selected destination is a symbolic link. The example in `SKILL.md` has the same traversal weakness and is additionally unquoted. If copied into another script, whitespace and wildcard characters in `$1` can trigger shell word splitting or pathname expansion. This is not s ...[truncated 1791 chars]- Remediation
View remediation
&2 exit 1 fi ``` 2. **Construct and verify canonical paths.** Resolve both the log directory and destination, then ensure the destination remains beneath the approved directory: ```bash LOG_DIR="$(realpath -m -- ".learnings/skills")" mkdir -p -- "$LOG_DIR" DESTINATION="$(realpath -m -- "$LOG_DIR/${SKILL_NAME}.md")" case "$DESTINATION" in "$LOG_DIR"/*) ;; *) printf '%s\n' "Error: destination escapes the log directory." >&2 exit 1 ;; esac ``` Canonical containment validation should supplement, rather than replace, strict filename validation. 3. **Defend against symbolic-link destinations where appropriate.** If logs must only be regular files, reject symbolic links before writing: ```bash if [[ -L "$DESTINATION" ]]; then printf '%s\n' "Error: symbolic-link destinations are not allowed." >&2 exit 1 fi ``` For security-sensitive or concurrently writable directories, use a safer file-opening implementation that rejects symbolic links atomically. 4. **Validate the remaining arguments.** Require `DURATION` to be a positive integer and `QUALITY` to be an integer from 1 through 10. This prevents malformed or deceptive records even though these fields do not currently create command injection. 5. **Update `SKILL.md`.** Replace the unsafe example with the validated implementation and quote every path expansion. Do not teach users to construct paths directly from `$1`. 6. **Add regression tests.** Verify that names such as `../../../tmp/test`, `foo/bar`, `..`, empty strings, nam ...[truncated 164 chars]