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",