T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/skill_sync.py:337
- Finding
- Destructive update proceeds after an incomplete backup<![CDATA[ ## Vulnerability Details **File Location**: `scripts/skill_sync.py:337-352` **Vulnerability Type**: Improper error handling during backup creation **Risk Level**: High ### Complete Code Snippet ```python dest = os.path.join(BACKUP_ROOT, slug, ts) os.makedirs(dest, exist_ok=True) for entry in os.listdir(slug_dir): if entry == SYNC_META: continue s, d = os.path.join(slug_dir, entry), os.path.join(dest, entry) try: shutil.copytree(s, d) if os.path.isdir(s) else shutil.copy2(s, d) except Exception as e: print(f" ⚠ 备份 {entry} 失败:{type(e).__name__}: {str(e)[:80]}") # 指针也写外部(写技能目录内会被 --force 一起删掉) try: os.makedirs(os.path.join(BACKUP_ROOT, slug), exist_ok=True) open(os.path.join(BACKUP_ROOT, slug, "last_backup.txt"), "w").write(dest) except Exception: pass ``` ### Technical Analysis The backup process catches errors for each file or directory, logs the failure, and continues. It then records the potentially incomplete directory as the latest backup. No success result, manifest comparison, file-count check, or hash verification is required before the caller proceeds. The update path subsequently invokes platform installation commands with `--force`. Those commands may replace the entire existing skill directory. Consequently, the claimed invariant that an update always has a complete rollback point is not enforced. Failures can result from unreadable files, insufficient storage, filesystem errors, path conflicts, interrupted copies, or destination collisions. This issue does not require elevated privileges, but it can destroy any files that the invoking user can modify in an installed skill directory. ### Attack Path 1. An installed skill contains one or more files that cannot be copied successfully, or the backup destination becomes unavailable or full. 2. The user runs `apply --execute`, or explicitly enables a scheduled `autorun --execute`. 3. `backup_dir()` catches the copy exception and contin ...[truncated 693 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Make `backup_dir()` fail closed: any copy failure must abort the backup and prevent installation. - Delete or quarantine partial backup directories after an error. - Generate a source manifest containing relative paths, file types, sizes, and SHA-256 hashes. - Compare the completed backup against that manifest before marking it valid. - Write `last_backup.txt` only after successful verification. - Use a temporary backup directory and atomically rename it after verification. - Propagate a structured success or failure result to `cmd_apply()`. - Refuse to invoke any force-installation command unless a verified backup exists. - Add tests covering unreadable files, disk exhaustion, interrupted copies, and partial directory-copy failures. ]]>
