T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/install_skill.py:16
- Finding
- Unvalidated installation name permits directory escape and recursive deletion<![CDATA[ ## Vulnerability Details **File Location**: `scripts/install_skill.py`, lines 16-21 and 54-65 **Vulnerability Type**: Path traversal leading to arbitrary filesystem deletion and replacement **Risk Level**: High ### Vulnerable Code ```python def copytree(src: pathlib.Path, dst: pathlib.Path, force: bool): if dst.exists(): if not force: raise SystemExit(f"target exists: {dst} (pass --force to overwrite)") shutil.rmtree(dst) ignore = shutil.ignore_patterns("__pycache__", "*.pyc", ".DS_Store") shutil.copytree(src, dst, ignore=ignore) ``` ```python parser.add_argument("--target", default=str(DEFAULT_TARGET), help="Skills directory, default: OPENCLAW_SKILLS_DIR or ~/.openclaw/skills") parser.add_argument("--name", default=PACKAGE_DIR.name, help="Installed directory name") parser.add_argument("--force", action="store_true", help="Overwrite an existing installed copy") parser.add_argument("--skip-package-verify", action="store_true", help="Skip bundled checksum verification before install") parser.add_argument("--verify-backend", action="store_true", help="Run scripts/verify_backend.py after install when present") args = parser.parse_args() if not args.skip_package_verify: code = run_package_verify(PACKAGE_DIR) if code != 0: return code target = pathlib.Path(args.target).expanduser().resolve() dest = target / args.name target.mkdir(parents=True, exist_ok=True) copytree(PACKAGE_DIR, dest, args.force) ``` ### Technical Analysis The installer treats `--name` as a trusted directory name but does not verify that it is a single safe path component. `pathlib` permits this argument to contain parent-directory components such as `../` or to be an absolute path. If `args.name` is absolute, the expression `target / args.name` resolves to the absolute value and discards the intended target prefix. If it contains `../`, filesystem operations normalize those components when accessing the path, allowing the dest ...[truncated 1794 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require `--name` to be exactly one directory component: - Reject absolute paths. - Reject `.` and `..`. - Reject `/`, `\`, and platform-specific path separators. - Reject names for which `pathlib.Path(name).name != name`. 2. Resolve the final destination and enforce containment before any write or deletion: ```python target = pathlib.Path(args.target).expanduser().resolve() name = pathlib.Path(args.name) if name.is_absolute() or name.name != args.name or args.name in {".", ".."}: raise SystemExit("--name must be a single safe directory name") dest = (target / name).resolve() if dest.parent != target: raise SystemExit("installation destination escapes the selected target") ``` 3. Before calling `shutil.rmtree()`, repeat the containment check and explicitly refuse dangerous destinations such as the filesystem root, home directory, target root, or package source directory. 4. Consider replacing destructive overwrite behavior with an atomic backup-and-rename process. Require explicit confirmation when deleting a nonempty destination. 5. Add regression tests covering absolute names, `../` traversal, nested names, symlink-related edge cases, empty names, and platform-specific separators. ]]>