T09 · Insecure Skill Coding Practices
Error
- Location
- tools/_skill_support/persona_review.py:465
- Finding
- Path Traversal Enables Arbitrary Directory Access, File Overwrite, and Recursive Deletion<![CDATA[ ## Vulnerability Details **File Location**: `tools/_skill_support/persona_review.py:465-526` **Vulnerability Type**: Path traversal and unrestricted filesystem operations **Risk Level**: High ### Vulnerable Code ```python def load_self_card_payload(cards_root: str | Path, card_id: str) -> dict[str, Any]: card_dir = Path(cards_root) / str(card_id or "").strip() if not card_dir.exists(): raise FileNotFoundError(card_id) meta = _load_card_meta(card_dir) profile_path = _resolve_card_profile_path(card_dir) if profile_path is None: raise FileNotFoundError(card_id) profile = load_profile_source(profile_path) fields = read_self_card_fields(profile) return { "card_id": card_dir.name, "fields": fields, "preview": build_self_card_preview(fields), "profile_path": str(profile_path.resolve()), "created_at": str(meta.get("created_at", "")).strip(), "updated_at": str(meta.get("updated_at", "")).strip(), } def save_self_card_payload(cards_root: str | Path, *, card_id: str, fields: dict[str, Any], utc_now: Callable[[], str]) -> dict[str, Any]: normalized = normalize_self_card_fields(fields) validate_self_card_fields(normalized) resolved_card_id = str(card_id or "").strip() or f"card-{uuid4().hex[:10]}" card_dir = Path(cards_root) / resolved_card_id if str(card_id or "").strip() and not card_dir.exists(): raise FileNotFoundError(card_id) card_dir.mkdir(parents=True, exist_ok=True) now = utc_now() meta = _load_card_meta(card_dir) if (card_dir / SELF_CARD_META_FILE).exists() else {} created_at = str(meta.get("created_at", "")).strip() or now profile = build_self_card_profile(normalized) (card_dir / "PROFILE.md").write_text(render_profile_md(profile), encoding="utf-8") (card_dir / SELF_CARD_META_FILE).write_text( json.dumps( { "card_id": resolved_card_id, "creat ...[truncated 4221 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Enforce a strict card identifier format** Accept only simple identifiers that cannot express paths: ```python import re CARD_ID_PATTERN = re.compile(r"^[A-Za-z0-9_-]{1,64}$") def validate_card_id(card_id: str) -> str: value = str(card_id or "").strip() if not CARD_ID_PATTERN.fullmatch(value): raise ValueError("Invalid card identifier") return value ``` 2. **Centralize secure path resolution** Resolve the root and candidate and verify containment before every load, save, or delete operation: ```python def resolve_card_dir(cards_root: str | Path, card_id: str) -> Path: safe_id = validate_card_id(card_id) root = Path(cards_root).resolve() candidate = (root / safe_id).resolve() if not candidate.is_relative_to(root): raise ValueError("Card path escapes the configured card root") return candidate ``` For Python versions without `Path.is_relative_to`, use `candidate.relative_to(root)` and reject `ValueError`. 3. **Explicitly reject absolute paths and traversal components** Defense in depth should reject IDs where `Path(card_id).is_absolute()` is true or where any component equals `..`, even if strict identifier validation is already present. 4. **Apply validation consistently** Replace direct path joins in `load_self_card_payload`, `save_self_card_payload`, and `delete_self_card_payload` with the same secure resolver. Validation must not be limited to deletion. 5. **Restrict deletion to expected card artifacts** Avoid recursively deleting every item beneath a selected path. Delete only files explicitly owned by the self-card feature, such as `PROFILE.md` and the known metadata file, and refuse deletion when unexpected content is present. 6. **Harden against links** Reject a card directory if it is a symbolic link. When stronger local-adversary protection is needed, inspect path components and ...[truncated 507 chars]