Skip to content

Commit 7cd2cd4

Browse files
codexByron
authored andcommitted
fix(index): confine staging reads to worktree files
`IndexFile.add()` could read through intermediate directory symlinks and store files outside the working tree. Null-SHA `Blob` and `BaseIndexEntry` inputs reached the same reader, while unsafe entry and rewritten paths could enter the in-memory index with `write=False` (finding 21501). Validate supplied object paths and all final entry paths with the shared repository path checker. Confine filesystem inputs in `_store_path()` and reject intermediate symlinks or redirected directories before opening files. Preserve final symlinks as links, including when rewriting paths, and reject special files. Use `O_NOFOLLOW` and `O_NONBLOCK` where available and check the opened regular file with `fstat()`. Skip Git metadata during directory expansion and stage directory symlinks themselves. Validate complete relative directory paths so nested POSIX colon names are not mistaken for drive-prefixed paths. Parent-directory checks handle the existing filesystem layout; they do not make staging atomic against concurrent directory replacement. The open flags protect final-component substitution on supporting platforms. Tests cover path/glob/object inputs, inside and outside symlink targets, rewriters, in-memory index validation, metadata exclusion, directory symlinks, special files, and nested colon names. Validation: 129 combined tree/index tests passed, with two platform skips; Ruff checks and formatting checks passed. Windows CI exposed an existing symlink-storage bug: `lstat().st_size` can be zero for a link, so the object database stored an empty target. Read and encode the link target once and advertise its actual byte length, while retaining `fstat()` sizing for regular files. Compare the native `readlink()` representation in the directory-symlink test rather than assuming separator spelling. A portable regression supplies zero symlink metadata size and a multibyte target; it fails before the fix and passes afterward. All 53 selected staging/index tests pass, with two platform skips; locked Basedpyright, mypy, and all pre-commit hooks pass.
1 parent 309e1db commit 7cd2cd4

2 files changed

Lines changed: 156 additions & 8 deletions

File tree

‎git/index/base.py‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from io import BytesIO
1515
import os
1616
import os.path as osp
17-
from stat import S_ISLNK
17+
from stat import S_ISLNK, S_ISREG
1818
import subprocess
1919
import sys
2020
import tempfile
@@ -36,6 +36,7 @@
3636
file_contents_ro,
3737
_is_path_rooted,
3838
_to_relative_path,
39+
_validate_repo_path,
3940
to_native_path_linux,
4041
unbare_repo,
4142
to_bin_sha,
@@ -476,7 +477,17 @@ def raise_exc(e: Exception) -> NoReturn:
476477
continue
477478
# END glob handling
478479
try:
479-
for root, _dirs, files in os.walk(abs_path, onerror=raise_exc):
480+
for root, dirs, files in os.walk(abs_path, onerror=raise_exc):
481+
for dirname in dirs[:]:
482+
directory = osp.join(root, dirname)
483+
try:
484+
_validate_repo_path(to_native_path_linux(osp.relpath(directory, r)))
485+
except ValueError:
486+
dirs.remove(dirname)
487+
continue
488+
if osp.islink(directory):
489+
dirs.remove(dirname)
490+
yield osp.relpath(directory, r)
480491
for rela_file in files:
481492
# Add relative paths only.
482493
yield osp.join(root.replace(rs, ""), rela_file)
@@ -717,6 +728,8 @@ def _preprocess_add_items(
717728
else:
718729
raise TypeError("Invalid Type: %r" % item)
719730
# END for each item
731+
for entry in entries:
732+
_validate_repo_path(entry.path)
720733
return paths, entries
721734

722735
def _store_path(self, filepath: PathLike, fprogress: Callable) -> BaseIndexEntry:
@@ -726,20 +739,43 @@ def _store_path(self, filepath: PathLike, fprogress: Callable) -> BaseIndexEntry
726739
This needs the :func:`~git.index.util.git_working_dir` decorator active!
727740
This must be ensured in the calling code.
728741
"""
729-
st = os.lstat(filepath) # Handles non-symlinks as well.
730-
742+
filepath = self._to_relative_path(filepath)
743+
_validate_repo_path(filepath)
744+
parent = osp.realpath(self.repo.working_dir)
745+
for component in os.fspath(filepath).split("/")[:-1]:
746+
parent = osp.join(parent, component)
747+
if osp.islink(parent) or osp.normcase(osp.realpath(parent)) != osp.normcase(osp.abspath(parent)):
748+
raise ValueError("Cannot stage a path beyond a symbolic link: %r" % filepath)
749+
st = os.lstat(filepath)
750+
if not S_ISLNK(st.st_mode) and not S_ISREG(st.st_mode):
751+
raise ValueError("Can only stage a regular file or symbolic link: %r" % filepath)
752+
753+
stream_size = st.st_size
731754
if S_ISLNK(st.st_mode):
732755
# readlink is a string, but we need bytes.
756+
target = force_bytes(os.readlink(filepath), encoding=defenc)
757+
stream_size = len(target)
758+
733759
def open_stream() -> BinaryIO:
734-
return BytesIO(force_bytes(os.readlink(filepath), encoding=defenc))
760+
return BytesIO(target)
735761
else:
736762

737763
def open_stream() -> BinaryIO:
738-
return open(filepath, "rb")
764+
# Do not follow a final symlink or block on a FIFO substituted
765+
# between lstat and open on platforms supporting these flags.
766+
def opener(path: str, flags: int) -> int:
767+
return os.open(path, flags | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_NONBLOCK", 0))
768+
769+
return open(filepath, "rb", opener=opener)
739770

740771
with open_stream() as stream:
772+
if not S_ISLNK(st.st_mode):
773+
st = os.fstat(stream.fileno())
774+
if not S_ISREG(st.st_mode):
775+
raise ValueError("Can only stage a regular file: %r" % filepath)
776+
stream_size = st.st_size
741777
fprogress(filepath, False, filepath)
742-
istream = self.repo.odb.store(IStream(Blob.type, st.st_size, stream))
778+
istream = self.repo.odb.store(IStream(Blob.type, stream_size, stream))
743779
fprogress(filepath, True, filepath)
744780
return BaseIndexEntry(
745781
(
@@ -777,7 +813,7 @@ def _entries_for_paths(
777813
blob = Blob(
778814
self.repo,
779815
Blob.NULL_BIN_SHA,
780-
stat_mode_to_index_mode(os.stat(abspath).st_mode),
816+
stat_mode_to_index_mode(os.lstat(abspath).st_mode),
781817
to_native_path_linux(gitrelative_path),
782818
)
783819
# TODO: variable undefined
@@ -989,6 +1025,8 @@ def handle_null_entries(self: "IndexFile") -> None:
9891025

9901026
# FINALIZE
9911027
# Add the new entries to this instance.
1028+
for entry in entries_added:
1029+
_validate_repo_path(entry.path)
9921030
for entry in entries_added:
9931031
self.entries[(entry.path, 0)] = IndexEntry.from_base(entry)
9941032

‎test/test_index_add_security.py‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
# This module is part of GitPython and is released under the
2+
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
3+
4+
import os
5+
from stat import S_ISLNK
6+
from unittest import mock
7+
8+
import pytest
9+
10+
from git import Blob, Repo
11+
from git.index.typ import BaseIndexEntry
12+
13+
14+
@pytest.mark.parametrize("kind", ["path", "glob", "blob", "entry"])
15+
@pytest.mark.parametrize("outside", [False, True])
16+
def test_staging_never_reads_through_a_directory_symlink(tmp_path, kind, outside):
17+
with Repo.init(tmp_path / "repo") as repo:
18+
root = tmp_path / "repo"
19+
target = (tmp_path if outside else root) / "target"
20+
target.mkdir()
21+
(target / "secret").write_text("private data")
22+
try:
23+
(root / "link").symlink_to(target, target_is_directory=True)
24+
except OSError:
25+
pytest.skip("Symlinks unavailable")
26+
item = "link/secret"
27+
if kind == "glob":
28+
item = "link/*"
29+
elif kind == "blob":
30+
item = Blob(repo, Blob.NULL_BIN_SHA, 0o100644, item)
31+
elif kind == "entry":
32+
item = BaseIndexEntry((0o100644, Blob.NULL_BIN_SHA, 0, item))
33+
with mock.patch.object(repo.odb, "store", wraps=repo.odb.store) as store:
34+
with pytest.raises(ValueError, match="symbolic link"):
35+
repo.index.add([item], write=False)
36+
store.assert_not_called()
37+
38+
39+
@pytest.mark.parametrize("kind", ["blob", "entry", "rewriter"])
40+
def test_staging_rejects_unsafe_object_paths_even_without_writing(tmp_path, kind):
41+
with Repo.init(tmp_path) as repo:
42+
entry = BaseIndexEntry((0o100644, b"a" * 20, 0, "../outside"))
43+
item = Blob(repo, entry.binsha, entry.mode, entry.path) if kind == "blob" else entry
44+
kwargs = {}
45+
if kind == "rewriter":
46+
item = BaseIndexEntry((0o100644, b"a" * 20, 0, "safe"))
47+
kwargs["path_rewriter"] = lambda entry: "../outside"
48+
index = repo.index
49+
with pytest.raises(ValueError):
50+
index.add([item], write=False, **kwargs)
51+
assert not index.entries
52+
53+
54+
def test_staging_root_preserves_symlinks_and_skips_git_metadata(tmp_path):
55+
root = tmp_path / "repo"
56+
(tmp_path / "outside").mkdir()
57+
with Repo.init(root) as repo:
58+
(root / "file").write_text("contents")
59+
try:
60+
(root / "link").symlink_to("../outside", target_is_directory=True)
61+
except OSError:
62+
pytest.skip("Symlinks unavailable")
63+
entries = repo.index.add(["."])
64+
assert {entry.path for entry in entries} == {"file", "link"}
65+
link = repo.index.entries[("link", 0)]
66+
assert link.mode == 0o120000
67+
assert repo.odb.stream(link.binsha).read() == os.fsencode(os.readlink(root / "link"))
68+
69+
70+
def test_staging_symlink_measures_encoded_target_instead_of_stat_size(tmp_path):
71+
with Repo.init(tmp_path) as repo:
72+
link = tmp_path / "link"
73+
try:
74+
link.symlink_to("../café", target_is_directory=True)
75+
except OSError:
76+
pytest.skip("Symlinks unavailable")
77+
target = os.fsencode(os.readlink(link))
78+
original_lstat = os.lstat
79+
80+
def zero_size_for_symlinks(*args, **kwargs):
81+
result = original_lstat(*args, **kwargs)
82+
if S_ISLNK(result.st_mode):
83+
fields = list(result)
84+
fields[6] = 0
85+
return os.stat_result(fields)
86+
return result
87+
88+
with mock.patch("os.lstat", side_effect=zero_size_for_symlinks):
89+
(entry,) = repo.index.add(["link"])
90+
assert entry.mode == 0o120000
91+
assert repo.odb.stream(entry.binsha).read() == target
92+
93+
94+
@pytest.mark.skipif(os.name == "nt", reason="Colons are not valid Windows filenames")
95+
def test_staging_nested_colon_directory(tmp_path):
96+
with Repo.init(tmp_path) as repo:
97+
directory = tmp_path / "nested" / "a:b"
98+
directory.mkdir(parents=True)
99+
(directory / "file").write_text("contents")
100+
assert [entry.path for entry in repo.index.add(["nested"])] == ["nested/a:b/file"]
101+
assert repo.index.write_tree()["nested/a:b/file"].data_stream.read() == b"contents"
102+
103+
104+
@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="FIFOs unavailable")
105+
def test_staging_special_files_fails_before_opening(tmp_path):
106+
with Repo.init(tmp_path) as repo:
107+
os.mkfifo(tmp_path / "fifo")
108+
with mock.patch("builtins.open", side_effect=AssertionError("must not open a FIFO")):
109+
with pytest.raises(ValueError, match="regular file"):
110+
repo.index.add(["fifo"], write=False)

0 commit comments

Comments
 (0)