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 <zapabob@users.noreply.github.com>

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
 # ---------------------------------------------------------------------------

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)