T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/package_skill.py:28
- Finding
- Unrestricted Archive Collection Can Disclose Local Files## Vulnerability Details **File Location**: `scripts/package_skill.py`, lines 28-31 **Vulnerability Type**: Unsafe archive creation and unintended file disclosure **Risk Level**: Medium ### Vulnerable Code ```python with zipfile.ZipFile(out_file, "w", zipfile.ZIP_DEFLATED) as zf: for path in skill_dir.rglob("*"): if path.is_file(): arcname = f"{skill_name}/{path.relative_to(skill_dir)}" zf.write(path, arcname) ``` ### Technical Analysis The packaging function recursively archives every file found below the supplied Skill directory. It does not: - Restrict packaging to an explicit allowlist. - Exclude secret-bearing files such as `.env` or private configuration files. - Exclude VCS metadata, temporary files, build artifacts, or the output directory. - Reject symbolic links before calling `is_file()` and `zf.write()`. - Resolve each candidate and verify that its target remains within the Skill directory. `Path.is_file()` follows symbolic links. Consequently, a symbolic link located inside the Skill directory can point to an arbitrary readable file elsewhere on the host, and `ZipFile.write()` can copy the target's contents into the generated archive. The archive entry retains the in-tree symbolic-link path, making the external origin of the packaged content non-obvious. If `--out` points to a directory inside `skill_dir`, the archive currently being generated may also be encountered by the recursive traversal. This can produce malformed, unexpectedly large, or non-deterministic artifacts. ### Attack Path 1. An attacker gains the ability to contribute files to the Skill directory, such as through a malicious pull request or compromised source archive. 2. The attacker adds a symbolic link beneath that directory pointing to a predictable sensitive file readable by the packaging user, or adds secret-bearing files that should not be distributed. 3. A maintainer runs `scripts/package_skill.py` without reviewing every recurs ...[truncated 1011 chars]
- Remediation
- ## Remediation Suggestions 1. **Use an explicit packaging allowlist.** Package only known distributable files and directories, such as `SKILL.md`, approved files under `references/`, and approved scripts under `scripts/`. 2. **Reject symbolic links explicitly.** ```python if path.is_symlink(): raise ValueError(f"Symbolic links are not allowed: {path}") ``` 3. **Enforce containment after resolution.** Resolve every candidate and verify that it remains beneath the resolved Skill root before reading it. ```python skill_root = skill_dir.resolve() resolved = path.resolve(strict=True) if not resolved.is_relative_to(skill_root): raise ValueError(f"Path escapes skill directory: {path}") ``` 4. **Exclude sensitive and generated content.** At minimum, reject `.env*`, credential and key files, `.git`, caches, temporary files, build outputs, and distribution directories. 5. **Require the output directory to be outside the Skill directory.** Resolve both paths and abort if the output directory equals or is nested beneath `skill_dir`. 6. **Precompute the file manifest before opening the output archive.** Validate the complete manifest first, then create the archive. This prevents the newly created archive from entering an active recursive traversal. 7. **Log and review the manifest.** Print every included relative path and optionally require confirmation before creating a release artifact. 8. **Add regression tests** covering external symbolic links, internal symbolic links, `.env` exclusion, nested output directories, and valid allowlisted packaging.