T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/sentinel.py:636
- Finding
- Arbitrary Directory Deletion Through Unvalidated Skill Path## Vulnerability Details **File Location**: `scripts/sentinel.py`, lines 636-665 **Vulnerability Type**: Path traversal leading to arbitrary recursive directory deletion **Risk Level**: High ### Vulnerable Code ```python def cmd_reject(workspace, skill_name): skills_dir = workspace / "skills" skill_path = skills_dir / skill_name if not skill_path.exists(): qp = skills_dir / f"{QUARANTINE_PREFIX}{skill_name}" if qp.exists(): skill_path = qp else: print(f"Skill not found: {skill_name}"); return 1 if not skill_path.is_dir(): print(f"Not a skill directory: {skill_path}"); return 1 print("=" * 62); print("OPENCLAW SENTINEL FULL — REJECT SKILL"); print("=" * 62) print(f"Skill: {skill_name}\nTimestamp: {now_iso()}\n") tdb = load_threat_db(workspace) all_names = [d.name for d in collect_skill_dirs(workspace)] findings, score = scan_skill(skill_path, workspace, tdb, all_names) print(f" Risk Score: {score}/100 [{risk_label(score)}]\n Findings: {len(findings)}\n") if score < 50: print(f"[BLOCKED] Risk score {score} is below HIGH threshold (50).") print(f" Use 'quarantine {skill_name}' to disable without removal.") print(f" Reject is reserved for HIGH+ risk skills.\n"); return 1 evidence = {"skill": skill_name, "rejected_at": now_iso(), "risk_score": score, "risk_label": risk_label(score), "findings_count": len(findings), "findings": findings[:50], "original_path": str(skill_path), "file_inventory": file_inventory(skill_path)} ev_dir = quarantine_evidence_dir(workspace) save_json(ev_dir / f"{skill_name}-evidence.json", evidence) reject_dest = ev_dir / skill_name if reject_dest.exists(): shutil.rmtree(reject_dest) try: shutil.move(str(skill_path), str(reject_dest)) except OSError as e: print(f"Failed to move skill: {e}"); return 1 ``` ### Technical ...[truncated 3233 chars]
- Remediation
- ## Remediation Suggestions 1. **Restrict the input to a single directory name.** Reject absolute paths, empty values, `.` and `..`, path separators, and names that differ from `Path(skill_name).name`. ```python candidate = Path(skill_name) if ( candidate.is_absolute() or skill_name in {"", ".", ".."} or candidate.name != skill_name ): print("Invalid skill name") return 1 ``` 2. **Resolve and enforce path boundaries before filesystem operations.** ```python skills_dir = (workspace / "skills").resolve() skill_path = (skills_dir / skill_name).resolve() ev_dir = quarantine_evidence_dir(workspace).resolve() reject_dest = (ev_dir / skill_name).resolve() if skill_path.parent != skills_dir: print("Skill path escapes the skills directory") return 1 if reject_dest.parent != ev_dir: print("Destination escapes the evidence directory") return 1 ``` 3. **Ensure source and destination are different.** ```python if skill_path == reject_dest: print("Source and destination must differ") return 1 ``` 4. **Avoid recursively deleting an existing destination automatically.** Fail safely if the archive destination exists, or generate a unique destination name. If replacement is required, verify the resolved destination is a direct child of the evidence directory before deletion. 5. **Use a sanitized identifier for evidence filenames.** Do not interpolate an untrusted path into the filename. Generate a safe identifier or restrict names to a conservative allowlist such as letters, digits, underscores, periods, and hyphens. 6. **Apply the same validation to `quarantine` and `unquarantine`.** Although their current rename behavior does not expose the same direct `rmtree` sink, they also construct filesystem paths from `skill_name` and should enforce identical workspace boundaries. 7. **Add r ...[truncated 170 chars]