diff --git a/agent/curator_backup.py b/agent/curator_backup.py index 5e39443bae0d8..70ef5271ed35a 100644 --- a/agent/curator_backup.py +++ b/agent/curator_backup.py @@ -605,13 +605,21 @@ def rollback(backup_id: Optional[str] = None) -> Tuple[bool, str, Optional[Path] with tarfile.open(archive, "r:gz") as tf: # Python 3.12+ supports filter='data' for safer extraction. # Fall back to the unfiltered call for older interpreters but - # still reject absolute paths and .. components defensively. + # still reject unsafe members defensively. filter='data' is only + # available on 3.12+ and on 3.11.4+ (PEP 706 backport) — earlier + # 3.11 patch releases hit the fallback unconditionally, so the + # pre-check has to refuse anything that isn't a plain file or + # directory. Mirrors hermes_cli/profiles._safe_extract_profile_archive. for member in tf.getmembers(): name = member.name if name.startswith("/") or ".." in Path(name).parts: raise tarfile.TarError( f"refusing to extract unsafe path: {name!r}" ) + if not (member.isfile() or member.isdir()): + raise tarfile.TarError( + f"refusing to extract unsupported tar member type: {name!r}" + ) try: tf.extractall(str(skills), filter="data") # type: ignore[call-arg] except TypeError: diff --git a/tests/agent/test_curator_backup.py b/tests/agent/test_curator_backup.py index b375f98688f57..08bc75bfa874e 100644 --- a/tests/agent/test_curator_backup.py +++ b/tests/agent/test_curator_backup.py @@ -259,6 +259,70 @@ def test_rollback_rejects_unsafe_tarball(backup_env, monkeypatch): assert "unsafe" in msg.lower() or "refus" in msg.lower() or "extract" in msg.lower() +def test_rollback_rejects_symlink_member(backup_env, monkeypatch, tmp_path): + """Tarballs with symlink/hardlink/device members must be refused at the + pre-check stage, not relying on ``filter='data'`` which is unavailable on + Python 3.11.0–3.11.3 (PEP 706 was only backported in 3.11.4). + + Without a pre-check on the member type, the fallback + ``tf.extractall(str(skills))`` call materialises an attacker-supplied + symlink via ``os.symlink(linkname, targetpath)``. A snapshot tarball + placed in ``.curator_backups/`` (e.g. by a malicious skill, a + co-tenant with write access, or a user tricked into importing a + third-party 'snapshot') can then plant ``skills/leak → /etc/passwd``, + leaking the target's content to the agent on the next skill_view. + """ + cb = backup_env["cb"] + skills = backup_env["skills"] + _write_skill(skills, "alpha") + cb.snapshot_skills(reason="legit") + + # Plant a victim file outside the skills tree. After a successful + # exploit, this file's contents become readable from inside skills/. + victim = tmp_path / "secret" + victim.write_text("CONFIDENTIAL", encoding="utf-8") + + # Replace the legit snapshot tarball with one whose only member is a + # symlink pointing at the victim. The member name itself is safe + # ("leak", no "/" prefix, no ".."), so only a type-aware pre-check + # can stop it. + rows = cb.list_backups() + snap_dir = Path(rows[0]["path"]) + mal = snap_dir / "skills.tar.gz" + mal.unlink() + with tarfile.open(mal, "w:gz") as tf: + info = tarfile.TarInfo(name="leak") + info.type = tarfile.SYMTYPE + info.linkname = str(victim) + tf.addfile(info) + + # Force the legacy fallback branch (``filter`` kwarg unavailable) so we + # exercise the same code path Python 3.11.0–3.11.3 hits unconditionally. + real_extractall = tarfile.TarFile.extractall + + def no_filter_extractall(self, *args, **kwargs): + if "filter" in kwargs: + raise TypeError( + "extractall() got an unexpected keyword argument 'filter'" + ) + return real_extractall(self, *args, **kwargs) + + monkeypatch.setattr(tarfile.TarFile, "extractall", no_filter_extractall) + + ok, msg, _ = cb.rollback() + assert not ok, f"rollback should refuse a symlink member, got msg={msg!r}" + + # The symlink must never have materialised under skills/, and the + # victim's contents must not be reachable from inside skills/. + leak = skills / "leak" + assert not leak.is_symlink(), ( + f"symlink should be rejected before extract; resolved to " + f"{leak.resolve() if leak.exists() else ''}" + ) + if leak.exists(): + assert leak.read_text(encoding="utf-8") != "CONFIDENTIAL" + + # --------------------------------------------------------------------------- # Integration with run_curator_review # ---------------------------------------------------------------------------