T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/diagnose.py:148
- Finding
- Automatic Deletion of Workspace Files Without Confirmation or Sufficient Validation<![CDATA[ ## Vulnerability Details **File Location**: `scripts/diagnose.py`, lines 148-170 **Vulnerability Type**: Unsafe automatic file deletion **Risk Level**: Medium ### Vulnerable Code ```python def check_lock_files(): """锁文件清理（dry-run模式不删除；直接尝试删除，PermissionError表示被占用）""" if not WORKSPACE: return "skip", "无法确定工作区，跳过" removed = 0 skipped = 0 for f in os.listdir(WORKSPACE): if not (f.endswith(".lock") or f.endswith(".tmp")): continue path = os.path.join(WORKSPACE, f) try: age = datetime.now().timestamp() - os.path.getmtime(path) if age <= 600: continue if not DRY_RUN: os.remove(path) removed += 1 else: removed += 1 # dry-run 计数但不实际删除 except PermissionError: skipped += 1 except Exception: pass ``` ### Technical Analysis The diagnostic script deletes every top-level workspace entry whose name ends in `.lock` or `.tmp` and whose modification time is more than 600 seconds old. Deletion occurs during normal execution because dry-run behavior is opt-in through `--dry-run`. A filename suffix and modification age are insufficient to establish that a file is stale or safe to remove. The implementation does not: - Confirm that the file belongs to this skill or the OpenClaw application. - Validate an expected filename pattern or file contents. - Determine whether a process is still actively using the file. - Require explicit cleanup authorization from the user. - Restrict deletion to a dedicated, application-owned cleanup directory. - Confirm that the selected path represents an expected regular disposable file. This behavior also conflicts with the documented principle that the skill only diagnoses problems and that irreversible deletion requires confirmation. Although the documentation describes this cleanup as safely reversible, `os.remove()` does ...[truncated 1736 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Make diagnostic mode strictly read-only by default. Do not delete files unless the user supplies a separate explicit option such as `--cleanup-stale-locks`. 2. Require interactive confirmation immediately before deletion, listing every affected path. For unattended use, require an explicit high-risk acknowledgment flag. 3. Restrict cleanup to a dedicated application-owned lock directory rather than scanning the workspace root. 4. Use an allowlist of exact application-generated filename patterns instead of accepting every `.lock` or `.tmp` suffix. 5. Verify that each candidate is a regular file and belongs to the expected application before deletion. 6. Where lock files contain process identifiers or ownership metadata, validate that the associated process no longer exists and that the metadata matches the current workspace. 7. Replace silent exception handling with structured logging that records the affected path, operation, and error without exposing sensitive file contents. 8. Consider moving candidates to an application-controlled quarantine or recycle location first, allowing recovery before permanent deletion. 9. Update `SKILL.md` to state accurately that file deletion is irreversible and requires prior approval under the documented risk model. 10. Refactor the interface so cleanup is separate from diagnostics, for example: ```python parser.add_argument( "--cleanup-stale-locks", action="store_true", help="Explicitly request cleanup of validated application-owned stale lock files", ) if not args.cleanup_stale_locks: return "info", "Cleanup not requested; diagnostic scan only" ``` ]]>