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<![CDATA[ ## Vulnerability Details **File Location**: `scripts/quick-log.sh`, lines 13-22 **Vulnerability Type**: Path traversal and arbitrary file append **Risk Level**: Medium ### Vulnerable Code ```bash SKILL_NAME="$1" DURATION="$2" QUALITY="$3" DATE=$(date +%Y%m%d) TIMESTAMP=$(date -Iseconds) # 创建技能目录（如果不存在） mkdir -p ".learnings/skills" # 追加日志条目 cat >> ".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
- <![CDATA[ ## Remediation Suggestions 1. **Apply a strict allowlist to the skill name.** Accept only a limited filename-safe character set, such as ASCII letters, digits, underscores, and hyphens: ```bash SKILL_NAME="$1" if [[ ! "$SKILL_NAME" =~ ^[A-Za-z0-9_-]+$ ]]; then printf '%s\n' "Error: skill name may contain only letters, digits, underscores, and hyphens." >&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]