T09 · Insecure Skill Coding Practices
Warning
- Location
- dx-monitor.py:203
- Finding
- Predictable Temporary State File Allows Symlink-Based File Overwrite## Vulnerability Details **File Location**: `dx-monitor.py:20` and `dx-monitor.py:203-214` **Vulnerability Type**: Unsafe temporary-file handling and symbolic-link following **Risk Level**: Medium ```python STATE_FILE = "/tmp/dx-monitor-state.json" ``` ```python def load_state() -> Dict: """Load previous state.""" try: with open(STATE_FILE) as f: return json.load(f) except: return {'last_spots': [], 'last_check': 0} def save_state(state: Dict): """Save state.""" with open(STATE_FILE, 'w') as f: json.dump(state, f, indent=2) ``` ### Technical Analysis The monitor stores state at the fixed, globally predictable path `/tmp/dx-monitor-state.json`. The file is opened using ordinary `open()` operations without: - Verifying that the path is not a symbolic link. - Verifying ownership or file type. - Creating an owner-only state directory. - Setting explicit restrictive permissions. - Using exclusive or atomic file creation. - Writing to a protected temporary file and atomically replacing the destination. On systems where applicable symbolic-link protections are absent, disabled, or bypassable, another local user can create the expected path as a symbolic link before the monitor creates it. When `save_state()` opens the path using write mode, Python follows the link and truncates the linked destination. The broad exception handler in `load_state()` also conceals ownership, format, and access errors, making manipulated state appear equivalent to missing state and reducing visibility into an attack. ### Attack Path 1. The attacker confirms that the victim runs `dx-monitor.py watch --new-only`, potentially through the documented recurring cron configuration. 2. Before the victim creates the state file, the attacker creates `/tmp/dx-monitor-state.json` as a symbolic link to a chosen file. 3. The chosen target must be writable by the account tha ...[truncated 1642 chars]
- Remediation
- ## Remediation Suggestions 1. Replace the shared `/tmp` path with an account-specific state directory, preferably: - `$XDG_STATE_HOME/ham-radio-dx/state.json`, or - `~/.local/state/ham-radio-dx/state.json`. 2. Create the parent directory with permissions `0700`. 3. Create state files with permissions `0600`. 4. Reject symbolic links and non-regular files using `os.lstat()` and, where supported, `os.open()` with `O_NOFOLLOW`. 5. Write to a temporary file in the same protected directory, flush and synchronize it, and then use `os.replace()` for atomic publication. 6. Verify that any existing state file is owned by the current effective user. 7. Catch specific exceptions and report unsafe ownership, file-type, and permission conditions rather than silently treating all errors as missing state. 8. Correct the documentation inconsistency: `SKILL.md` states both `~/dx-monitor-state.json` and `/tmp/dx-monitor-state.json`. Document only the hardened account-specific location. A secure implementation should follow this pattern: ```python import json import os import tempfile from pathlib import Path state_root = Path( os.environ.get( "XDG_STATE_HOME", Path.home() / ".local" / "state" ) ) / "ham-radio-dx" state_root.mkdir(mode=0o700, parents=True, exist_ok=True) os.chmod(state_root, 0o700) state_file = state_root / "state.json" def save_state(state): fd, temporary_path = tempfile.mkstemp( prefix=".state-", dir=state_root ) try: os.fchmod(fd, 0o600) with os.fdopen(fd, "w") as output: json.dump(state, output, indent=2) output.flush() os.fsync(output.fileno()) os.replace(temporary_path, state_file) except Exception: try: os.unlink(temporary_path) except FileNotFoundError: pass raise ```