From 8f7567c325139a4cc7034002d7cacedf6271797b Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 6 Jun 2026 18:33:44 -0700 Subject: [PATCH] fix(bitwarden): prevent zip-slip path traversal when extracting bws binary (#40569) Salvaged from #40381; cleaned up, re-verified against main, tests added. Co-authored-by: zapabob --- agent/secret_sources/bitwarden.py | 34 +++++++++++- tests/test_bitwarden_secrets.py | 88 +++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/agent/secret_sources/bitwarden.py b/agent/secret_sources/bitwarden.py index b19451fda22..256e3294a10 100644 --- a/agent/secret_sources/bitwarden.py +++ b/agent/secret_sources/bitwarden.py @@ -324,8 +324,11 @@ def install_bws(*, force: bool = False) -> Path: with zipfile.ZipFile(zip_path) as zf: member = _pick_zip_member(zf, _platform_binary_name()) - zf.extract(member, tmp) - extracted = tmp / member + # Zip-slip guard: a malicious archive can carry member names like + # ``../../etc/cron.d/x`` or absolute paths. ``ZipFile.extract`` + # joins the member onto ``tmp`` without verifying the result stays + # inside it, so validate containment before touching the disk. + extracted = _safe_extract_member(zf, member, tmp) # Move into place atomically. We write to a sibling tempfile in # the final directory so the rename can't cross filesystems. @@ -395,6 +398,33 @@ def _pick_zip_member(zf: zipfile.ZipFile, binary_name: str) -> str: return candidates[0] +def _safe_extract_member( + zf: zipfile.ZipFile, member: str, dest_dir: Path +) -> Path: + """Extract a single archive member, refusing path traversal. + + ``ZipFile.extract`` will happily honour member names containing + ``../`` or absolute paths, letting a malicious archive write outside + ``dest_dir`` (a "zip-slip"). We resolve the would-be target and + confirm it stays within ``dest_dir`` before extracting. + """ + dest_root = os.path.realpath(dest_dir) + target = os.path.realpath(os.path.join(dest_root, member)) + # ``commonpath`` raises ValueError for e.g. different drives on + # Windows; treat that as an escape too. + try: + contained = os.path.commonpath([dest_root, target]) == dest_root + except ValueError: + contained = False + if not contained or target == dest_root: + raise RuntimeError( + f"Refusing to extract unsafe archive member {member!r}: " + f"it escapes the extraction directory" + ) + zf.extract(member, dest_root) + return Path(target) + + # --------------------------------------------------------------------------- # Secret fetch + apply # --------------------------------------------------------------------------- diff --git a/tests/test_bitwarden_secrets.py b/tests/test_bitwarden_secrets.py index dbedd0cb7a5..ac5057c18b8 100644 --- a/tests/test_bitwarden_secrets.py +++ b/tests/test_bitwarden_secrets.py @@ -98,6 +98,94 @@ def _make_fake_zip(binary_bytes: bytes) -> bytes: return buf.getvalue() +# --------------------------------------------------------------------------- +# _safe_extract_member — zip-slip containment +# --------------------------------------------------------------------------- + + +def test_safe_extract_member_extracts_normal_member(tmp_path): + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr("bws", b"hello") + buf.seek(0) + + dest = tmp_path / "extract" + dest.mkdir() + with zipfile.ZipFile(buf) as zf: + out = bw._safe_extract_member(zf, "bws", dest) + + assert out == (dest / "bws").resolve() or out == dest / "bws" + assert Path(out).read_bytes() == b"hello" + # Nothing escaped the destination directory. + assert Path(out).resolve().parent == dest.resolve() + + +@pytest.mark.parametrize( + "evil_name", + [ + "../escape", + "../../escape", + "sub/../../escape", + ], +) +def test_safe_extract_member_rejects_traversal(tmp_path, evil_name): + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr(evil_name, b"pwned") + buf.seek(0) + + dest = tmp_path / "extract" + dest.mkdir() + outside = tmp_path / "escape" + + with zipfile.ZipFile(buf) as zf: + with pytest.raises(RuntimeError, match="unsafe archive member"): + bw._safe_extract_member(zf, evil_name, dest) + + # The traversal target must not have been written. + assert not outside.exists() + + +def test_safe_extract_member_rejects_absolute_path(tmp_path): + # An absolute member name should never resolve inside dest. + abs_member = "/etc/cron.d/evil" if os.name != "nt" else "C:/Windows/evil" + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr(abs_member, b"pwned") + buf.seek(0) + + dest = tmp_path / "extract" + dest.mkdir() + with zipfile.ZipFile(buf) as zf: + # Absolute paths are reduced to a relative member by zipfile, but + # we exercise the guard directly with the raw escaping name too. + with pytest.raises(RuntimeError, match="unsafe archive member"): + bw._safe_extract_member(zf, "../../../etc/cron.d/evil", dest) + + +def test_install_bws_rejects_malicious_member(hermes_home, monkeypatch): + # Build an archive whose only matching member escapes the temp dir. + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr(f"../../{bw._platform_binary_name()}", b"pwned") + zip_bytes = buf.getvalue() + asset_name = bw._platform_asset_name() + checksum_text = f"{hashlib.sha256(zip_bytes).hexdigest()} {asset_name}\n" + + def fake_download(url, dest): + if url.endswith(".zip"): + Path(dest).write_bytes(zip_bytes) + elif url.endswith(".txt"): + Path(dest).write_text(checksum_text) + else: + raise AssertionError(f"unexpected download url: {url}") + + monkeypatch.setattr(bw, "_http_download", fake_download) + + with pytest.raises(RuntimeError, match="unsafe archive member"): + bw.install_bws() + + def test_install_bws_happy_path(hermes_home, monkeypatch): fake_binary = b"#!/bin/sh\necho 'bws fake 2.0.0'\n" zip_bytes = _make_fake_zip(fake_binary)