Skip to content

Commit bae35f9

Browse files
codexByron
authored andcommitted
Guard read-tree index output paths
GHSA-4gmw-gg2m-w46p reports that caller-controlled treeish arguments could be parsed by git read-tree as --index-output and select an arbitrary output path. A regression test showed that from_tree reached Git instead of raising UnsafeOptionError; the same unchecked path was reachable through reset and both merge_tree treeish positions. Add the project-standard unsafe-option guard and explicit opt-out to from_tree, merge_tree, and reset. Check positional and keyword candidates so abbreviations and alternate forwarding forms are covered before read-tree runs. A broader audit found only two read-tree sinks in the codebase; both are now guarded, and reset delegates to the guarded from_tree path. The only remaining index-output use is GitPython's controlled temporary index. Git cf5497b14 confirms read-tree parses this path-taking option before tree arguments. Focused index tests and Ruff checks pass.
1 parent 86870cd commit bae35f9

2 files changed

Lines changed: 53 additions & 3 deletions

File tree

git/index/base.py

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
131131
"""
132132

133133
unsafe_git_checkout_index_options = ["--prefix"]
134+
unsafe_git_read_tree_options = ["--index-output"]
134135

135136
__slots__ = ("repo", "version", "entries", "_extension_data", "_file_path")
136137

@@ -256,7 +257,12 @@ def write(
256257

257258
@post_clear_cache
258259
@default_index
259-
def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexFile":
260+
def merge_tree(
261+
self,
262+
rhs: Treeish,
263+
base: Union[None, Treeish] = None,
264+
allow_unsafe_options: bool = False,
265+
) -> "IndexFile":
260266
"""Merge the given `rhs` treeish into the current index, possibly taking
261267
a common base treeish into account.
262268
@@ -270,6 +276,9 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
270276
Optional treeish reference pointing to the common base of `rhs` and this
271277
index which equals lhs.
272278
279+
:param allow_unsafe_options:
280+
Allow options that may write to arbitrary paths.
281+
273282
:return:
274283
self (containing the merge and possibly unmerged entries in case of
275284
conflicts)
@@ -280,6 +289,12 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
280289
yourself, you have to commit the changed index (or make a valid tree from
281290
it) and retry with a three-way :meth:`index.from_tree <from_tree>` call.
282291
"""
292+
if not allow_unsafe_options:
293+
Git.check_unsafe_options(
294+
options=Git._option_candidates([base, rhs]),
295+
unsafe_options=self.unsafe_git_read_tree_options,
296+
)
297+
283298
# -i : ignore working tree status
284299
# --aggressive : handle more merge cases
285300
# -m : do an actual merge
@@ -324,7 +339,13 @@ def new(cls, repo: "Repo", *tree_sha: Union[str, Tree]) -> "IndexFile":
324339
return inst
325340

326341
@classmethod
327-
def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile":
342+
def from_tree(
343+
cls,
344+
repo: "Repo",
345+
*treeish: Treeish,
346+
allow_unsafe_options: bool = False,
347+
**kwargs: Any,
348+
) -> "IndexFile":
328349
R"""Merge the given treeish revisions into a new index which is returned.
329350
The original index will remain unaltered.
330351
@@ -348,6 +369,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
348369
:param kwargs:
349370
Additional arguments passed to :manpage:`git-read-tree(1)`.
350371
372+
:param allow_unsafe_options:
373+
Allow options that may write to arbitrary paths.
374+
351375
:return:
352376
New :class:`IndexFile` instance. It will point to a temporary index location
353377
which does not exist anymore. If you intend to write such a merged Index,
@@ -365,6 +389,12 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
365389
if len(treeish) == 0 or len(treeish) > 3:
366390
raise ValueError("Please specify between 1 and 3 treeish, got %i" % len(treeish))
367391

392+
if not allow_unsafe_options:
393+
Git.check_unsafe_options(
394+
options=Git._option_candidates(treeish, kwargs),
395+
unsafe_options=cls.unsafe_git_read_tree_options,
396+
)
397+
368398
arg_list: List[Union[Treeish, str]] = []
369399
# Ignore that the working tree and index possibly are out of date.
370400
if len(treeish) > 1:
@@ -1414,6 +1444,7 @@ def reset(
14141444
working_tree: bool = False,
14151445
paths: Union[None, Iterable[PathLike]] = None,
14161446
head: bool = False,
1447+
allow_unsafe_options: bool = False,
14171448
**kwargs: Any,
14181449
) -> "IndexFile":
14191450
"""Reset the index to reflect the tree at the given commit. This will not adjust
@@ -1445,6 +1476,9 @@ def reset(
14451476
The paths need to exist at the commit, otherwise an exception will be
14461477
raised.
14471478
1479+
:param allow_unsafe_options:
1480+
Allow options that may write to arbitrary paths.
1481+
14481482
:param kwargs:
14491483
Additional keyword arguments passed to :manpage:`git-reset(1)`.
14501484
@@ -1461,7 +1495,7 @@ def reset(
14611495
"""
14621496
# What we actually want to do is to merge the tree into our existing index,
14631497
# which is what git-read-tree does.
1464-
new_inst = type(self).from_tree(self.repo, commit)
1498+
new_inst = type(self).from_tree(self.repo, commit, allow_unsafe_options=allow_unsafe_options)
14651499
if not paths:
14661500
self.entries = new_inst.entries
14671501
else:

test/test_index.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,22 @@ def add_bad_blob():
327327
except Exception as ex:
328328
assert "index.lock' could not be obtained" not in str(ex)
329329

330+
@with_rw_repo("0.1.6")
331+
def test_read_tree_methods_reject_index_output(self, rw_repo):
332+
output_path = (Path(rw_repo.working_tree_dir) / "alternate-index").as_posix()
333+
unsafe_option = f"--index-output={output_path}"
334+
335+
with pytest.raises(UnsafeOptionError):
336+
IndexFile.from_tree(rw_repo, unsafe_option)
337+
with pytest.raises(UnsafeOptionError):
338+
IndexFile.from_tree(rw_repo, "HEAD", index_output=output_path)
339+
with pytest.raises(UnsafeOptionError):
340+
rw_repo.index.reset(unsafe_option)
341+
with pytest.raises(UnsafeOptionError):
342+
rw_repo.index.merge_tree(unsafe_option)
343+
with pytest.raises(UnsafeOptionError):
344+
rw_repo.index.merge_tree("HEAD", base=unsafe_option)
345+
330346
@with_rw_repo("0.1.6")
331347
def test_index_file_from_tree(self, rw_repo):
332348
common_ancestor_sha = "5117c9c8a4d3af19a9958677e45cda9269de1541"

0 commit comments

Comments
 (0)