Fix path traversal via metadata file_name in folder-based builders

src/datasets/packaged_modules/folder_based_builder/folder_based_builder.py

@@ -2,6 +2,7 @@
 import io
 import itertools
 import os
+import posixpath
 from dataclasses import dataclass
 from typing import Any, Callable, Iterator, Optional, Union
 
@@ -401,8 +402,47 @@ def set_feature(item, feature_path: _VisitPath):
                         )
                     elif len(feature_path) == 0:
                         if item is not None:
+                            # Guard against path traversal (CWE-22): a crafted `file_name` such as
+                            # "../../etc/passwd" or an absolute path must not be able to escape the
+                            # metadata file's directory and read arbitrary files on the host.
+                            #
+                            # The attacker-controlled `file_name` must be a plain relative path. In
+                            # particular it must not introduce an fsspec URL scheme: `file://` and
+                            # `local://` resolve to arbitrary *local* files, and any other scheme
+                            # would sidestep the containment check below. Legitimate reads from a
+                            # downloaded archive use a `zip://<file_name>::<container>` URL where the
+                            # scheme lives on `downloaded_metadata_dir` (the container), never on the
+                            # `file_name` value itself, so forbidding "://" here does not break them.
+                            if "://" in item:
+                                raise ValueError(
+                                    f"Invalid metadata file_name '{item}': `file_name` must be a relative path "
+                                    f"pointing inside the directory containing the metadata file. URL schemes "
+                                    f"(e.g. 'file://', 'local://') are not allowed."
+                                )
                             file_relpath = os.path.normpath(item).replace("\\", "/")
-                            item = os.path.join(downloaded_metadata_dir, file_relpath)
+                            if (
+                                os.path.isabs(item)
+                                or os.path.isabs(file_relpath)
+                                or file_relpath == ".."
+                                or file_relpath.startswith("../")
+                            ):
+                                raise ValueError(
+                                    f"Invalid metadata file_name '{item}': `file_name` must be a relative path "
+                                    f"pointing inside the directory containing the metadata file. Absolute paths "
+                                    f"and parent-directory ('..') traversal that escape the dataset directory are "
+                                    f"not allowed."
+                                )
+                            resolved_reference = os.path.join(downloaded_metadata_dir, file_relpath)
+                            # Defense in depth: confirm the resolved reference stays inside the
+                            # metadata directory. This works for local dirs (resolving symlinks) as
+                            # well as for the fsspec URLs (possibly chained with "::") produced when
+                            # reading from a downloaded archive. See `_metadata_reference_is_contained`.
+                            if not _metadata_reference_is_contained(downloaded_metadata_dir, resolved_reference):
+                                raise ValueError(
+                                    f"Invalid metadata file_name '{item}': the resolved path escapes the "
+                                    f"dataset directory containing the metadata file."
+                                )
+                            item = resolved_reference
                     return item
 
                 for pa_metadata_table in self._read_metadata(downloaded_metadata_file, metadata_ext=metadata_ext):
@@ -431,6 +471,47 @@ def set_feature(item, feature_path: _VisitPath):
                     yield Key(shard_idx, sample_idx), sample
 
 
+def _metadata_reference_is_contained(downloaded_metadata_dir: str, resolved_reference: str) -> bool:
+    """Return whether ``resolved_reference`` stays inside ``downloaded_metadata_dir``.
+
+    ``downloaded_metadata_dir`` is the directory containing the metadata file and
+    ``resolved_reference`` is a metadata ``file_name`` joined onto it. Both may be plain local
+    paths or fsspec URLs, and either may be a chained URL that uses the "::" hop separator to
+    navigate a downloaded archive, e.g.:
+
+    * ``/path/to/local/metadata_dir``
+    * ``hf://path/to/remote/metadata_dir``
+    * ``zip://metadata_dir::/path/to/local/archive.zip``
+    * ``zip://metadata_dir::hf://path/to/remote/archive.zip``
+
+    Joining a relative ``file_name`` only extends the first "::" component (the path *inside* the
+    archive, or the whole reference when there is no archive); the remaining components describe
+    the fixed container and must not change. So containment is checked on the first component,
+    while every subsequent component must stay identical.
+    """
+    root_parts = downloaded_metadata_dir.split("::")
+    reference_parts = resolved_reference.split("::")
+    # The container part(s) after the first hop are fixed by the download step; a `file_name`
+    # must never alter them.
+    if root_parts[1:] != reference_parts[1:]:
+        return False
+    root, reference = root_parts[0], reference_parts[0]
+    if "://" in root:
+        # fsspec URL component: normalize textually with posixpath. os.path must not be used here
+        # because on Windows it rewrites "/" as "\\" and collapses the "//" of the URL scheme.
+        root_norm = posixpath.normpath(root)
+        reference_norm = posixpath.normpath(reference)
+        return reference_norm == root_norm or reference_norm.startswith(root_norm + "/")
+    # Genuine local path: resolve symlinks, then confirm containment.
+    real_root = os.path.realpath(root)
+    real_reference = os.path.realpath(reference)
+    try:
+        return os.path.commonpath([real_root, real_reference]) == real_root
+    except ValueError:
+        # commonpath raises ValueError for paths on different drives (Windows).
+        return False
+
+
 def _nested_apply(item: Any, feature_path: _VisitPath, func: Callable[[Any, _VisitPath], Any]):
     # see _visit_with_path() to see how feature paths are constructed
     item = func(item, feature_path)

tests/packaged_modules/test_folder_based_builder.py

@@ -1,4 +1,6 @@
 import importlib
+import json
+import os
 import shutil
 import textwrap
 
@@ -11,6 +13,7 @@
 from datasets.packaged_modules.folder_based_builder.folder_based_builder import (
     FolderBasedBuilder,
     FolderBasedBuilderConfig,
+    _metadata_reference_is_contained,
 )
 
 
@@ -449,6 +452,206 @@ def test_data_files_with_wrong_metadata_file_name(cache_dir, tmp_path, auto_text
     assert all("additional_feature" not in example for _, example in generator)
 
 
+@pytest.mark.parametrize(
+    "malicious_file_name",
+    [
+        "../outside/secret.txt",  # relative traversal
+        "../../outside/secret.txt",  # deeper relative traversal
+        "subdir/../../outside/secret.txt",  # traversal hidden after a valid segment
+        "dummy/../../../etc/passwd",  # valid segment then escapes past the root (review on #8325)
+        "subdir/../../../../outside/secret.txt",  # deep escape after a valid segment
+    ],
+)
+def test_data_files_with_metadata_path_traversal_is_rejected(cache_dir, tmp_path, auto_text_file, malicious_file_name):
+    # Regression test for https://github.com/huggingface/datasets/issues/8324
+    # A crafted `file_name` must not be able to escape the metadata file's directory.
+    secret_dir = tmp_path / "outside"
+    secret_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, secret_dir / "secret.txt")
+
+    data_dir = tmp_path / "data_dir_with_traversal_metadata"
+    data_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, data_dir / "file.txt")
+    metadata_filename = data_dir / "metadata.jsonl"
+    metadata = f'{{"file_name": {json.dumps(malicious_file_name)}, "additional_feature": "Malicious file"}}\n'
+    with open(metadata_filename, "w", encoding="utf-8") as f:
+        f.write(metadata)
+
+    data_files = DataFilesDict.from_patterns(get_data_patterns(str(data_dir)), data_dir.as_posix())
+    autofolder = DummyFolderBasedBuilder(data_files=data_files, cache_dir=cache_dir)
+    gen_kwargs = autofolder._split_generators(StreamingDownloadManager())[0].gen_kwargs
+    with pytest.raises(ValueError, match="Invalid metadata file_name"):
+        list(autofolder._generate_examples(**gen_kwargs))
+
+
+def test_data_files_with_metadata_absolute_path_is_rejected(cache_dir, tmp_path, auto_text_file):
+    # Regression test for https://github.com/huggingface/datasets/issues/8324
+    # An absolute `file_name` must not be able to read files outside the dataset directory.
+    secret_dir = tmp_path / "outside"
+    secret_dir.mkdir(parents=True, exist_ok=True)
+    secret_file = secret_dir / "secret.txt"
+    shutil.copyfile(auto_text_file, secret_file)
+
+    data_dir = tmp_path / "data_dir_with_absolute_metadata"
+    data_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, data_dir / "file.txt")
+    metadata_filename = data_dir / "metadata.jsonl"
+    metadata = f'{{"file_name": {json.dumps(str(secret_file))}, "additional_feature": "Malicious file"}}\n'
+    with open(metadata_filename, "w", encoding="utf-8") as f:
+        f.write(metadata)
+
+    data_files = DataFilesDict.from_patterns(get_data_patterns(str(data_dir)), data_dir.as_posix())
+    autofolder = DummyFolderBasedBuilder(data_files=data_files, cache_dir=cache_dir)
+    gen_kwargs = autofolder._split_generators(StreamingDownloadManager())[0].gen_kwargs
+    with pytest.raises(ValueError, match="Invalid metadata file_name"):
+        list(autofolder._generate_examples(**gen_kwargs))
+
+
+@pytest.mark.parametrize(
+    "malicious_file_name",
+    [
+        "file://../../outside/secret.txt",  # fsspec local scheme + traversal
+        "local://../../outside/secret.txt",  # fsspec local scheme + traversal
+        "file://../outside/secret.txt",  # scheme where os.path.normpath mangles the "../" away
+        "file:///etc/passwd",  # absolute-looking local scheme
+        "file://outside/secret.txt",  # scheme without traversal
+    ],
+)
+def test_data_files_with_metadata_url_scheme_is_rejected(cache_dir, tmp_path, auto_text_file, malicious_file_name):
+    # Regression test for the review on https://github.com/huggingface/datasets/pull/8325:
+    # fsspec schemes such as `file://` / `local://` resolve to local files and previously bypassed
+    # the containment check. A `file_name` carrying any URL scheme ("://") must be rejected.
+    secret_dir = tmp_path / "outside"
+    secret_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, secret_dir / "secret.txt")
+
+    data_dir = tmp_path / "data_dir_with_scheme_metadata"
+    data_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, data_dir / "file.txt")
+    metadata_filename = data_dir / "metadata.jsonl"
+    metadata = f'{{"file_name": {json.dumps(malicious_file_name)}, "additional_feature": "Malicious file"}}\n'
+    with open(metadata_filename, "w", encoding="utf-8") as f:
+        f.write(metadata)
+
+    data_files = DataFilesDict.from_patterns(get_data_patterns(str(data_dir)), data_dir.as_posix())
+    autofolder = DummyFolderBasedBuilder(data_files=data_files, cache_dir=cache_dir)
+    gen_kwargs = autofolder._split_generators(StreamingDownloadManager())[0].gen_kwargs
+    with pytest.raises(ValueError, match="Invalid metadata file_name"):
+        list(autofolder._generate_examples(**gen_kwargs))
+
+
+def test_data_files_with_metadata_symlink_escape_is_rejected(cache_dir, tmp_path, auto_text_file):
+    # Regression test for the review on https://github.com/huggingface/datasets/pull/8325:
+    # the string-based checks (URL scheme / absolute path / ".." traversal) only see a plain,
+    # innocent-looking relative `file_name` and let it through. If a subdirectory of the dataset
+    # is actually a symlink pointing outside the dataset directory, a reference such as
+    # "evil_link/secret.txt" resolves to a file outside the dataset root. Only the realpath-based
+    # containment check (`_metadata_reference_is_contained`) catches this, so this test guards that
+    # second layer of defense: with the string checks alone the reference would be read.
+    secret_dir = tmp_path / "outside"
+    secret_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, secret_dir / "secret.txt")
+
+    data_dir = tmp_path / "data_dir_with_symlink_metadata"
+    data_dir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, data_dir / "file.txt")
+
+    # A subdirectory of the dataset that is really a symlink escaping the dataset root. Creating
+    # symlinks may require privileges (e.g. Windows without Developer Mode); skip when unavailable.
+    # The test still runs on Linux CI, where this attack matters.
+    evil_link = data_dir / "evil_link"
+    try:
+        evil_link.symlink_to(secret_dir, target_is_directory=True)
+    except (OSError, NotImplementedError) as e:
+        pytest.skip(f"symlink creation not available on this platform: {e!r}")
+
+    metadata_filename = data_dir / "metadata.jsonl"
+    metadata = '{"file_name": "evil_link/secret.txt", "additional_feature": "Malicious file"}\n'
+    with open(metadata_filename, "w", encoding="utf-8") as f:
+        f.write(metadata)
+
+    data_files = DataFilesDict.from_patterns(get_data_patterns(str(data_dir)), data_dir.as_posix())
+    autofolder = DummyFolderBasedBuilder(data_files=data_files, cache_dir=cache_dir)
+    gen_kwargs = autofolder._split_generators(StreamingDownloadManager())[0].gen_kwargs
+    # The string checks accept this relative path; only the realpath-based containment check
+    # rejects it, with this specific message. Asserting on that message ensures the test would
+    # fail (the file would be read) if the containment check were removed.
+    with pytest.raises(ValueError, match="the resolved path escapes the dataset directory"):
+        list(autofolder._generate_examples(**gen_kwargs))
+
+
+def test_metadata_reference_containment_across_scheme_forms(tmp_path):
+    # Regression test for the review on https://github.com/huggingface/datasets/pull/8325:
+    # the containment check must actually run (not be skipped) for URLs and correctly handle
+    # chained "::" references. It must work for the four forms `downloaded_metadata_dir` can take:
+    #   1. a local path                          -> /path/to/metadata_dir
+    #   2. a remote URL                          -> hf://path/to/metadata_dir
+    #   3. a chained URL over a local archive    -> zip://metadata_dir::/path/to/archive.zip
+    #   4. a chained URL over a remote archive   -> zip://metadata_dir::hf://path/to/archive.zip
+    # `os.path.join` is patched to the fsspec-aware `xjoin` once a builder is instantiated, which is
+    # exactly how `set_feature` joins the reference at runtime, so use it here too.
+    _ = DummyFolderBasedBuilder(data_dir=".")  # triggers the streaming patch of os.path.join
+    from datasets.packaged_modules.folder_based_builder import folder_based_builder as fbb
+
+    xjoin = fbb.os.path.join
+
+    # a real local metadata dir (with a real subdir) so realpath-based containment can resolve
+    local_dir = tmp_path / "metadata_dir"
+    (local_dir / "subdir").mkdir(parents=True, exist_ok=True)
+    (tmp_path / "outside").mkdir(parents=True, exist_ok=True)
+
+    forms = {
+        "local": local_dir.as_posix(),
+        "hf_remote": "hf://datasets/user/repo@main/metadata_dir",
+        "zip_local": "zip://metadata_dir::/data/local/archive.zip",
+        "zip_remote": "zip://metadata_dir::hf://datasets/user/repo@main/archive.zip",
+    }
+    for name, metadata_dir in forms.items():
+        # a legitimate relative reference must be considered contained
+        legit = xjoin(metadata_dir, "subdir/file2.txt")
+        assert _metadata_reference_is_contained(metadata_dir, legit), f"legit ref rejected for {name}"
+
+        # a reference pointing at a sibling of the metadata dir must be rejected
+        head, _, _ = metadata_dir.partition("::")
+        sibling_head = head.rsplit("/", 1)[0] + "/evil_sibling"
+        escaping = sibling_head + metadata_dir[len(head) :]
+        assert not _metadata_reference_is_contained(metadata_dir, escaping), f"sibling escape allowed for {name}"
+
+        # for chained URLs, tampering with the container part (after "::") must be rejected
+        if "::" in metadata_dir:
+            tampered = legit.split("::")[0] + "::/attacker/other.zip"
+            assert not _metadata_reference_is_contained(metadata_dir, tampered), f"container tamper allowed for {name}"
+
+
+def test_data_files_with_metadata_legitimate_subdir_reference(cache_dir, tmp_path, auto_text_file):
+    # A legitimate `file_name` pointing to a file in a subdirectory of the metadata
+    # file's directory must keep working after the path traversal fix (#8324).
+    data_dir = tmp_path / "data_dir_with_subdir_metadata"
+    data_dir.mkdir(parents=True, exist_ok=True)
+    subdir = data_dir / "subdir"
+    subdir.mkdir(parents=True, exist_ok=True)
+    shutil.copyfile(auto_text_file, data_dir / "file.txt")
+    shutil.copyfile(auto_text_file, subdir / "file2.txt")
+    metadata_filename = data_dir / "metadata.jsonl"
+    metadata = textwrap.dedent(
+        """\
+        {"file_name": "file.txt", "additional_feature": "Same-dir file"}
+        {"file_name": "subdir/file2.txt", "additional_feature": "Subdir file"}
+        {"file_name": "./subdir/file2.txt", "additional_feature": "Subdir file with dot prefix"}
+        """
+    )
+    with open(metadata_filename, "w", encoding="utf-8") as f:
+        f.write(metadata)
+
+    data_files = DataFilesDict.from_patterns(get_data_patterns(str(data_dir)), data_dir.as_posix())
+    autofolder = DummyFolderBasedBuilder(data_files=data_files, cache_dir=cache_dir)
+    gen_kwargs = autofolder._split_generators(StreamingDownloadManager())[0].gen_kwargs
+    examples = [example for _, example in autofolder._generate_examples(**gen_kwargs)]
+    assert len(examples) == 3
+    assert all(example["base"] is not None for example in examples)
+    assert all(os.path.isfile(example["base"]) for example in examples)
+
+
 def test_data_files_with_custom_file_name_column_in_metadata_file(cache_dir, tmp_path, auto_text_file):
     data_dir = tmp_path / "data_dir_with_custom_file_name_metadata"
     data_dir.mkdir(parents=True, exist_ok=True)