T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/nightly_skill_review.py:178- Finding
Arbitrary Proposal-Path Write Through Unsanitized Skill Names
- Content
View full analysis
dict[str, Path]: result = {} if scope == "all" and target_root.exists(): for path in sorted(target_root.iterdir()): if path.name.startswith("."): continue if (path / "SKILL.md").exists(): result[path.name] = path.resolve() if scope == "managed": for name, record in manifest.get("installs", {}).items(): target = Path(record.get("target", "")).expanduser() source = Path(record.get("source", "")).expanduser() for candidate in [target, source, target_root / name]: if candidate and (candidate / "SKILL.md").exists(): result.setdefault(name, candidate.resolve()) break own_skill = target_root / "skill-forge" if (own_skill / "SKILL.md").exists(): result.setdefault("skill-forge", own_skill.resolve()) if scope == "feedback": for entry in feedback_entries: name = entry.get("skill") if not name or name in result: continue target = target_root / name if (target / "SKILL.md").exists(): result[name] = target.resolve() return result ``` The unvalidated name is then used as a path component for proposal output: ```python def write_update_proposal(skill: str, skill_path: Path, issues: list[str], feedback: dict, args: argparse.Namespace) -> dict: if not args.propose_updates: return {"status": "disabled"} if not issu ...[truncated 3577 chars]- Remediation
View remediation
str: if not isinstance(name, str) or not SAFE_SKILL_NAME.fullmatch(name): raise ValueError("Invalid skill name") if name in {".", ".."}: raise ValueError("Invalid skill name") return name ``` 2. Reject names that are absolute or contain path separators, `.` components, or `..` components. 3. Resolve the final destination and verify containment before creating directories or writing files: ```python output_root = Path(args.update_output).expanduser().resolve() proposal_root = (output_root / date_stamp() / validated_skill).resolve() if proposal_root != output_root and output_root not in proposal_root.parents: raise ValueError("Proposal path escapes the configured output root") ``` 4. Apply validation consistently to names from both feedback entries and manifest keys. 5. Use exclusive creation or an atomic temporary-file replacement strategy where overwriting an existing proposal is not required. 6. Record rejected skill identifiers in the local report without reproducing potentially sensitive or malicious values in externally delivered Telegram messages. 7. Add regression tests covering absolute paths, `../` traversal, nested separators, empty values, non-string JSON values, and valid identifiers. ]]>