From 97eadb888d2f99567e3113f4e455be4ca128f697 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 1 Oct 2026 19:17:13 +0200 Subject: [PATCH] fix(submodule): validate destinations before mutation Pretty much a rubber-stamp. It won't be out there long as the replacement with CLI + Gix is already on the way. Submodule checkout destinations could pass the containment check and be rejected by the index only after cloning had changed the filesystem. This addresses `GHSA-83vg-56qc-22m7` at the shared destination boundaries, including initialization and moves as well as creation. Reuse `_validate_repo_path` before checkout mutations to enforce portable NTFS/HFS metadata-alias checks and invalid-path rejection. Validate Windows filenames and submodule-name NULs before creating directories. Compare path components with `Repo.git_dir` and `Repo.common_dir` by filesystem identity, so separately named metadata directories and their aliases are protected too. Reject metadata destinations nested inside another submodule's Git directory before cloning, reuse, or renaming. Repeat the check after cloning and disable a clone that became nested. Preflight implicit metadata renames during moves, while preserving supported metadata symlinks and relocation of a submodule's own metadata directory. Git reference: `d38352cd43ab9745686d697872408bc3249a153f`, particularly `read-cache.c::verify_path_internal`, NTFS/HFS recognition, `compat/mingw.c::is_valid_win32_path`, and `submodule.c::validate_submodule_git_dir`. Related Git tests are in `t/t7450-bad-git-dotfiles.sh` and `t/t7406-submodule-update.sh`. Regression tests first demonstrated writes before rejection and acceptance of nested and separately named metadata destinations. Tests use harmless file content and compare portable aliases with native Git index validation. Coverage includes all 16 HFS ignored characters, Windows filename rules, relative and absolute paths, metadata reuse, and nesting during cloning. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6.0 Astra --- doc/source/changes.rst | 13 ++ git/objects/submodule/base.py | 91 ++++++++++++-- test/test_submodule.py | 225 ++++++++++++++++++++++++++++++++++ 3 files changed, 318 insertions(+), 11 deletions(-) diff --git a/doc/source/changes.rst b/doc/source/changes.rst index 2f537c99d..8447228db 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,19 @@ Changelog ========= +3.2.1 +===== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-83vg-56qc-22m7 + +If you can, also try and provide feedback on the upcoming v4 branch +https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.2.1 + 3.2.0 ===== diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 15fe877e6..74fbd39fe 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -8,6 +8,7 @@ import ntpath import os import os.path as osp +import re import shlex import stat import sys @@ -47,6 +48,7 @@ IterableList, RemoteProgress, _to_relative_path, + _validate_repo_path, join_path_native, rmtree, to_native_path_linux, @@ -305,20 +307,55 @@ def _config_parser_constrained(self, read_only: bool) -> SectionConstraint: def _validated_name(cls, name: str) -> str: if ( not name + or "\0" in name or name.startswith(("/", "\\")) or ntpath.splitdrive(name)[0] or ".." in name.replace("\\", "/").split("/") ): raise ValueError("Invalid submodule name %r" % name) + cls._validate_windows_path(name) return name + @staticmethod + def _validate_windows_path(path: PathLike) -> None: + """Apply Git for Windows' filename checks before creating directories.""" + if sys.platform == "win32": + for component in ntpath.splitdrive(os.fspath(path))[1].replace("\\", "/").split("/"): + if component in (".", ".."): + continue + stem = component.split(".", 1)[0].rstrip(" ").upper() + if ( + component.endswith((" ", ".")) + or any(ord(char) < 32 or char in '<>:"|?*' for char in component) + or re.fullmatch(r"CON(?:IN\$|OUT\$)?|PRN|AUX|NUL|COM[1-9]|LPT[1-9]", stem) + ): + raise ValueError("Invalid submodule path on Windows: %r" % path) + @classmethod - def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> PathLike: + def _module_abspath( + cls, parent_repo: "Repo", path: PathLike, name: str, *, moving_from: Union[PathLike, None] = None + ) -> PathLike: + """Reject nested Git directories, allowing the source of a pending rename.""" + from git.repo.fun import is_git_dir + name = cls._validated_name(name) if cls._need_gitfile_submodules(parent_repo.git): + directory = osp.join(parent_repo.git_dir, "modules") + for component in to_native_path_linux(name).split("/")[:-1]: + directory = osp.join(directory, component) + if is_git_dir(directory) and ( + moving_from is None or Path(directory).resolve() != Path(moving_from).resolve() + ): + raise ValueError( + "Submodule metadata for %r is inside another Git directory: %r" % (name, directory) + ) return osp.join(parent_repo.git_dir, "modules", name) if parent_repo.working_tree_dir: - return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path)) + return cls._checked_abspath( + parent_repo.working_tree_dir, + cls._to_relative_path(parent_repo, path), + git_dirs=(parent_repo.git_dir, parent_repo.common_dir), + ) raise NotADirectoryError() @classmethod @@ -360,7 +397,9 @@ def _clone_repo( path = cls._to_relative_path(repo, path) if repo.working_tree_dir is None: raise NotADirectoryError("Submodules require a working tree") - module_checkout_path = cls._checked_abspath(repo.working_tree_dir, path) + module_checkout_path = cls._checked_abspath( + repo.working_tree_dir, path, git_dirs=(repo.git_dir, repo.common_dir) + ) module_abspath = cls._module_abspath(repo, path, name) if cls._need_gitfile_submodules(repo.git): if not allow_unsafe_options: @@ -402,6 +441,16 @@ def _clone_repo( **kwargs, ) if cls._need_gitfile_submodules(repo.git): + # A concurrent clone may have turned a leading directory into a repository. + try: + cls._module_abspath(repo, path, name) + except ValueError: + clone.close() + try: + os.remove(osp.join(clone.git_dir, "HEAD")) + except FileNotFoundError: + pass + raise cls._write_git_file_and_module_config(module_checkout_path, module_abspath) return clone @@ -411,8 +460,9 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike: """:return: A path guaranteed to be relative to the given parent repository :raise ValueError: - If path is not contained in the parent repository's working tree. + If path is outside the working tree or is unsafe as a submodule checkout. """ + cls._validate_windows_path(path) if parent_repo.working_tree_dir: path = _to_relative_path(parent_repo.working_tree_dir, path) else: @@ -422,6 +472,7 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike: if not path or path == ".": raise ValueError("Submodule checkout path must not be the repository root") + _validate_repo_path(path) return path @property @@ -433,22 +484,35 @@ def abspath(self) -> PathLike: def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike: """Check a checkout path already normalized by :meth:`_to_relative_path`.""" - return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink) + return self._checked_abspath( + self.repo.working_tree_dir, + relative_path, + allow_final_symlink, + git_dirs=(self.repo.git_dir, self.repo.common_dir), + ) @classmethod def _checked_abspath( - cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False + cls, + root: Union[PathLike, None], + relative_path: PathLike, + allow_final_symlink: bool = False, + *, + git_dirs: Sequence[PathLike] = (), ) -> str: - """Reject symlinks below a trusted root before accessing submodule paths.""" + """Reject symlinks and checkout aliases of Git directories below a trusted root.""" if root is None: raise NotADirectoryError("Submodules require a working tree") path = os.fspath(root) + metadata_dirs = set(git_dirs) components = to_native_path_linux(relative_path).split("/") for index, component in enumerate(components): path = os.fspath(join_path_native(path, component)) - if allow_final_symlink and index == len(components) - 1: - break + if metadata_dirs and osp.exists(path) and any(osp.samefile(path, directory) for directory in metadata_dirs): + raise ValueError("Submodule checkout path aliases Git metadata: %r" % relative_path) if osp.islink(path): + if allow_final_symlink and index == len(components) - 1: + break raise ValueError("Submodule path %r contains a symbolic link" % relative_path) return path @@ -1122,6 +1186,11 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # Validate the source before removing the destination. cur_path = self.abspath + module_abspath = self._module_abspath(self.repo, self.path, self.name) + if self.path == self.name: + self._module_abspath( + self.repo, module_checkout_path, os.fspath(module_checkout_path), moving_from=module_abspath + ) module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True) if osp.isfile(module_checkout_abspath): raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath) @@ -1160,7 +1229,6 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool = renamed_module = True if osp.isfile(osp.join(module_checkout_abspath, ".git")): - module_abspath = self._module_abspath(self.repo, self.path, self.name) self._write_git_file_and_module_config(module_checkout_abspath, module_abspath) # END handle git file rewrite # END move physical module @@ -1522,8 +1590,8 @@ def rename(self, new_name: str) -> "Submodule": self._validated_name(self.name) self._validated_name(new_name) - destination_module_abspath = self._module_abspath(self.repo, self.path, new_name) mod = self.module() + destination_module_abspath = self._module_abspath(self.repo, self.path, new_name, moving_from=mod.git_dir) self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file) # .git/config @@ -1573,6 +1641,7 @@ def module(self) -> "Repo": """ self._validated_name(self.name) module_checkout_abspath = self.abspath + self._module_abspath(self.repo, self.path, self.name) try: repo = git.Repo(module_checkout_abspath) if repo != self.repo: diff --git a/test/test_submodule.py b/test/test_submodule.py index b1f4f5156..45b285c55 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -86,6 +86,7 @@ def test_submodule_update_preserves_literal_name(tmp_path, monkeypatch, caplog, def movable_submodule(tmp_path): """Create a committed local submodule whose logical name stays fixed when moved.""" with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: + source.git.symbolic_ref("HEAD", "refs/heads/master") (tmp_path / "source" / "file").write_text("content", encoding="utf-8") source.index.add(["file"]) source.index.commit("Create source") @@ -111,6 +112,207 @@ def _move_snapshot(submodule): ) +@pytest.mark.parametrize( + "path", + [ + ".git/child", + ".GiT/child", + "nested/.git/child", + "git~1/child", + "GIT~1 . /child", + ".git. /child", + ".git:stream/child", + ".git::$INDEX_ALLOCATION/child", + "nested\\.git\\child", + "C:relative", + "nul\0name", + ] + + [ + f".g{chr(codepoint)}it/child" + for codepoint in (*range(0x200C, 0x2010), *range(0x202A, 0x202F), *range(0x206A, 0x2070), 0xFEFF) + ], +) +@pytest.mark.parametrize("operation", ["add", "clone", "update", "move", "move-module", "move-config"]) +def test_submodule_rejects_unsafe_checkout_before_mutation(movable_submodule, tmp_path, path, operation): + sm = movable_submodule + root = Path(sm.repo.working_tree_dir) + before = _move_snapshot(sm) + paths = set(root.rglob("*")) + # Check the portable metadata aliases against Git's index validation as well. + if operation == "clone" and path not in ("C:relative", "nul\0name"): + with sm.repo.git.custom_environment(GIT_INDEX_FILE=str(tmp_path / "validation-index")): + with _patch_git_config("core.protectHFS", "true"), _patch_git_config("core.protectNTFS", "true"): + with pytest.raises(GitCommandError): + sm.repo.git.update_index("--add", "--cacheinfo", f"160000,{sm.hexsha},{path}") + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("clone attempted")): + with pytest.raises(ValueError): + if operation == "add": + sm.repo.create_submodule("new", path, sm.url) + elif operation == "clone": + Submodule._clone_repo(sm.repo, sm.url, path, "new") + elif operation == "update": + Submodule(sm.repo, sm.binsha, name="new", path=path, url=sm.url).update(init=True) + else: + sm.move(path, configuration=operation != "move-module", module=operation != "move-config") + assert _move_snapshot(sm) == before + assert set(root.rglob("*")) == paths + + +def test_add_rejects_metadata_checkout_without_writing_files(movable_submodule): + sm = movable_submodule + before = _move_snapshot(sm) + with pytest.raises(ValueError, match="Git metadata"): + sm.repo.create_submodule("new", ".git/new", sm.url) + assert not Path(sm.repo.git_dir, "new").exists() + assert not Path(sm.repo.git_dir, "modules/new").exists() + assert _move_snapshot(sm) == before + + +@pytest.mark.parametrize("absolute_path", [False, True]) +@pytest.mark.parametrize("metadata_name", ["metadata", "MeTaDaTa"]) +@pytest.mark.parametrize("operation", ["add", "clone", "update", "move"]) +def test_submodule_rejects_checkout_in_separate_metadata( + movable_submodule, tmp_path, absolute_path, metadata_name, operation +): + root = tmp_path / "separate" + with git.Repo.init(root, separate_git_dir=str(root / "metadata"), allow_unsafe_options=True) as parent: + if not (root / metadata_name).is_dir(): + pytest.skip("Requires a case-insensitive filesystem") + sm = parent.create_submodule("module", "module", movable_submodule.url) + parent.index.commit("Add submodule") + before = _move_snapshot(sm) + paths = set(root.rglob("*")) + path = root / metadata_name / "new" if absolute_path else f"{metadata_name}/new" + with pytest.raises(ValueError, match="Git metadata"): + if operation == "add": + parent.create_submodule("new", path, sm.url) + elif operation == "clone": + Submodule._clone_repo(parent, sm.url, path, "new") + elif operation == "update": + Submodule(parent, sm.binsha, name="new", path=path, url=sm.url).update(init=True) + else: + sm.move(path) + assert _move_snapshot(sm) == before + assert set(root.rglob("*")) == paths + + +@pytest.mark.parametrize("operation", ["add", "clone", "update", "rename", "move"]) +def test_submodule_rejects_nested_metadata_before_mutation(movable_submodule, operation): + sm = movable_submodule + other = sm.repo.create_submodule("other", "other", sm.url) + other.module().close() + root = Path(sm.repo.working_tree_dir) + before = _move_snapshot(sm), _move_snapshot(other) + paths = set(root.rglob("*")) + name = f"{sm.name}/child" + with pytest.raises(ValueError, match="inside.*Git directory"): + if operation == "add": + sm.repo.create_submodule(name, "new", sm.url) + elif operation == "clone": + Submodule._clone_repo(sm.repo, sm.url, "new", name) + elif operation == "update": + Submodule(sm.repo, sm.binsha, name=name, path="new", url=sm.url).update(init=True) + elif operation == "rename": + other.rename(name) + else: + other.move(name) + assert (_move_snapshot(sm), _move_snapshot(other)) == before + assert set(root.rglob("*")) == paths + + +@pytest.mark.parametrize("state", ["retained", "checked-out"]) +def test_update_rejects_existing_nested_metadata(movable_submodule, state): + sm = movable_submodule + name = f"{sm.name}/child" + checkout = Path(sm.repo.working_tree_dir, "new") + with git.Repo.clone_from( + sm.url, checkout, separate_git_dir=str(Path(sm.repo.git_dir, "modules", name)), allow_unsafe_options=True + ): + pass + if state == "retained": + shutil.rmtree(checkout) + before = _move_snapshot(sm) + paths = set(Path(sm.repo.working_tree_dir).rglob("*")) + with pytest.raises(ValueError, match="inside.*Git directory"): + Submodule(sm.repo, sm.binsha, name=name, path="new", url=sm.url).update(init=True, no_fetch=True) + assert _move_snapshot(sm) == before + assert set(Path(sm.repo.working_tree_dir).rglob("*")) == paths + + +@pytest.mark.parametrize( + "path", + [ + "CON", + "con.txt", + "CONIN$", + "conout$.txt", + "AUX .txt", + "PRN", + "NUL", + "COM9", + "LPT1", + "name:stream", + "space ", + "period.", + "line\nbreak", + "star*", + 'quote"', + "question?", + "angle<", + "angle>", + "pipe|", + ], +) +def test_submodule_rejects_windows_destination_names_before_mutation(movable_submodule, path): + sm = movable_submodule + before = _move_snapshot(sm) + paths = set(Path(sm.repo.working_tree_dir).rglob("*")) + with mock.patch("git.objects.submodule.base.sys", SimpleNamespace(platform="win32")): + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("clone attempted")): + with pytest.raises(ValueError): + sm.repo.create_submodule("new", f"nested/{path}", sm.url) + with pytest.raises(ValueError): + sm.repo.create_submodule(f"nested/{path}", "new", sm.url) + assert _move_snapshot(sm) == before + assert set(Path(sm.repo.working_tree_dir).rglob("*")) == paths + + +@pytest.mark.parametrize("path", ["nested/space ", "nested/period."]) +def test_windows_destination_validation_precedes_normalization(tmp_path, path): + parent = SimpleNamespace(working_tree_dir=str(tmp_path)) + # Windows' GetFullPathName removes trailing spaces and periods. + with mock.patch("git.objects.submodule.base._to_relative_path", return_value=path.rstrip(" .")): + with mock.patch("git.objects.submodule.base.sys", SimpleNamespace(platform="win32")): + with pytest.raises(ValueError, match="Invalid submodule path on Windows"): + Submodule._to_relative_path(parent, path) + + +def test_submodule_can_relocate_its_own_metadata(movable_submodule): + sm = movable_submodule + sm.rename(f"{sm.name}/child") + assert Path(sm.abspath, "file").read_text() == "content" + with sm.module() as module: + assert Path(module.git_dir) == Path(sm.repo.git_dir, "modules", sm.name) + + +def test_clone_disables_metadata_that_becomes_nested(movable_submodule, monkeypatch): + sm = movable_submodule + clone_from = git.Repo.clone_from + ancestor = Path(sm.repo.git_dir, "modules/new") + + def clone_and_create_ancestor(*args, **kwargs): + clone = clone_from(*args, **kwargs) + with git.Repo.init(ancestor, bare=True): + pass + return clone + + monkeypatch.setattr(git.Repo, "clone_from", clone_and_create_ancestor) + with pytest.raises(ValueError, match="inside.*Git directory"): + Submodule._clone_repo(sm.repo, sm.url, "new", "new/child") + assert not (ancestor / "child/HEAD").exists() + assert (ancestor / "HEAD").is_file() + + @pytest.mark.parametrize("target_kind", ["relative", "absolute", "internal", "dangling"]) @pytest.mark.parametrize("configuration,module", [(True, True), (False, True), (True, False)]) @pytest.mark.parametrize("absolute_path", [False, True]) @@ -167,6 +369,28 @@ def test_move_normal_destination(movable_submodule, absolute_path): assert _move_snapshot(submodule) == before +@pytest.mark.parametrize("metadata_dir", ["git_dir", "common_dir"]) +def test_move_rejects_leaf_symlink_to_metadata(movable_submodule, tmp_path, metadata_dir): + root = tmp_path / "worktree" + movable_submodule.repo.git.worktree("add", "--detach", str(root)) + with git.Repo(root) as parent: + assert not osp.samefile(parent.git_dir, parent.common_dir) + submodule = parent.submodules[0] + submodule.update(init=True) + target = Path(getattr(parent, metadata_dir)) + destination = root / "destination" + destination.symlink_to(target, target_is_directory=True) + before = _move_snapshot(submodule) + + with pytest.raises(ValueError, match="Git metadata"): + submodule.move("destination", module=False) + + assert _move_snapshot(submodule) == before + assert destination.is_symlink() + assert destination.samefile(target) + assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content" + + @pytest.mark.parametrize("kind", ["empty", "nonempty", "file", "dangling"]) def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind): """Preserve leaf-symlink replacement without modifying the external target. @@ -1410,6 +1634,7 @@ def test_update_rejects_parent_component_in_name(self, rwdir): invalid_names = ( "", + "nul\0name", "..", "../module", R"..\module",