get: reject copy destinations that resolve above the target

fsspec/asyn.py

@@ -15,7 +15,7 @@
 from .exceptions import FSTimeoutError
 from .implementations.local import LocalFileSystem, make_path_posix, trailing_sep
 from .spec import AbstractBufferedFile, AbstractFileSystem
-from .utils import glob_translate, is_exception, other_paths
+from .utils import check_contained, glob_translate, is_exception, other_paths
 
 private = re.compile("_[^_]")
 iothread = [None]  # dedicated fsspec IO thread
@@ -698,6 +698,11 @@ async def _get(
                 exists=exists,
                 flatten=not source_is_str,
             )
+            if isinstance(lpath, str):
+                # The names came from the source listing; ".." in one of them
+                # would otherwise place the copy above the destination. When
+                # lpath is a list the caller named every destination itself.
+                check_contained(lpath, lpaths)
 
         [os.makedirs(os.path.dirname(lp), exist_ok=True) for lp in lpaths]
         batch_size = kwargs.pop("batch_size", self.batch_size)

fsspec/implementations/tests/test_archive.py

@@ -380,3 +380,49 @@ def test_read_empty_file(self, scenario: ArchiveTestScenario):
         with scenario.provider(archive_data) as archive:
             fs = fsspec.filesystem(scenario.protocol, fo=archive)
             assert fs.open("a").read() == b""
+
+    def test_get_does_not_write_above_destination(
+        self, scenario: ArchiveTestScenario, tmp_path
+    ):
+        # Member names come from the archive, so a name holding ".." must not
+        # place the copy above the destination the caller asked for.
+        data = {"readme.txt": b"ok", "../escaped.txt": b"escaped"}
+        dest = tmp_path / "dest"
+        dest.mkdir()
+        outside = tmp_path / "escaped.txt"
+
+        with scenario.provider(data) as archive:
+            fs = fsspec.filesystem(scenario.protocol, fo=archive)
+            try:
+                fs.get("*", str(dest), recursive=True)
+            except ValueError:
+                pass
+
+        assert not outside.exists(), f"copy wrote {outside}, above {dest}"
+
+    def test_get_rejects_member_pointing_above_destination(
+        self, scenario: ArchiveTestScenario, tmp_path
+    ):
+        data = {"readme.txt": b"ok", "../escaped.txt": b"escaped"}
+        dest = tmp_path / "dest"
+        dest.mkdir()
+
+        with scenario.provider(data) as archive:
+            fs = fsspec.filesystem(scenario.protocol, fo=archive)
+            with pytest.raises(ValueError, match="outside the destination"):
+                fs.get("*", str(dest), recursive=True)
+
+    def test_get_keeps_dotdot_inside_destination(
+        self, scenario: ArchiveTestScenario, tmp_path
+    ):
+        # ".." that resolves within the destination stays a legitimate name.
+        data = {"plain.txt": b"ok", "a/b/../inner.txt": b"inner"}
+        dest = tmp_path / "dest"
+        dest.mkdir()
+
+        with scenario.provider(data) as archive:
+            fs = fsspec.filesystem(scenario.protocol, fo=archive)
+            fs.get("*", str(dest), recursive=True)
+
+        assert (dest / "plain.txt").read_bytes() == b"ok"
+        assert (dest / "a" / "inner.txt").read_bytes() == b"inner"

fsspec/implementations/tests/test_asyn_wrapper.py

@@ -1,5 +1,7 @@
 import asyncio
+import io
 import os
+import tarfile
 from itertools import cycle
 
 import pytest
@@ -218,3 +220,28 @@ async def test_deadlock_when_asynchronous():
 
     with pytest.raises(RuntimeError, match="Concurrent requests!"):
         await asyncio.gather(*(fs._cat_file(path) for path in paths))
+
+
+def test_get_does_not_write_above_destination(tmp_path):
+    # The async copy builds its destinations the same way the sync one does,
+    # so a source name holding ".." must not escape the destination there
+    # either. tar is synchronous, so it is wrapped to exercise
+    # AsyncFileSystem._get.
+    archive = tmp_path / "traversal.tar"
+    with tarfile.open(archive, "w") as t:
+        for name, data in [("readme.txt", b"ok"), ("../escaped.txt", b"escaped")]:
+            info = tarfile.TarInfo(name)
+            info.size = len(data)
+            t.addfile(info, io.BytesIO(data))
+
+    dest = tmp_path / "dest"
+    dest.mkdir()
+    outside = tmp_path / "escaped.txt"
+
+    fs = AsyncFileSystemWrapper(fsspec.filesystem("tar", fo=str(archive)))
+    try:
+        fs.get("*", str(dest), recursive=True)
+    except ValueError:
+        pass
+
+    assert not outside.exists(), f"async copy wrote {outside}, above {dest}"

fsspec/spec.py

@@ -18,6 +18,7 @@
 from .transaction import Transaction
 from .utils import (
     _unstrip_protocol,
+    check_contained,
     glob_translate,
     isfilelike,
     other_paths,
@@ -1057,6 +1058,11 @@ def get(
                 exists=exists,
                 flatten=not source_is_str,
             )
+            if isinstance(lpath, str):
+                # The names came from the source listing; ".." in one of them
+                # would otherwise place the copy above the destination. When
+                # lpath is a list the caller named every destination itself.
+                check_contained(lpath, lpaths)
 
         callback.set_size(len(lpaths))
         for lpath, rpath in callback.wrap(zip(lpaths, rpaths)):

fsspec/utils.py

@@ -422,6 +422,36 @@ def other_paths(
     return path2
 
 
+def check_contained(root: str, paths: list[str]) -> None:
+    """Raise if any of ``paths`` lies outside the destination ``root``.
+
+    Bulk copies build their destination names by joining source names onto a
+    destination root. Those names come from the source listing, so a name
+    holding ".." segments resolves above the root and writes outside the
+    destination the caller asked for.
+
+    Parameters
+    ----------
+    root: str
+        The destination the caller passed.
+    paths: list of str
+        The destination names built for that root.
+    """
+    root_abs = os.path.abspath(root)
+    # normcase so that a case-insensitive platform does not report a false
+    # escape, while the message keeps the paths as the caller would see them.
+    root_key = os.path.normcase(root_abs)
+    prefix = root_key.rstrip(os.sep) + os.sep
+    for path in paths:
+        path_abs = os.path.abspath(path)
+        path_key = os.path.normcase(path_abs)
+        if path_key != root_key and not path_key.startswith(prefix):
+            raise ValueError(
+                f"path {path!r} would be copied to {path_abs!r}, which is "
+                f"outside the destination {root!r}"
+            )
+
+
 def is_exception(obj: Any) -> bool:
     return isinstance(obj, BaseException)