T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/plan.sh:552- Finding
Regular Expression Injection in Task Completion Logic
- Content
View full analysis
/dev/null; then # Escape sed special chars in task name TASK_SED=$(printf '%s' "$TASK" | sed 's/[&/\]/\\&/g') sed -i.bak "s/\[ \] ${TASK_SED}/[x] ${TASK_SED}/" "$TODAY_FILE" rm -f "${TODAY_FILE}.bak" echo "✅ Marked complete: $TASK" else # Add to wins section echo "" >> "$TODAY_FILE" echo "- × $TASK (added retroactively)" >> "$TODAY_FILE" fi ``` ### Technical Analysis The `done` command accepts user-controlled task text and incorporates it into a `sed` basic regular expression. The escaping operation only attempts to handle `&`, `/`, and `\`. It does not escape regular-expression metacharacters such as: - `.` - `*` - `[` - `]` - `^` - `$` The initial `grep -Fq` check treats the task as a fixed string, but the subsequent `sed` command interprets the same value as a regular expression. Consequently, the validation and modification operations use different matching semantics. For example, if the planner contains both of the following tasks: ```markdown - [ ] a.b - [ ] axb ``` Running: ```bash ./scripts/plan.sh done 'a.b' ``` passes the fixed-string check because the literal task `a.b` exists. In the `sed` expression, however, `.` matches any character. Both task lines can therefore match and be modified. Because the user value is also used as replacement text, the second task may additionally be rewritten as `a.b`, causing further data corruption. Malformed expressions involving brackets or other operators may also cause `sed` to fail. Since the script uses `set -e`, such an error can terminate the command unexpectedly. This flaw does not provide shell command execution because the task value remains inside a quoted shell expansion and is not evaluated as shell syntax. The vulnerabil ...[truncated 1314 chars]- Remediation
View remediation
"$tmp_file" mv -- "$tmp_file" "$TODAY_FILE" trap - EXIT ``` Additional hardening measures: 1. Match the complete task line rather than a substring to prevent ambiguous updates. 2. Modify only the first exact match unless duplicate-task behavior is explicitly defined. 3. Create temporary files in the destination directory so the final rename remains atomic. 4. Use `mktemp` rather than a predictable `.bak` filename. 5. Install a cleanup trap so temporary files are removed after errors or interruptions. 6. Add regression tests using task names containing `.`, `*`, `[`, `]`, `^`, `$`, `/`, `&`, and backslashes. 7. If `sed` must be retained, independently escape every basic-regex metacharacter in the search value and every replacement metacharacter in the replacement value. Exact string processing remains preferable. ]]>