T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/install.sh:34
- Finding
- Unvalidated configuration is injected into persistent scheduler definitions<![CDATA[ ## Vulnerability Details **File Location**: `scripts/install.sh:34-35`, `scripts/install.sh:47-70`, and `scripts/install.sh:91-100` **Vulnerability Type**: Scheduler configuration and command injection **Risk Level**: High ### Vulnerable Code ```bash # Load check interval CHECK_INTERVAL=$(jq -r '.checkIntervalMin // 10' "$CONFIG_FILE") ``` ```bash cat > "$SERVICE_FILE" <<EOF [Unit] Description=OpenClaw Job Execution Monitor healthcheck After=network.target [Service] Type=oneshot ExecStart=${HEALTHCHECK_SCRIPT} Environment="OPENCLAW_WORKSPACE=${WORKSPACE}" StandardOutput=journal StandardError=journal [Install] WantedBy=default.target EOF cat > "$TIMER_FILE" <<EOF [Unit] Description=OpenClaw Job Execution Monitor timer Requires=openclaw-job-execution-monitor.service [Timer] OnBootSec=2min OnUnitActiveSec=${CHECK_INTERVAL}min AccuracySec=1min [Install] WantedBy=timers.target EOF ``` ```bash CRON_LINE="*/${CHECK_INTERVAL} * * * * OPENCLAW_WORKSPACE=${WORKSPACE} ${HEALTHCHECK_SCRIPT} >> ${WORKSPACE}/job-execution-monitor.log 2>&1" # Check if already in crontab if crontab -l 2>/dev/null | grep -qF "$HEALTHCHECK_SCRIPT"; then echo "⚠️ Cron entry already exists" else (crontab -l 2>/dev/null; echo "$CRON_LINE") | crontab - echo "✅ Cron job added" fi ``` ### Technical Analysis `CHECK_INTERVAL` is loaded from a user-writable JSON configuration without verifying that it is an integer within a safe range. `WORKSPACE` is derived from `OPENCLAW_WORKSPACE` without rejecting newlines, quotes, whitespace, or shell metacharacters. These values are directly interpolated into systemd unit files or a crontab entry. A newline in either value can introduce additional systemd directives or cron records. In the cron fallback, shell metacharacters in the workspace value can alter the command interpreted by the cron shell. Because the generated definition is enabled and started, successful injection is persistent and executes whenever the timer or cron entry ...[truncated 1379 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require `checkIntervalMin` to be a JSON number and a bounded positive integer: ```bash CHECK_INTERVAL=$(jq -er ' .checkIntervalMin // 10 | select(type == "number" and floor == . and . >= 1 and . <= 1440) ' "$CONFIG_FILE") || { echo "ERROR: checkIntervalMin must be an integer from 1 to 1440" >&2 exit 1 } ``` 2. Canonicalize the workspace path and reject control characters, newlines, carriage returns, and NUL bytes. 3. Escape values according to systemd unit-file syntax rather than inserting raw environment values into a heredoc. 4. Avoid assembling a cron shell command from configurable text. Use a fixed wrapper script whose path is controlled by the package and pass validated configuration through a protected environment file. 5. Quote all shell path uses within the generated cron command using a robust shell-escaping mechanism. 6. Generate definitions in a temporary file with restrictive permissions, validate them, and then atomically move them into place. 7. Require explicit user confirmation before enabling the persistent timer, especially when a non-default workspace or interval is used. ]]>