T09 · Insecure Skill Coding Practices
Warning
- Location
- astrill-watchdog.sh:223
- Finding
- Unsafe PID File Trust and Broad Process Matching Can Terminate Unrelated Processes## Vulnerability Details **File Location**: `astrill-watchdog.sh:223-256` **Vulnerability Type**: Unsafe process identification and termination **Risk Level**: Medium The standalone lifecycle commands trust a PID read from a persistent file without verifying the identity of the referenced process. They also use broad full-command-line matching to locate and terminate presumed orphan watchdog processes. Vulnerable code in `cmd_start`: ```bash cmd_start() { if [[ -f "$PID_FILE" ]]; then local old old="$(cat "$PID_FILE" 2>/dev/null)" || old="" if [[ -n "$old" ]] && kill -0 "$old" 2>/dev/null; then echo "Watchdog already running (PID ${old})." exit 0 fi rm -f "$PID_FILE" fi local orphans orphans="$(pgrep -f 'astrill-watchdog.sh _loop' 2>/dev/null || true)" if [[ -n "$orphans" ]]; then echo "Cleaning up orphaned loop processes: ${orphans}" echo "$orphans" | xargs kill 2>/dev/null || true sleep 1 fi nohup bash "$0" _loop >> "$LOG_FILE" 2>&1 & local new_pid="$!" echo "$new_pid" > "$PID_FILE" chmod 600 "$PID_FILE" echo "Watchdog started (PID ${new_pid}). Log: ${LOG_FILE}" } ``` Vulnerable code in `cmd_stop`: ```bash cmd_stop() { local killed=0 if [[ -f "$PID_FILE" ]]; then local pid pid="$(cat "$PID_FILE" 2>/dev/null)" || pid="" if [[ -n "$pid" ]] && kill "$pid" 2>/dev/null; then echo "Watchdog (PID ${pid}) stopped." killed=1 fi rm -f "$PID_FILE" fi local orphans orphans="$(pgrep -f 'astrill-watchdog.sh _loop' 2>/dev/null || true)" if [[ -n "$orphans" ]]; then echo "$orphans" | xargs kill 2>/dev/null || true echo "Killed orphaned loop processes: ${orphans}" killed=1 ...[truncated 2638 chars]
- Remediation
- ## Remediation Suggestions 1. Use systemd as the sole lifecycle authority when the watchdog is installed as a user service. Replace manual PID discovery and termination with: ```bash systemctl --user start astrill-watchdog.service systemctl --user stop astrill-watchdog.service systemctl --user is-active astrill-watchdog.service ``` 2. If standalone operation must remain, validate PID-file contents before using them: - Require a strictly numeric value. - Reject PID values less than or equal to 1. - Confirm `/proc/$pid` exists. - Verify `/proc/$pid/exe` resolves to the expected Bash executable or installed watchdog. - Verify the NUL-separated `/proc/$pid/cmdline` contains the exact installed script path and `_loop` argument. - Compare the process start time against a start-time value stored alongside the PID to detect PID reuse. 3. Use an exclusive lock, such as `flock`, held for the lifetime of the loop rather than relying on PID-file existence: ```bash exec 9>"$LOG_DIR/watchdog.lock" flock -n 9 || exit 0 ``` 4. Remove `pgrep -f | xargs kill`. If orphan recovery is required, enumerate candidate PIDs and independently validate each process's executable, exact argument vector, user ID, and installed script path before sending a signal. 5. After validation, signal one PID at a time using `kill -- "$pid"` and recheck its identity immediately before signaling to reduce time-of-check/time-of-use exposure. 6. Remove the PID file with an `EXIT`, `TERM`, and `INT` trap when running in standalone mode, while retaining identity validation because abrupt termination can bypass cleanup.