FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Merge pull request #2204 from gitpython-developers/security-fixes · gitpython-developers/GitPython@9a8f6fe · GitHub

/ GitPython Public

Commit 9a8f6fe

Browse files
Byron
authored
Merge pull request #2204 from gitpython-developers/security-fixes
Harden config and Git option validation
2 parents f4277fa + f2550b6 commit 9a8f6fe

10 files changed

Lines changed: 284 additions & 7 deletions

File tree

‎git/cmd.py‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,12 @@ class Git(metaclass=_GitMeta):
654654
"--upload-pack",
655655
]
656656

657+
unsafe_git_pathspec_from_file_options = [
658+
# Reads pathspecs from a caller-controlled file. Some commands include an
659+
# unmatched pathspec in their error output, which can disclose the file.
660+
"--pathspec-from-file",
661+
]
662+
657663
def __getstate__(self) -> Dict[str, Any]:
658664
return slots_to_dict(self, exclude=self._excluded_)
659665

@@ -1044,13 +1050,21 @@ def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[s
10441050
values = value if isinstance(value, (list, tuple)) else (value,)
10451051
if any(value is True or (value is not False and value is not None) for value in values):
10461052
key = str(key)
1047-
options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}")
1048-
if len(key) == 1 and split_single_char_options:
1053+
if len(key) != 1:
1054+
options.append(f"--{dashify(key)}")
1055+
elif split_single_char_options:
1056+
options.append(f"-{key}")
10491057
options.extend(
10501058
str(value)
10511059
for value in values
10521060
if value is not True and value not in (False, None) and str(value).startswith("-")
10531061
)
1062+
else:
1063+
options.extend(
1064+
f"-{key}" if value is True else f"-{key}{value}"
1065+
for value in values
1066+
if value is True or (value is not False and value is not None)
1067+
)
10541068
return options
10551069

10561070
AutoInterrupt: TypeAlias = _AutoInterrupt

‎git/config.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@
7575
UNSAFE_CONFIG_CHARS_RE = re.compile(r"[\r\n\x00]")
7676
"""Characters that cannot be safely written in config names or values."""
7777

78+
VALID_CONFIG_OPTION_NAME_RE = re.compile(r"^[A-Za-z0-9_.-]+$")
79+
"""Pattern for option names that can be written without changing config syntax."""
80+
7881

7982
class MetaParserBuilder(abc.ABCMeta): # noqa: B024
8083
"""Utility class wrapping base-class methods into decorators that assure read-only
@@ -900,6 +903,8 @@ def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> s
900903
def _assure_config_name_safe(self, name: "cp._SectionName", label: str) -> None:
901904
if isinstance(name, str) and UNSAFE_CONFIG_CHARS_RE.search(name):
902905
raise ValueError("Git config %s names must not contain CR, LF, or NUL" % label)
906+
if label == "option" and isinstance(name, str) and not VALID_CONFIG_OPTION_NAME_RE.fullmatch(name):
907+
raise ValueError("Git config option names may contain only letters, digits, '-', '_', or '.'")
903908
if label == "section" and isinstance(name, str):
904909
in_quotes = False
905910
escaped = False

‎git/index/base.py‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
134134
"""
135135

136136
unsafe_git_checkout_index_options = ["--prefix"]
137+
unsafe_git_read_tree_options = ["--index-output"]
137138

138139
__slots__ = ("repo", "version", "entries", "_extension_data", "_file_path")
139140

@@ -259,7 +260,12 @@ def write(
259260

260261
@post_clear_cache
261262
@default_index
262-
def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexFile":
263+
def merge_tree(
264+
self,
265+
rhs: Treeish,
266+
base: Union[None, Treeish] = None,
267+
allow_unsafe_options: bool = False,
268+
) -> "IndexFile":
263269
"""Merge the given `rhs` treeish into the current index, possibly taking
264270
a common base treeish into account.
265271
@@ -273,6 +279,9 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
273279
Optional treeish reference pointing to the common base of `rhs` and this
274280
index which equals lhs.
275281
282+
:param allow_unsafe_options:
283+
Allow options that may write to arbitrary paths.
284+
276285
:return:
277286
self (containing the merge and possibly unmerged entries in case of
278287
conflicts)
@@ -283,6 +292,12 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF
283292
yourself, you have to commit the changed index (or make a valid tree from
284293
it) and retry with a three-way :meth:`index.from_tree <from_tree>` call.
285294
"""
295+
if not allow_unsafe_options:
296+
Git.check_unsafe_options(
297+
options=Git._option_candidates([base, rhs]),
298+
unsafe_options=self.unsafe_git_read_tree_options,
299+
)
300+
286301
# -i : ignore working tree status
287302
# --aggressive : handle more merge cases
288303
# -m : do an actual merge
@@ -327,7 +342,13 @@ def new(cls, repo: "Repo", *tree_sha: Union[str, Tree]) -> "IndexFile":
327342
return inst
328343

329344
@classmethod
330-
def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile":
345+
def from_tree(
346+
cls,
347+
repo: "Repo",
348+
*treeish: Treeish,
349+
allow_unsafe_options: bool = False,
350+
**kwargs: Any,
351+
) -> "IndexFile":
331352
R"""Merge the given treeish revisions into a new index which is returned.
332353
The original index will remain unaltered.
333354
@@ -351,6 +372,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
351372
:param kwargs:
352373
Additional arguments passed to :manpage:`git-read-tree(1)`.
353374
375+
:param allow_unsafe_options:
376+
Allow options that may write to arbitrary paths.
377+
354378
:return:
355379
New :class:`IndexFile` instance. It will point to a temporary index location
356380
which does not exist anymore. If you intend to write such a merged Index,
@@ -368,6 +392,12 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
368392
if len(treeish) == 0 or len(treeish) > 3:
369393
raise ValueError("Please specify between 1 and 3 treeish, got %i" % len(treeish))
370394

395+
if not allow_unsafe_options:
396+
Git.check_unsafe_options(
397+
options=Git._option_candidates(treeish, kwargs),
398+
unsafe_options=cls.unsafe_git_read_tree_options,
399+
)
400+
371401
arg_list: List[Union[Treeish, str]] = []
372402
# Ignore that the working tree and index possibly are out of date.
373403
if len(treeish) > 1:
@@ -994,6 +1024,7 @@ def remove(
9941024
self,
9951025
items: Union[PathLike, Sequence[Union[PathLike, Blob, BaseIndexEntry, "Submodule"]]],
9961026
working_tree: bool = False,
1027+
allow_unsafe_options: bool = False,
9971028
**kwargs: Any,
9981029
) -> List[str]:
9991030
R"""Remove the given items from the index and optionally from the working tree
@@ -1024,6 +1055,10 @@ def remove(
10241055
physically removing the respective file. This may fail if there are
10251056
uncommitted changes in it.
10261057
1058+
:param allow_unsafe_options:
1059+
Allow unsafe options such as ``--pathspec-from-file`` to be passed to
1060+
:manpage:`git-rm(1)`.
1061+
10271062
:param kwargs:
10281063
Additional keyword arguments to be passed to :manpage:`git-rm(1)`, such as
10291064
``r`` to allow recursive removal.
@@ -1035,6 +1070,11 @@ def remove(
10351070
This is interesting to know in case you have provided a directory or globs.
10361071
Paths are relative to the repository.
10371072
"""
1073+
if not allow_unsafe_options:
1074+
Git.check_unsafe_options(
1075+
options=Git._option_candidates([], kwargs),
1076+
unsafe_options=Git.unsafe_git_pathspec_from_file_options,
1077+
)
10381078
args = []
10391079
if not working_tree:
10401080
args.append("--cached")
@@ -1416,6 +1456,7 @@ def reset(
14161456
working_tree: bool = False,
14171457
paths: Union[None, Iterable[PathLike]] = None,
14181458
head: bool = False,
1459+
allow_unsafe_options: bool = False,
14191460
**kwargs: Any,
14201461
) -> "IndexFile":
14211462
"""Reset the index to reflect the tree at the given commit. This will not adjust
@@ -1447,6 +1488,9 @@ def reset(
14471488
The paths need to exist at the commit, otherwise an exception will be
14481489
raised.
14491490
1491+
:param allow_unsafe_options:
1492+
Allow options that may write to arbitrary paths.
1493+
14501494
:param kwargs:
14511495
Additional keyword arguments passed to :manpage:`git-reset(1)`.
14521496
@@ -1463,7 +1507,7 @@ def reset(
14631507
"""
14641508
# What we actually want to do is to merge the tree into our existing index,
14651509
# which is what git-read-tree does.
1466-
new_inst = type(self).from_tree(self.repo, commit)
1510+
new_inst = type(self).from_tree(self.repo, commit, allow_unsafe_options=allow_unsafe_options)
14671511
if not paths:
14681512
self.entries = new_inst.entries
14691513
else:

‎git/refs/head.py‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
from typing import Any, Sequence, TYPE_CHECKING, Union
2121

22+
from git.cmd import Git
2223
from git.types import Commit_ish, PathLike
2324

2425
if TYPE_CHECKING:
@@ -62,6 +63,7 @@ def reset(
6263
index: bool = True,
6364
working_tree: bool = False,
6465
paths: Union[PathLike, Sequence[PathLike], None] = None,
66+
allow_unsafe_options: bool = False,
6567
**kwargs: Any,
6668
) -> "HEAD":
6769
"""Reset our HEAD to the given commit optionally synchronizing the index and
@@ -84,12 +86,21 @@ def reset(
8486
Single path or list of paths relative to the git root directory
8587
that are to be reset. This allows to partially reset individual files.
8688
89+
:param allow_unsafe_options:
90+
Allow unsafe options such as ``--pathspec-from-file`` to be passed to
91+
:manpage:`git-reset(1)`.
92+
8793
:param kwargs:
8894
Additional arguments passed to :manpage:`git-reset(1)`.
8995
9096
:return:
9197
self
9298
"""
99+
if not allow_unsafe_options:
100+
Git.check_unsafe_options(
101+
options=Git._option_candidates([commit], kwargs),
102+
unsafe_options=Git.unsafe_git_pathspec_from_file_options,
103+
)
93104
mode: Union[str, None]
94105
mode = "--soft"
95106
if index:
@@ -234,7 +245,12 @@ def rename(self, new_path: PathLike, force: bool = False) -> "Head":
234245
self.path = "%s/%s" % (self._common_path_default, new_path)
235246
return self
236247

237-
def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
248+
def checkout(
249+
self,
250+
force: bool = False,
251+
allow_unsafe_options: bool = False,
252+
**kwargs: Any,
253+
) -> Union["HEAD", "Head"]:
238254
"""Check out this head by setting the HEAD to this reference, by updating the
239255
index to reflect the tree we point to and by updating the working tree to
240256
reflect the latest index.
@@ -246,6 +262,10 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
246262
If ``False``, :exc:`~git.exc.GitCommandError` will be raised in that
247263
situation.
248264
265+
:param allow_unsafe_options:
266+
Allow unsafe options such as ``--pathspec-from-file`` to be passed to
267+
:manpage:`git-checkout(1)`.
268+
249269
:param kwargs:
250270
Additional keyword arguments to be passed to git checkout, e.g.
251271
``b="new_branch"`` to create a new branch at the given spot.
@@ -261,6 +281,11 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]:
261281
the HEAD detached which is allowed and possible, but remains a special state
262282
that some tools might not be able to handle.
263283
"""
284+
if not allow_unsafe_options:
285+
Git.check_unsafe_options(
286+
options=Git._option_candidates([], kwargs),
287+
unsafe_options=Git.unsafe_git_pathspec_from_file_options,
288+
)
264289
kwargs["f"] = force
265290
if kwargs["f"] is False:
266291
kwargs.pop("f")

‎git/repo/base.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,14 @@ class Repo:
142142
re_author_committer_start = re.compile(r"^(author|committer)")
143143
re_tab_full_line = re.compile(r"^\t(.*)$")
144144

145+
unsafe_git_init_options = [
146+
# Can install hooks that execute during later Git commands:
147+
"--template",
148+
# Redirects the repository metadata to a caller-controlled path:
149+
"--separate-git-dir",
150+
]
151+
"""Options to :manpage:`git-init(1)` that permit unsafe code execution or I/O."""
152+
145153
unsafe_git_clone_options = [
146154
# Executes arbitrary commands:
147155
"--upload-pack",
@@ -1398,6 +1406,7 @@ def init(
13981406
mkdir: bool = True,
13991407
odbt: Type[GitCmdObjectDB] = GitCmdObjectDB,
14001408
expand_vars: bool = True,
1409+
allow_unsafe_options: bool = False,
14011410
**kwargs: Any,
14021411
) -> "Repo":
14031412
"""Initialize a git repository at the given path if specified.
@@ -1422,13 +1431,22 @@ def init(
14221431
information disclosure, allowing attackers to access the contents of
14231432
environment variables.
14241433
1434+
:param allow_unsafe_options:
1435+
Allow unsafe options to be used, such as ``--template`` and
1436+
``--separate-git-dir``.
1437+
14251438
:param kwargs:
14261439
Keyword arguments serving as additional options to the
14271440
:manpage:`git-init(1)` command.
14281441
14291442
:return:
14301443
:class:`Repo` (the newly created repo)
14311444
"""
1445+
if not allow_unsafe_options:
1446+
Git.check_unsafe_options(
1447+
options=Git._option_candidates([], kwargs),
1448+
unsafe_options=cls.unsafe_git_init_options,
1449+
)
14321450
if path:
14331451
path = expand_path(path, expand_vars)
14341452
if mkdir and path and not osp.exists(path):

‎test/test_config.py‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,43 @@ def test_set_value_rejects_unsafe_section_and_option_names(self, rw_dir):
194194
self.assertEqual(git_config.get_value("user", "name"), "safe")
195195
self.assertFalse(git_config.has_section("core"))
196196

197+
@with_rw_directory
198+
def test_writer_rejects_invalid_option_names(self, rw_dir):
199+
config_path = osp.join(rw_dir, "config")
200+
bad_options = (
201+
"name=value",
202+
"name#comment",
203+
"name;comment",
204+
"name with space",
205+
"name\twith-tab",
206+
"name[section",
207+
"name]section",
208+
"name:colon",
209+
'name"quote',
210+
"name\\escape",
211+
)
212+
213+
with GitConfigParser(config_path, read_only=False) as git_config:
214+
git_config.add_section("user")
215+
for bad_option in bad_options:
216+
with pytest.raises(ValueError, match="option name"):
217+
git_config.set("user", bad_option, "unsafe")
218+
with pytest.raises(ValueError, match="option name"):
219+
git_config.set_value("user", bad_option, "unsafe")
220+
with pytest.raises(ValueError, match="option name"):
221+
git_config.add_value("user", bad_option, "unsafe")
222+
223+
git_config.set_value("user", "safe-option1", "safe")
224+
git_config.set_value("user", "safe_option2", "safe")
225+
git_config.set_value("user", "3safe_option", "safe")
226+
git_config.set_value("user", "safe.option3", "safe")
227+
228+
with GitConfigParser(config_path, read_only=True) as git_config:
229+
self.assertEqual(git_config.get_value("user", "safe-option1"), "safe")
230+
self.assertEqual(git_config.get_value("user", "safe_option2"), "safe")
231+
self.assertEqual(git_config.get_value("user", "3safe_option"), "safe")
232+
self.assertEqual(git_config.get_value("user", "safe.option3"), "safe")
233+
197234
@with_rw_directory
198235
def test_writer_rejects_unquoted_section_terminators(self, rw_dir):
199236
config_path = osp.join(rw_dir, "config")

‎test/test_git.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,18 @@ def test_option_candidates_ignore_untransformed_kwargs(self):
215215

216216
self.assertEqual(options, ["--max-count"])
217217

218+
def test_option_candidates_include_falsey_non_boolean_values(self):
219+
kwargs = {"pathspec_from_file": 0}
220+
candidates = Git._option_candidates(kwargs=kwargs)
221+
222+
self.assertEqual(candidates, ["--pathspec-from-file"])
223+
self.assertEqual(self.git.transform_kwargs(**kwargs), ["--pathspec-from-file=0"])
224+
with self.assertRaises(UnsafeOptionError):
225+
Git.check_unsafe_options(
226+
options=candidates,
227+
unsafe_options=Git.unsafe_git_pathspec_from_file_options,
228+
)
229+
218230
def test_option_candidates_include_split_single_char_option_values(self):
219231
cases = [
220232
({"n": "--upload-pack=helper"}, ["-n", "--upload-pack=helper"], ["--upload-pack"]),
@@ -230,7 +242,15 @@ def test_option_candidates_include_split_single_char_option_values(self):
230242

231243
unsplit_kwargs = {"n": "--upload-pack=helper", "split_single_char_options": False}
232244
self.assertEqual(self.git.transform_kwargs(**unsplit_kwargs), ["-n--upload-pack=helper"])
233-
self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n"])
245+
self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n--upload-pack=helper"])
246+
247+
def test_option_candidates_include_joined_single_char_option_values(self):
248+
kwargs = {"n": "uhelper", "split_single_char_options": False}
249+
candidates = Git._option_candidates(kwargs=kwargs)
250+
251+
self.assertEqual(candidates, ["-nuhelper"])
252+
with self.assertRaises(UnsafeOptionError):
253+
Git.check_unsafe_options(options=candidates, unsafe_options=["-u"])
234254

235255
_shell_cases = (
236256
# value_in_call, value_from_class, expected_popen_arg

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL