diff --git a/git/index/base.py b/git/index/base.py index 4ecd0c5b1..3d32d7d81 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -728,6 +728,8 @@ def _preprocess_add_items( else: raise TypeError("Invalid Type: %r" % item) # END for each item + # Source paths must be safe to read, but their recorded names may be rewritten. + # Apply mode-dependent restrictions to the final entries in add(). for entry in entries: _validate_repo_path(entry.path) return paths, entries @@ -1026,7 +1028,7 @@ def handle_null_entries(self: "IndexFile") -> None: # FINALIZE # Add the new entries to this instance. for entry in entries_added: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) for entry in entries_added: self.entries[(entry.path, 0)] = IndexEntry.from_base(entry) diff --git a/git/index/fun.py b/git/index/fun.py index 929038f1f..2491f55ae 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -279,7 +279,7 @@ def write_cache( # Body for entry in entries: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) beginoffset = tell() write(entry.ctime_bytes) # ctime write(entry.mtime_bytes) # mtime @@ -394,7 +394,7 @@ def read_cache( if terminator != b"\0": raise ValueError("Unterminated index entry path") path = path_bytes.decode(defenc) - _validate_repo_path(path) + _validate_repo_path(path, mode) real_size = (tell() - beginoffset + 7) & ~7 padding_size = beginoffset + real_size - tell() @@ -462,7 +462,7 @@ def write_tree_from_cache( """ if si == 0: for entry in entries[sl]: - _validate_repo_path(entry.path) + _validate_repo_path(entry.path, entry.mode) tree_items: List["TreeCacheTup"] = [] ci = sl.start @@ -510,7 +510,7 @@ def write_tree_from_cache( def _tree_entry_to_baseindexentry(tree_entry: "TreeCacheTup", stage: int) -> BaseIndexEntry: - _validate_repo_path(tree_entry[2]) + _validate_repo_path(tree_entry[2], tree_entry[1]) return BaseIndexEntry((tree_entry[1], tree_entry[0], stage << CE_STAGESHIFT, tree_entry[2])) diff --git a/git/objects/fun.py b/git/objects/fun.py index ff1bc8483..41ddecafd 100644 --- a/git/objects/fun.py +++ b/git/objects/fun.py @@ -39,11 +39,11 @@ # --------------------------------------------------- -def _validate_tree_entry_name(name: str) -> None: +def _validate_tree_entry_name(name: str, mode: Union[int, None] = None) -> None: if "/" in name: raise ValueError("Tree entry names must not contain '/' characters") # A tree name is a component, not a rooted path; a colon cannot select a drive. - _validate_repo_path("tree/" + name) + _validate_repo_path("tree/" + name, mode) def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer"], Union[int, None]]) -> None: @@ -82,7 +82,7 @@ def tree_to_stream(entries: Sequence[EntryTup], write: Callable[["ReadableBuffer name_bytes = name.encode(defenc) else: name_bytes = name # type: ignore[unreachable] # check runtime types - is always str? - _validate_tree_entry_name(safe_decode(name_bytes)) + _validate_tree_entry_name(safe_decode(name_bytes), mode) write(b"".join((mode_str, b" ", name_bytes, b"\0", binsha))) # END for each item @@ -112,7 +112,7 @@ def tree_entries_from_data(data: bytes) -> List[EntryTup]: if name_end < 0 or name_end + 21 > len(data): raise ValueError("Truncated tree entry") name = safe_decode(bytes(data[mode_end + 1 : name_end])) - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) offset = name_end + 21 out.append((bytes(data[name_end + 1 : offset]), mode, name)) return out diff --git a/git/objects/tree.py b/git/objects/tree.py index d2a5859df..96e263530 100644 --- a/git/objects/tree.py +++ b/git/objects/tree.py @@ -110,7 +110,7 @@ def add(self, sha: bytes, mode: int, name: str, force: bool = False) -> "TreeMod :return: self """ - _validate_tree_entry_name(name) + _validate_tree_entry_name(name, mode) if (mode >> 12) not in Tree._map_id_to_type: raise ValueError("Invalid object type according to mode %o" % mode) diff --git a/git/util.py b/git/util.py index 75677b7d3..61ddab861 100644 --- a/git/util.py +++ b/git/util.py @@ -29,63 +29,62 @@ if sys.platform == "win32": __all__.append("to_native_path_windows") -from abc import abstractmethod import contextlib -from functools import wraps import getpass import logging import ntpath import os import os.path as osp -from pathlib import Path import platform import re import shutil import stat import subprocess import time -from urllib.parse import urlsplit, urlunsplit import warnings - -# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of -# these be used indirectly through other GitPython modules, which avoids having to write -# gitdb all the time in their imports. They are not in __all__, at least currently, -# because they could be removed or changed at any time, and so should not be considered -# conceptually public to code outside GitPython. Linters of course do not like it. -from gitdb.util import ( - LazyMixin, # noqa: F401 - LockedFD, # noqa: F401 - bin_to_hex, # noqa: F401 - file_contents_ro, # noqa: F401 - file_contents_ro_filepath, # noqa: F401 - hex_to_bin, # noqa: F401 - make_sha, - to_bin_sha, # noqa: F401 - to_hex_sha, # noqa: F401 -) +from abc import abstractmethod +from functools import wraps +from pathlib import Path # typing --------------------------------------------------------- - from typing import ( + IO, + TYPE_CHECKING, Any, AnyStr, Callable, Dict, Generator, - IO, Iterator, List, Optional, Pattern, Sequence, Tuple, - TYPE_CHECKING, Type, TypeVar, Union, cast, overload, ) +from urllib.parse import urlsplit, urlunsplit + +# NOTE: Unused imports can be improved now that CI testing has fully resumed. Some of +# these be used indirectly through other GitPython modules, which avoids having to write +# gitdb all the time in their imports. They are not in __all__, at least currently, +# because they could be removed or changed at any time, and so should not be considered +# conceptually public to code outside GitPython. Linters of course do not like it. +from gitdb.util import ( + LazyMixin, # noqa: F401 + LockedFD, # noqa: F401 + bin_to_hex, # noqa: F401 + file_contents_ro, # noqa: F401 + file_contents_ro_filepath, # noqa: F401 + hex_to_bin, # noqa: F401 + make_sha, + to_bin_sha, # noqa: F401 + to_hex_sha, # noqa: F401 +) if TYPE_CHECKING: from git.cmd import Git @@ -94,9 +93,9 @@ from git.repo.base import Repo from git.types import ( + HSH_TD, Files_TD, Has_id_attribute, - HSH_TD, Literal, PathLike, Protocol, @@ -385,12 +384,27 @@ def _to_relative_path(root: PathLike, path: PathLike) -> str: "", "", "\u200c\u200d\u200e\u200f\u202a\u202b\u202c\u202d\u202e\u206a\u206b\u206c\u206d\u206e\u206f\ufeff" ) +# Match Git's is_ntfs_dotgitmodules in path.c on a lowercased, trimmed name. +# Besides gitmod~1..4, fallback aliases have exactly eight ASCII characters: +# a shrinking prefix of "gi7eba", "~", and digits with no leading zero. +# Explicit digit counts avoid accepting shorter/longer names or Unicode digits. +_NTFS_DOTGITMODULES_SHORT_NAME = re.compile( + r"(?:gitmod~[1-4]|gi7eba~[1-9]|gi7eb~[1-9][0-9]|gi7e~[1-9][0-9]{2}|" + r"gi7~[1-9][0-9]{3}|gi~[1-9][0-9]{4}|g~[1-9][0-9]{5}|~[1-9][0-9]{6})" +) + -def _validate_repo_path(path: PathLike) -> None: +def _validate_repo_path(path: PathLike, mode: Union[int, None] = None) -> None: """Reject unsafe tree/index paths without normalizing away their components. Protect Git metadata aliases on NTFS and HFS even when writing on another platform. Other POSIX filename characters, including newlines, remain valid. + + :param mode: + Mode of the index or tree entry the path belongs to, where one is known. + Git refuses a symbolic link that aliases ``.gitmodules``, since the + submodule configuration would then be read through the link, so that + name is only rejected once the mode says the entry is a link. """ name = os.fspath(path) if not name or "\0" in name or ntpath.splitdrive(name)[0] or name.startswith("/"): @@ -406,6 +420,13 @@ def _validate_repo_path(path: PathLike) -> None: hfs_name = part.translate(_HFS_IGNORABLES).lower() if ntfs_name in (".git", "git~1") or hfs_name == ".git": raise ValueError("Repository path aliases Git metadata: %r" % name) + if mode is not None and stat.S_ISLNK(mode): + if ( + ntfs_name == ".gitmodules" + or hfs_name == ".gitmodules" + or _NTFS_DOTGITMODULES_SHORT_NAME.fullmatch(ntfs_name) is not None + ): + raise ValueError("Symbolic link aliases the submodule configuration: %r" % name) def assure_directory_exists(path: PathLike, is_file: bool = False) -> bool: diff --git a/test/test_index.py b/test/test_index.py index 30cb54565..150b39466 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -189,10 +189,10 @@ def _make_hook(git_dir, name, content, make_exec=True): return hp -def _raw_index(path): +def _raw_index(path, mode=0o100644): """Build an index without using the writer under test.""" name = path.encode("utf-8") - entry = struct.pack(">10L20sH", 0, 0, 0, 0, 0, 0, 0o100644, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name + entry = struct.pack(">10L20sH", 0, 0, 0, 0, 0, 0, mode, 0, 0, 0, b"a" * 20, min(len(name), 0xFFF)) + name entry += b"\0" * (8 - len(entry) % 8) data = b"DIRC" + struct.pack(">LL", 2, 1) + entry return data + sha1(data).digest() @@ -340,6 +340,68 @@ def test_index_reader_and_writer_reject_unsafe_paths(self, path): with pytest.raises(ValueError): write_cache([entry], BytesIO()) + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules.", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~4", + "gi7eba~1", + "gi7eba~9", + "GI7EB~10", + "GI7EB~99", + "GI7E~100", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "GI7EB~10. ", + "GI7E~100:$DATA", + "sub/~1000000", + "sub/.gitmodules", + ) + def test_index_reader_and_writer_reject_gitmodules_symlinks(self, path): + """An entry that turns .gitmodules into a symbolic link is rejected, while the + same name stays valid for a regular file. The spellings are those + `git update-index --add --cacheinfo 120000,,` refuses on git 2.52.0.""" + with pytest.raises(ValueError): + read_cache(BytesIO(_raw_index(path, mode=0o120000))) + with pytest.raises(ValueError): + write_cache([IndexEntry((0o120000, b"a" * 20, 0, path))], BytesIO()) + + assert next(iter(read_cache(BytesIO(_raw_index(path)))[1])) == (path, 0) + stream = BytesIO() + write_cache([IndexEntry((0o100644, b"a" * 20, 0, path))], stream) + stream.seek(0) + assert next(iter(read_cache(stream)[1])) == (path, 0) + + @ddt.data("GI7EB~10", "GI7E~100", "GI7~1000", "GI~10000", "G~100000", "~1000000", "~9999999") + @with_rw_directory + def test_index_add_and_write_tree_reject_gitmodules_fallback_symlinks(self, rw_dir, path): + with Repo.init(rw_dir) as repo: + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + index = repo.index + for item in (Blob(repo, binsha, 0o120000, path), BaseIndexEntry((0o120000, binsha, 0, path))): + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], write=False) + assert not index.entries + + index.entries[(path, 0)] = IndexEntry((0o120000, binsha, 0, path)) + with pytest.raises(ValueError, match="submodule configuration"): + index.write_tree() + assert not Path(index.path).exists() + + index.add([BaseIndexEntry((0o100644, binsha, 0, path))]) + assert repo.index.entries[(path, 0)].mode == 0o100644 + assert index.write_tree()[path].mode == 0o100644 + def test_valid_unusual_index_names_round_trip(self): names = ["a b", "a\nb", "a\tb", "name:value", "dir/.gitignore", "café"] if os.name != "nt": @@ -381,20 +443,26 @@ def _cmp_tree_index(self, tree, index): @with_rw_repo("0.1.6") def test_index_lock_handling(self, rw_repo): - def add_bad_blob(): - rw_repo.index.add([Blob(rw_repo, b"f" * 20, "bad-permissions", "foo")]) - - try: - ## First, fail on purpose adding into index. - add_bad_blob() - except Exception as ex: - assert "required argument is not an integer" in str(ex) - - ## The second time should not fail due to stray lock file. - try: - add_bad_blob() - except Exception as ex: - assert "index.lock' could not be obtained" not in str(ex) + index = rw_repo.index + index_path = Path(index.path) + lock_path = Path(str(index_path) + ".lock") + before = index_path.read_bytes() + + def fail_serialize(stream, ignore_extension_data): + assert lock_path.exists() + stream.write(b"partial index") + raise OSError("simulated index write failure") + + with mock.patch.object(IndexFile, "_serialize", side_effect=fail_serialize): + for _ in range(2): + with pytest.raises(OSError, match="simulated index write failure"): + index.write() + assert not lock_path.exists() + assert index_path.read_bytes() == before + + index.add([Blob(rw_repo, b"f" * 20, 0o100644, "foo")]) + assert not lock_path.exists() + assert rw_repo.index.entries[("foo", 0)].mode == 0o100644 @with_rw_repo("0.1.6") def test_read_tree_methods_reject_index_output(self, rw_repo): @@ -1221,6 +1289,60 @@ def test_staging_rejects_unsafe_object_paths_even_without_writing(self, rw_dir, index.add([item], write=False, **kwargs) assert not index.entries + @ddt.data(*product(("path", "blob", "entry", "stored-blob", "stored-entry"), (False, True), (False, True))) + @ddt.unpack + @with_rw_directory + def test_staging_gitmodules_symlink_check_uses_rewritten_path(self, rw_dir, kind, unsafe_destination, write): + with Repo.init(rw_dir) as repo: + source, destination = ("safe-link", ".gitmodules") if unsafe_destination else (".gitmodules", "safe-link") + binsha = Blob.NULL_BIN_SHA + if kind.startswith("stored-"): + binsha = repo.odb.store(IStream("blob", 6, BytesIO(b"target"))).binsha + else: + try: + (Path(rw_dir) / source).symlink_to("target") + except OSError: + pytest.skip("Symlinks unavailable") + if kind == "path": + item = source + elif kind.endswith("blob"): + item = Blob(repo, binsha, 0o120000, source) + else: + item = BaseIndexEntry((0o120000, binsha, 0, source)) + index = repo.index + rewriter = mock.Mock(return_value=destination) + if unsafe_destination: + with pytest.raises(ValueError, match="submodule configuration"): + index.add([item], path_rewriter=rewriter, write=write) + assert not index.entries + assert not Path(index.path).exists() + else: + added = index.add([item], path_rewriter=rewriter, write=write) + assert [(entry.path, entry.mode) for entry in added] == [(destination, 0o120000)] + assert set(index.entries) == {(destination, 0)} + assert index.write_tree()[destination].mode == 0o120000 + if write: + assert repo.index.entries[(destination, 0)].mode == 0o120000 + rewriter.assert_called_once() + assert rewriter.call_args[0][0].path == source + + @ddt.data(*product(("blob", "entry"), ("../outside", ".git/config"))) + @ddt.unpack + @with_rw_directory + def test_staging_rewriter_cannot_sanitize_unsafe_source_paths(self, rw_dir, kind, path): + with Repo.init(rw_dir) as repo: + item = ( + Blob(repo, b"a" * 20, 0o120000, path) + if kind == "blob" + else BaseIndexEntry((0o120000, b"a" * 20, 0, path)) + ) + index = repo.index + rewriter = mock.Mock(return_value="safe-link") + with pytest.raises(ValueError): + index.add([item], path_rewriter=rewriter, write=False) + rewriter.assert_not_called() + assert not index.entries + @with_rw_directory def test_staging_root_preserves_symlinks_and_skips_git_metadata(self, rw_dir): tmp_path = Path(rw_dir) diff --git a/test/test_tree.py b/test/test_tree.py index 803f1f381..3a85405bd 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -138,6 +138,113 @@ def test_tree_names_are_checked_at_construction_and_serialization(self, name): with pytest.raises(ValueError): tree_to_stream([(b"a" * 20, 0o100644, name)], BytesIO().write) + @ddt.data( + ".gitmodules", + ".GITMODULES", + ".gitmodules ", + ".gi\u200ctmodules", + "gitmod~1", + "gitmod~2", + "gitmod~3", + "GITMOD~4", + "gi7eba~1", + "GI7EBA~9", + "GI7EB~10", + "GI7EB~11", + "GI7EB~99", + "GI7E~100", + "GI7E~101", + "GI7E~999", + "GI7~1000", + "GI7~9999", + "GI~10000", + "GI~99999", + "G~100000", + "G~999999", + "~1000000", + "~9999999", + "Gi7Eb~42", + "gi7e~120", + "GITMOD~4 . ", + "GI7EB~10. ", + "GI7E~100:$DATA", + "~1000000 . :$DATA", + ) + def test_gitmodules_symlink_entries_are_rejected(self, name): + """A symbolic link named like the submodule configuration would make Git read + it from outside the repository, so such an entry is refused in both + directions. A regular file with the same name is the normal case.""" + symlink_mode = 0o120000 + cache = [] + with pytest.raises(ValueError): + TreeModifier(cache).add(b"a" * 20, symlink_mode, name) + assert not cache + with pytest.raises(ValueError): + tree_to_stream([(b"a" * 20, symlink_mode, name)], BytesIO().write) + raw = b"120000 " + name.encode() + b"\0" + b"a" * 20 + with pytest.raises(ValueError): + tree_entries_from_data(raw) + + TreeModifier(cache).add(b"a" * 20, 0o100644, name) + assert cache == [(b"a" * 20, 0o100644, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + assert tree_entries_from_data(data.getvalue()) == cache + + @ddt.data( + "gitmod~0", + "gitmod~5", + "gitmod~10", + "GI7EBA~", + "GI7EBA~0", + "GI7EBA~~1", + "GI7EBA~X", + "GI7EBA~10", + "Gx7EBA~1", + "GI7EBX~1", + "GI7EB~1", + "GI7EB~01", + "GI7EB~1X", + "GI7EB~100", + "GI7E~10", + "GI7E~010", + "GI7E~1000", + "GI7~100", + "GI7~0100", + "GI7~10000", + "GI~1000", + "GI~01000", + "GI~100000", + "G~10000", + "G~010000", + "G~1000000", + "~100000", + "~0100000", + "~10000000", + "GI7EBA~\u0661", + "GI7EB~1\uff10", + "GI7EB~10x", + "GI7EB~10.x", + " GI7EB~10", + "GI7EB~10\n", + "GI7EB~10\t", + "GI7EB~10x:$DATA", + ".gitmodules x", + ".gitmodules .x", + ".gitmodules,:$DATA", + ) + def test_gitmodules_short_name_near_misses_round_trip(self, name): + """Only exact aliases are forbidden, and only for symbolic links.""" + for mode in (0o100644, 0o120000): + cache = [] + TreeModifier(cache).add(b"a" * 20, mode, name) + assert cache == [(b"a" * 20, mode, name)] + data = BytesIO() + tree_to_stream(cache, data.write) + raw = ("%o " % mode).encode() + name.encode() + b"\0" + b"a" * 20 + assert data.getvalue() == raw + assert tree_entries_from_data(raw) == cache + def test_traverse(self): root = self.rorepo.tree("0.1.6") num_recursive = 0