T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/restore_state.py:214
- Finding
- Unsafe Archive Extraction and Restore Path Traversal<![CDATA[ ## Vulnerability Details **File Location**: `scripts/restore_state.py:57-68`, `scripts/restore_state.py:214-217`, and `scripts/restore_state.py:248-257` **Vulnerability Type**: Unsafe archive extraction and insufficient destination-path validation **Risk Level**: High ### Vulnerable Code ```python def archive_to_dest(rel: str, workspace: Path, state_dir: Path) -> Path: rel_path = Path(rel) parts = rel_path.parts if len(parts) < 3: raise RuntimeError(f"Unexpected archive path: {rel}") scope = parts[1] remainder = Path(*parts[2:]) if scope == "workspace": return workspace / remainder if scope == "state": return state_dir / remainder raise RuntimeError(f"Unknown archive scope: {scope}") ``` ```python with tempfile.TemporaryDirectory(prefix="openclaw-restore-") as td: tmpdir = Path(td) with tarfile.open(archive, "r:gz") as tar: tar.extractall(tmpdir) ``` ```python rollback_archive = make_rollback(workspace, state_dir, rollback_dir, include_prefixes, exclude_prefixes) restored_paths = [] for item in verified_files: src = tmpdir / item["path"] dst = archive_to_dest(item["path"], workspace, state_dir) dst.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(src, dst) restored_paths.append(str(dst)) ``` ### Technical Analysis The restore operation passes every archive member to `tar.extractall()` before validating the archive manifest, member types, or member paths. On Python versions or configurations that do not enforce a safe extraction filter, malicious absolute paths, parent-directory components, symbolic links, hard links, or special archive entries can cause extraction outside the temporary directory. There is a second, independent path-containment issue in `archive_to_dest()`. The function checks the archive scope but does not reject `..` components, absolute remainders, or resolved paths outside the selected workspace or state directory. For example, ...[truncated 2091 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Validate every archive member before extraction** - Reject absolute paths. - Reject paths containing `..`. - Reject symbolic links, hard links, device nodes, FIFOs, and other non-regular entries. - Reject duplicate members and files not declared in the manifest. - Require member names to follow one of the exact permitted layouts: - `mutable/workspace/...` - `mutable/state/...` - `static/workspace/...` 2. **Use safe extraction controls** - On supported Python versions, use an appropriate safe extraction filter. - Retain explicit path and type validation because runtime defaults vary. - Prefer extracting validated regular files individually instead of calling unrestricted `extractall()`. 3. **Enforce source and destination containment** - Resolve the candidate path and its intended root. - Require the candidate to remain under that root using `Path.is_relative_to()` or an equivalent containment check. - Apply this validation both to temporary extraction paths and final restore destinations. ```python def contained_path(root: Path, relative: Path) -> Path: root = root.resolve() if relative.is_absolute() or ".." in relative.parts: raise RuntimeError(f"Unsafe relative path: {relative}") candidate = (root / relative).resolve() if not candidate.is_relative_to(root): raise RuntimeError(f"Path escapes allowed root: {relative}") return candidate ``` 4. **Validate before any filesystem write** - Load the manifest without extracting arbitrary archive content. - Validate its schema, path prefixes, unique entries, checksums, and member correspondence. - Only after successful structural validation should regular files be written to the temporary directory. 5. **Add adversarial tests** - Test absolute member names and `../` traversal. - Test traversal in manifest paths. - Test symbolic-link and hard-link entries. - Test duplicate ...[truncated 121 chars]