T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/cleanup-zombie-browsers.sh:38
- Finding
- User-Controlled Filters Can Broaden Destructive Browser Process Termination<![CDATA[ ## Vulnerability Details **File Location**: `scripts/cleanup-zombie-browsers.sh:38-42`, `scripts/cleanup-zombie-browsers.sh:142-143`, `scripts/cleanup-zombie-browsers.sh:282-283`, and `scripts/cleanup-zombie-browsers.sh:330-369` **Vulnerability Type**: Unsafe user-controlled process selection and insufficient revalidation **Risk Level**: High ### Vulnerable Code Argument parsing permits arbitrary age thresholds and browser-identification patterns: ```bash --min-age) MIN_AGE_SECONDS="$2"; shift 2 ;; --grace) GRACE_PERIOD_SECONDS="$2"; shift 2 ;; --json) OUTPUT_FORMAT="json"; shift ;; --log) LOG_FILE="$2"; shift 2 ;; --pattern) OPENCLAW_BROWSER_PATTERN="$2"; shift 2 ;; ``` The user-controlled pattern is used as a substring filter. An empty pattern matches every command line: ```bash # Must have OpenClaw browser pattern in cmdline [[ "$cmdline" == *"$OPENCLAW_BROWSER_PATTERN"* ]] || continue ``` The user-controlled minimum age determines whether a selected process becomes a termination candidate: ```bash # Check: must be old enough if (( age_seconds < MIN_AGE_SECONDS )); then ``` Immediately before termination, only the command-line pattern is rechecked. The script then signals the process group: ```bash for pid in "${zombie_pids[@]}"; do # Verify process still exists and still matches criteria before killing if [[ "$OS" == "linux" ]] && [[ -d "/proc/$pid" ]]; then verify_cmdline="$(tr '\0' ' ' < "/proc/$pid/cmdline" 2>/dev/null)" || continue if [[ "$verify_cmdline" != *"$OPENCLAW_BROWSER_PATTERN"* ]]; then log "SKIP PID $pid: re-verification failed (cmdline changed)" continue fi fi # Count child processes that will also be terminated child_count=0 if [[ "$OS" == "linux" ]]; then child_count=$(grep -r "PPid:.*$pid" /proc ...[truncated 3832 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Remove arbitrary pattern overrides from destructive mode.** Use a fixed, canonical OpenClaw browser directory marker when `--kill` is enabled. 2. **Reject unsafe patterns.** If configurability is required, reject empty, whitespace-only, wildcard-like, relative, or overly broad values. Resolve the configured directory to a canonical absolute path and require it to remain beneath the expected OpenClaw browser directory. 3. **Validate numeric arguments before use.** Require `--min-age` and `--grace` to be bounded, non-negative decimal integers. For example: ```bash [[ "$MIN_AGE_SECONDS" =~ ^[0-9]+$ ]] || { echo "Invalid --min-age value" >&2 exit 2 } [[ "$GRACE_PERIOD_SECONDS" =~ ^[0-9]+$ ]] || { echo "Invalid --grace value" >&2 exit 2 } ``` Destructive mode should enforce a reasonable nonzero minimum age unless an explicitly named unsafe override is separately confirmed. 4. **Revalidate every safety property immediately before signaling.** Confirm current ownership, executable identity, canonical user-data directory, PPID/orphan state, minimum age, and process start time. 5. **Defend against PID reuse.** Record the process start time during discovery and compare it with the start time immediately before each signal. 6. **Avoid signaling unchecked process groups.** Signal only the verified candidate PID and descendants that have been independently enumerated and validated. If process-group signaling remains necessary, verify every group member first and abort when an unrelated member is present. 7. **Apply the same checks before SIGKILL.** Repeat identity and ownership validation after the grace period so that a changed or reused PID is not forcefully terminated. 8. **Update documentation and help text.** Clearly state the risks of any safety-control overrides, or remove `--pattern` from the public interface entirely. ]]>