T09 · Insecure Skill Coding Practices
- Location
scripts/init-planning.sh:30- Finding
Unrestricted Task Slug Allows Path Traversal and File Overwrite
- Content
View full analysis
Vulnerability Details
File Location:
scripts/init-planning.sh, lines 30–73
Vulnerability Type: Path traversal and arbitrary fixed-name file overwrite
Risk Level: MediumVulnerable Code
bash SLUG="$1" PLAN_DIR=".planning/${DATE}-${SLUG}" # Check if directory already exists if [ -d "$PLAN_DIR" ]; then echo "Warning: Planning directory already exists: $PLAN_DIR" read -p "Overwrite? (y/N) " -n 1 -r echo if [[ ! $REPLY =~ ^[Yy]$ ]]; then echo "Aborted." exit 0 fi fi # Create directory mkdir -p "$PLAN_DIR" # Copy templates cp "$TEMPLATES_DIR/task_plan.md" "$PLAN_DIR/" cp "$TEMPLATES_DIR/findings.md" "$PLAN_DIR/" cp "$TEMPLATES_DIR/progress.md" "$PLAN_DIR/" # Replace placeholders with task slug TASK_NAME=$(echo "$SLUG" | sed 's/-/ /g' | awk '{for(i=1;i<=NF;i++) $i=toupper(substr($i,1,1)) tolower(substr($i,2))}1') # Update task_plan.md sed -i.bak "s/\[TASK_NAME\]/$TASK_NAME/g" "$PLAN_DIR/task_plan.md" rm -f "$PLAN_DIR/task_plan.md.bak" # Update findings.md sed -i.bak "s/\[TASK_NAME\]/$TASK_NAME/g" "$PLAN_DIR/findings.md" rm -f "$PLAN_DIR/findings.md.bak" # Update progress.md sed -i.bak "s/\[TASK_NAME\]/$TASK_NAME/g" "$PLAN_DIR/progress.md" rm -f "$PLAN_DIR/progress.md.bak" # Update timestamps TIMESTAMP=$(date '+%Y-%m-%d %H:%M') sed -i.bak "s/\[YYYY-MM-DD HH:MM\]/$TIMESTAMP/g" "$PLAN_DIR/task_plan.md" sed -i.bak "s/\[YYYY-MM-DD HH:MM\]/$TIMESTAMP/g" "$PLAN_DIR/findings.md" sed -i.bak "s/\[YYYY-MM-DD HH:MM\]/$TIMESTAMP/g" "$PLAN_DIR/progress.md" rm -f "$PLAN_DIR/"*.bakTechnical Analysis
The script incorporates the first command-line argument directly into
PLAN_DIRwithout enforcing a slug grammar or verifying the canonical destination path:bash PLAN_DIR=".planning/${DATE}-${SLUG}"Although the path is quoted, quoting only prevents shell word splitting and glob expansion. It does not prevent filesystem traversal through slash-separated
..components. A slug containing a normal component follow ...[truncated 2504 chars]- Remediation
View remediation
Remediation Suggestions
-
Enforce a strict slug grammar before constructing any path.
Accept only lowercase letters, digits, and internal hyphens, with a bounded length:
bash SLUG="$1" if [[ ! "$SLUG" =~ ^[a-z0-9][a-z0-9-]{0,63}$ ]]; then echo "Error: task slug must contain only lowercase letters, digits, and hyphens." exit 1 fi -
Reject path-related and control characters explicitly.
The validation must reject
/,\,.., whitespace, newlines, shell metacharacters, and empty normalized values. A strict allowlist is preferable to maintaining a denylist. -
Verify the canonical destination remains below
.planning.Resolve both the planning root and intended destination to canonical absolute paths, then verify that the destination begins with the planning root plus a path separator before performing writes.
-
Avoid broad wildcard deletion.
Replace:
bash rm -f "$PLAN_DIR/"*.bakwith explicit removal of only the backup files created by the script:
bash rm -f \ "$PLAN_DIR/task_plan.md.bak" \ "$PLAN_DIR/findings.md.bak" \ "$PLAN_DIR/progress.md.bak" -
Escape values used in
sedreplacement expressions.Escape backslashes, ampersands, and the selected delimiter before interpolating
TASK_NAME, or use a language with safer literal string replacement semantics. -
Fail safely on existing destination files.
Prefer creating a new directory atomically and refusing to overwrite existing files unless an explicit noninteractive overwrite option is supplied by the trusted caller.
-
Add regression tests.
Tests should confirm rejection of traversal and malformed inputs, including:
text ../target x/../../../target /tmp/target x\..\target task/name task&name task followed by a newline
-