diff --git a/src/datasets/packaged_modules/folder_based_builder/folder_based_builder.py b/src/datasets/packaged_modules/folder_based_builder/folder_based_builder.py index 82586c648cb..b9ee4949f1d 100644 --- a/src/datasets/packaged_modules/folder_based_builder/folder_based_builder.py +++ b/src/datasets/packaged_modules/folder_based_builder/folder_based_builder.py @@ -401,8 +401,54 @@ 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://::` 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("\\", "/") + 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." + ) item = os.path.join(downloaded_metadata_dir, file_relpath) + # For local metadata directories, additionally resolve symlinks and confirm + # containment. Skipped when the metadata dir is itself an fsspec URL (which + # contains "://", e.g. "zip://...::..." from a downloaded archive) since + # realpath is not scheme-aware and would corrupt the URL. + if "://" not in downloaded_metadata_dir: + real_root = os.path.realpath(downloaded_metadata_dir) + real_path = os.path.realpath(item) + try: + is_contained = os.path.commonpath([real_root, real_path]) == real_root + except ValueError: + # commonpath raises ValueError for paths on different drives (Windows) + is_contained = False + if not is_contained: + raise ValueError( + f"Invalid metadata file_name '{item}': the resolved path escapes the " + f"dataset directory containing the metadata file." + ) return item for pa_metadata_table in self._read_metadata(downloaded_metadata_file, metadata_ext=metadata_ext): diff --git a/tests/packaged_modules/test_folder_based_builder.py b/tests/packaged_modules/test_folder_based_builder.py index 6880377776f..e3126f52096 100644 --- a/tests/packaged_modules/test_folder_based_builder.py +++ b/tests/packaged_modules/test_folder_based_builder.py @@ -1,4 +1,6 @@ import importlib +import json +import os import shutil import textwrap @@ -449,6 +451,121 @@ 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 + ], +) +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_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)