T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/schedule_sync.sh:36
- Finding
- Persistent Command Execution Through Unsafe Cron Entry Construction<![CDATA[ ## Vulnerability Details **File Location**: `scripts/schedule_sync.sh:36-60` and `scripts/schedule_sync.sh:112-120` **Vulnerability Type**: Cron command injection **Risk Level**: High ### Vulnerable Code ```bash setup_cron() { local directory="$1" local interval="$2" # Validate directory if [ ! -d "$directory" ]; then echo -e "${RED}Error: directory does not exist - $directory${NC}" exit 1 fi # Create log directory mkdir -p "$LOG_DIR" # Create cron task local cron_job="0 */$interval * * * $SYNC_SCRIPT \"$directory\" >> \"$LOG_FILE\" 2>&1" # Check whether a task already exists if crontab -l 2>/dev/null | grep -q "$SYNC_SCRIPT"; then echo -e "${YELLOW}Scheduled task already exists; updating...${NC}" crontab -l 2>/dev/null | grep -v "$SYNC_SCRIPT" | crontab - fi # Add the new task (crontab -l 2>/dev/null; echo "$cron_job") | crontab - } ``` The interval is accepted without validation: ```bash INTERVAL=1 while [[ $# -gt 0 ]]; do case $1 in --interval) INTERVAL="$2" shift 2 ;; ``` ### Technical Analysis The script interpolates the user-controlled `interval` and `directory` values directly into a crontab command. Cron executes task commands through a shell, so quoting the directory with double quotes does not prevent command substitution, variable expansion, or quote termination when the task later runs. The `interval` parameter can also contain spaces or newline characters because it is not restricted to an integer. Such input can alter the cron schedule or introduce an additional cron entry. The directory existence check does not eliminate the issue. Unix paths can contain spaces, quotes, dollar signs, parentheses, and other shell-significant characters. An attacker can create a directory whose literal name contains a command-substitution expression and then register that directory. Sched ...[truncated 1488 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Validate `interval` as a bounded decimal integer before constructing the schedule: ```bash if ! [[ "$interval" =~ ^[0-9]+$ ]] || (( interval < 1 || interval > 24 )); then echo "Invalid interval" >&2 exit 1 fi ``` 2. Reject directory values containing newlines, carriage returns, NUL-equivalent input, or other characters that cannot be represented safely in a crontab entry. 3. Do not embed an arbitrary directory directly into a shell command. Store the canonical directory in a user-owned configuration file with mode `0600`, and install a fixed cron command that reads the configuration without evaluating it as shell syntax. 4. If direct insertion is unavoidable, use a rigorously tested shell-quoting routine rather than double quotes alone. 5. Resolve and validate the directory with `realpath`, and ensure the cron wrapper treats the resulting value strictly as data. 6. Add a unique marker to the managed cron entry and modify only that exact entry rather than removing every line that happens to contain the script path. 7. Display the complete proposed cron entry and require explicit confirmation before installation. ]]>