T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/skill_install.py:157
- Finding
- Archive-Controlled Skill Name Allows Filesystem Escape, Arbitrary Directory Creation, and Destructive Replacement## Vulnerability Details **File Location**: `scripts/skill_install.py`, lines 157-188 **Vulnerability Type**: Path traversal and unsafe filesystem operations **Risk Level**: High ### Vulnerable Code ```python skill_name = os.path.basename(skill_source) if skill_name == "openclaw_skill_temp": skill_md = os.path.join(skill_source, "SKILL.md") if os.path.exists(skill_md): with open(skill_md, 'r', encoding='utf-8') as f: for line in f: if line.startswith('name:'): skill_name = line.split(':', 1)[1].strip() break if skill_name == "openclaw_skill_temp": skill_name = os.path.splitext(os.path.basename(zip_path))[0] target_dir = os.path.join(self.skills_dir, skill_name) if os.path.exists(target_dir): response = input("是否覆盖? (y/N): ").strip().lower() if response != 'y': return False, "用户取消安装" shutil.rmtree(target_dir) shutil.copytree(skill_source, target_dir) ``` ### Technical Analysis The installer reads `skill_name` from an untrusted `SKILL.md` file inside the supplied archive and uses it directly to construct `target_dir`. It does not reject: - Absolute paths - `..` traversal components - Forward or backward path separators - Symlinked destination components - Paths that resolve outside the OpenClaw skills directory In Python, an absolute second argument to `os.path.join()` replaces the preceding directory. A value such as `/tmp/attacker-target` therefore causes `target_dir` to point outside `self.skills_dir`. A relative value such as `../../attacker-target` can similarly escape the intended directory after path resolution. The same untrusted destination is passed to both `shutil.rmtree()` and `shutil.copytree()`. Consequently, accepting the overwrite prompt can cause an existing directory outside the skills directory to be recursively deleted before being replaced with archive co ...[truncated 1559 chars]
- Remediation
- ## Remediation Suggestions - Parse the YAML frontmatter with a real YAML parser instead of line-based string processing. - Require the skill name to match a restrictive slug pattern, such as: ```python r"^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$" ``` - Explicitly reject absolute paths, `..`, `/`, `\`, null bytes, and platform-specific path prefixes. - Resolve both the skills directory and candidate destination with `Path.resolve()`. - Verify that the resolved destination is a direct child of the resolved skills directory before performing any operation. - Reject symlinks in the destination and its relevant path components. - Do not recursively delete a path derived solely from package metadata. - Implement safer replacement by copying to a newly created staging directory, validating it, and then atomically renaming it into place. - Display the fully resolved destination to the user before any destructive operation.