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

fix: Guard unsafe git command options (GHSA-956x-8gvw-wg5v) · gitpython-developers/GitPython@701ce32 · GitHub

/ GitPython Public

Commit 701ce32

Browse files
codex
authored and
Byron
committed
fix: Guard unsafe git command options (GHSA-956x-8gvw-wg5v)
Reject command options that can execute arbitrary helpers or write files when passed through archive, revision traversal, blame, clone, fetch, pull, push, and ls-remote APIs. Validate positional option tokens and transformed keyword options consistently, including Git-supported long-option abbreviations, attached short-option values, and falsey stringifiable remotes. Preserve explicit allow_unsafe_options and allow_unsafe_protocols escape hatches. Add regression coverage for each guarded API, option spelling and abbreviation forms, inert keyword values, mixed positional and keyword candidates, tuple blame options, stringifiable remotes, and unsafe commands that must not create marker files. Reviewed-by: Sebastian Thiel <sebastian.thiel@icloud.com>
1 parent 65a7283 commit 701ce32

9 files changed

Lines changed: 279 additions & 12 deletions

File tree

‎git/cmd.py‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,11 @@ class Git(metaclass=_GitMeta):
649649

650650
re_unsafe_protocol = re.compile(r"(.+)::.+")
651651

652+
unsafe_git_ls_remote_options = [
653+
# This option allows arbitrary command execution in git-ls-remote.
654+
"--upload-pack",
655+
]
656+
652657
def __getstate__(self) -> Dict[str, Any]:
653658
return slots_to_dict(self, exclude=self._excluded_)
654659

@@ -1022,6 +1027,20 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) ->
10221027
f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
10231028
)
10241029

1030+
@classmethod
1031+
def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[str, Any]] = None) -> List[str]:
1032+
"""Collect possible option spellings before command-line transformation."""
1033+
options = [
1034+
option for option in cls._unpack_args([arg for arg in args if arg is not None]) if option.startswith("-")
1035+
]
1036+
if kwargs:
1037+
for key, value in kwargs.items():
1038+
values = value if isinstance(value, (list, tuple)) else (value,)
1039+
if any(value is True or (value is not False and value is not None) for value in values):
1040+
key = str(key)
1041+
options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}")
1042+
return options
1043+
10251044
AutoInterrupt: TypeAlias = _AutoInterrupt
10261045

10271046
CatFileContentStream: TypeAlias = _CatFileContentStream
@@ -1079,6 +1098,22 @@ def set_persistent_git_options(self, **kwargs: Any) -> None:
10791098

10801099
self._persistent_git_options = self.transform_kwargs(split_single_char_options=True, **kwargs)
10811100

1101+
def ls_remote(
1102+
self,
1103+
*args: Any,
1104+
allow_unsafe_options: bool = False,
1105+
**kwargs: Any,
1106+
) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]:
1107+
"""List references in a remote repository.
1108+
1109+
:param allow_unsafe_options:
1110+
Allow unsafe options, like ``--upload-pack``.
1111+
"""
1112+
if not allow_unsafe_options:
1113+
candidate_options = self._option_candidates(args, kwargs)
1114+
Git.check_unsafe_options(options=candidate_options, unsafe_options=self.unsafe_git_ls_remote_options)
1115+
return self._call_process("ls_remote", *args, **kwargs)
1116+
10821117
@property
10831118
def working_dir(self) -> Union[None, PathLike]:
10841119
""":return: Git directory we are working on"""
@@ -1585,7 +1620,7 @@ def transform_kwargs(self, split_single_char_options: bool = True, **kwargs: Any
15851620
return args
15861621

15871622
@classmethod
1588-
def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]:
1623+
def _unpack_args(cls, arg_list: Sequence[Any]) -> List[str]:
15891624
outlist = []
15901625
if isinstance(arg_list, (list, tuple)):
15911626
for arg in arg_list:

‎git/objects/commit.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,12 @@ class Commit(base.Object, TraversableIterableObj, Diffable, Serializable):
8686
# INVARIANTS
8787
default_encoding = "UTF-8"
8888

89+
# Options to :manpage:`git-rev-list(1)` that can overwrite files.
90+
unsafe_git_rev_options = [
91+
"--output",
92+
"-o",
93+
]
94+
8995
type: Literal["commit"] = "commit"
9096

9197
__slots__ = (
@@ -302,6 +308,7 @@ def iter_items(
302308
repo: "Repo",
303309
rev: Union[str, "Commit", "SymbolicReference"],
304310
paths: Union[PathLike, Sequence[PathLike]] = "",
311+
allow_unsafe_options: bool = False,
305312
**kwargs: Any,
306313
) -> Iterator["Commit"]:
307314
R"""Find all commits matching the given criteria.
@@ -330,6 +337,11 @@ def iter_items(
330337
raise ValueError("--pretty cannot be used as parsing expects single sha's only")
331338
# END handle pretty
332339

340+
if not allow_unsafe_options:
341+
Git.check_unsafe_options(
342+
options=Git._option_candidates([rev], kwargs), unsafe_options=cls.unsafe_git_rev_options
343+
)
344+
333345
# Use -- in all cases, to prevent possibility of ambiguous arguments.
334346
# See https://github.com/gitpython-developers/GitPython/issues/264.
335347

‎git/remote.py‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1071,7 +1071,10 @@ def fetch(
10711071
Git.check_unsafe_protocols(ref)
10721072

10731073
if not allow_unsafe_options:
1074-
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_fetch_options)
1074+
Git.check_unsafe_options(
1075+
options=Git._option_candidates([], kwargs),
1076+
unsafe_options=self.unsafe_git_fetch_options,
1077+
)
10751078

10761079
proc = self.repo.git.fetch(
10771080
"--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs
@@ -1125,7 +1128,10 @@ def pull(
11251128
Git.check_unsafe_protocols(ref)
11261129

11271130
if not allow_unsafe_options:
1128-
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_pull_options)
1131+
Git.check_unsafe_options(
1132+
options=Git._option_candidates([], kwargs),
1133+
unsafe_options=self.unsafe_git_pull_options,
1134+
)
11291135

11301136
proc = self.repo.git.pull(
11311137
"--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs
@@ -1198,7 +1204,10 @@ def push(
11981204
Git.check_unsafe_protocols(ref)
11991205

12001206
if not allow_unsafe_options:
1201-
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_push_options)
1207+
Git.check_unsafe_options(
1208+
options=Git._option_candidates([], kwargs),
1209+
unsafe_options=self.unsafe_git_push_options,
1210+
)
12021211

12031212
proc = self.repo.git.push(
12041213
"--",

‎git/repo/base.py‎

Lines changed: 73 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,20 @@ class Repo:
161161
https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt
162162
"""
163163

164+
unsafe_git_archive_options = [
165+
# Allows arbitrary command execution through the remote git-upload-archive command.
166+
"--exec",
167+
# Writes output to a caller-controlled filesystem path.
168+
"--output",
169+
"-o",
170+
]
171+
172+
unsafe_git_revision_options = [
173+
# This option allows output to be written to arbitrary files before revision parsing.
174+
"--output",
175+
"-o",
176+
]
177+
164178
# Invariants
165179
config_level: ConfigLevels_Tup = ("system", "user", "global", "repository")
166180
"""Represents the configuration level of a configuration file."""
@@ -775,6 +789,7 @@ def iter_commits(
775789
self,
776790
rev: Union[str, Commit, "SymbolicReference", None] = None,
777791
paths: Union[PathLike, Sequence[PathLike]] = "",
792+
allow_unsafe_options: bool = False,
778793
**kwargs: Any,
779794
) -> Iterator[Commit]:
780795
"""An iterator of :class:`~git.objects.commit.Commit` objects representing the
@@ -792,6 +807,9 @@ def iter_commits(
792807
Arguments to be passed to :manpage:`git-rev-list(1)`.
793808
Common ones are ``max_count`` and ``skip``.
794809
810+
:param allow_unsafe_options:
811+
Allow unsafe options in the revision argument, like ``--output``.
812+
795813
:note:
796814
To receive only commits between two named revisions, use the
797815
``"revA...revB"`` revision specifier.
@@ -802,7 +820,18 @@ def iter_commits(
802820
if rev is None:
803821
rev = self.head.commit
804822

805-
return Commit.iter_items(self, rev, paths, **kwargs)
823+
if not allow_unsafe_options:
824+
Git.check_unsafe_options(
825+
options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
826+
)
827+
828+
return Commit.iter_items(
829+
self,
830+
rev,
831+
paths,
832+
allow_unsafe_options=allow_unsafe_options,
833+
**kwargs,
834+
)
806835

807836
def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]:
808837
R"""Find the closest common ancestor for the given revision
@@ -1079,7 +1108,9 @@ def active_branch(self) -> Head:
10791108
)
10801109
return active_branch
10811110

1082-
def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) -> Iterator["BlameEntry"]:
1111+
def blame_incremental(
1112+
self, rev: str | HEAD | None, file: str, allow_unsafe_options: bool = False, **kwargs: Any
1113+
) -> Iterator["BlameEntry"]:
10831114
"""Iterator for blame information for the given file at the given revision.
10841115
10851116
Unlike :meth:`blame`, this does not return the actual file's contents, only a
@@ -1090,6 +1121,9 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) ->
10901121
uncommitted changes. Otherwise, anything successfully parsed by
10911122
:manpage:`git-rev-parse(1)` is a valid option.
10921123
1124+
:param allow_unsafe_options:
1125+
Allow unsafe options in revision argument, like ``--output``.
1126+
10931127
:return:
10941128
Lazy iterator of :class:`BlameEntry` tuples, where the commit indicates the
10951129
commit to blame for the line, and range indicates a span of line numbers in
@@ -1098,6 +1132,10 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) ->
10981132
If you combine all line number ranges outputted by this command, you should get
10991133
a continuous range spanning all line numbers in the file.
11001134
"""
1135+
if not allow_unsafe_options:
1136+
Git.check_unsafe_options(
1137+
options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
1138+
)
11011139

11021140
data: bytes = self.git.blame(rev, "--", file, p=True, incremental=True, stdout_as_string=False, **kwargs)
11031141
commits: Dict[bytes, Commit] = {}
@@ -1176,7 +1214,8 @@ def blame(
11761214
rev: Union[str, HEAD, None],
11771215
file: str,
11781216
incremental: bool = False,
1179-
rev_opts: Optional[List[str]] = None,
1217+
rev_opts: Optional[Sequence[str]] = None,
1218+
allow_unsafe_options: bool = False,
11801219
**kwargs: Any,
11811220
) -> List[List[Commit | List[str | bytes] | None]] | Iterator[BlameEntry] | None:
11821221
"""The blame information for the given file at the given revision.
@@ -1186,6 +1225,9 @@ def blame(
11861225
uncommitted changes. Otherwise, anything successfully parsed by
11871226
:manpage:`git-rev-parse(1)` is a valid option.
11881227
1228+
:param allow_unsafe_options:
1229+
Allow unsafe options in revision argument, like ``--output``.
1230+
11891231
:return:
11901232
list: [git.Commit, list: [<line>]]
11911233
@@ -1195,9 +1237,14 @@ def blame(
11951237
appearance.
11961238
"""
11971239
if incremental:
1198-
return self.blame_incremental(rev, file, **kwargs)
1199-
rev_opts = rev_opts or []
1200-
data: bytes = self.git.blame(rev, *rev_opts, "--", file, p=True, stdout_as_string=False, **kwargs)
1240+
return self.blame_incremental(rev, file, allow_unsafe_options=allow_unsafe_options, **kwargs)
1241+
rev_opts_list = list(rev_opts or [])
1242+
if not allow_unsafe_options:
1243+
Git.check_unsafe_options(
1244+
options=Git._option_candidates([rev, rev_opts_list], kwargs),
1245+
unsafe_options=self.unsafe_git_revision_options,
1246+
)
1247+
data: bytes = self.git.blame(rev, *rev_opts_list, "--", file, p=True, stdout_as_string=False, **kwargs)
12011248
commits: Dict[str, Commit] = {}
12021249
blames: List[List[Commit | List[str | bytes] | None]] = []
12031250

@@ -1408,7 +1455,10 @@ def _clone(
14081455
if not allow_unsafe_protocols:
14091456
Git.check_unsafe_protocols(url)
14101457
if not allow_unsafe_options:
1411-
Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=cls.unsafe_git_clone_options)
1458+
Git.check_unsafe_options(
1459+
options=Git._option_candidates([], kwargs),
1460+
unsafe_options=cls.unsafe_git_clone_options,
1461+
)
14121462
if not allow_unsafe_options and multi:
14131463
Git.check_unsafe_options(options=multi, unsafe_options=cls.unsafe_git_clone_options)
14141464

@@ -1583,6 +1633,8 @@ def archive(
15831633
ostream: Union[TextIO, BinaryIO],
15841634
treeish: Optional[str] = None,
15851635
prefix: Optional[str] = None,
1636+
allow_unsafe_options: bool = False,
1637+
allow_unsafe_protocols: bool = False,
15861638
**kwargs: Any,
15871639
) -> Repo:
15881640
"""Archive the tree at the given revision.
@@ -1605,6 +1657,12 @@ def archive(
16051657
repository-relative path to a directory or file to place into the archive,
16061658
or a list or tuple of multiple paths.
16071659
1660+
:param allow_unsafe_options:
1661+
Allow unsafe options, like ``--exec`` or ``--output``.
1662+
1663+
:param allow_unsafe_protocols:
1664+
Allow unsafe protocols to be used in ``remote``, like ``ext``.
1665+
16081666
:raise git.exc.GitCommandError:
16091667
If something went wrong.
16101668
@@ -1615,6 +1673,14 @@ def archive(
16151673
treeish = self.head.commit
16161674
if prefix and "prefix" not in kwargs:
16171675
kwargs["prefix"] = prefix
1676+
remote = kwargs.get("remote")
1677+
if not allow_unsafe_protocols and remote is not None:
1678+
Git.check_unsafe_protocols(str(remote))
1679+
if not allow_unsafe_options:
1680+
Git.check_unsafe_options(
1681+
options=Git._option_candidates([], kwargs),
1682+
unsafe_options=self.unsafe_git_archive_options,
1683+
)
16181684
kwargs["output_stream"] = ostream
16191685
path = kwargs.pop("path", [])
16201686
path = cast(Union[PathLike, List[PathLike], Tuple[PathLike, ...]], path)

‎test/test_clone.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,13 @@ def test_clone_unsafe_options(self, rw_repo):
117117
tmp_file = tmp_dir / "pwn"
118118
unsafe_options = [
119119
f"--upload-pack='touch {tmp_file}'",
120+
f"--upl='touch {tmp_file}'",
120121
f"-u 'touch {tmp_file}'",
121122
f"-utouch {tmp_file}; false",
122123
f"-futouch${{IFS}}{tmp_file}; false",
123124
f"-qutouch${{IFS}}{tmp_file}; false",
124125
"--config=protocol.ext.allow=always",
126+
"--conf=protocol.ext.allow=always",
125127
"-c protocol.ext.allow=always",
126128
"-cprotocol.ext.allow=always",
127129
"-vcprotocol.ext.allow=always",
@@ -134,8 +136,10 @@ def test_clone_unsafe_options(self, rw_repo):
134136
unsafe_options = [
135137
{"upload-pack": f"touch {tmp_file}"},
136138
{"upload_pack": f"touch {tmp_file}"},
139+
{"upl": f"touch {tmp_file}"},
137140
{"u": f"touch {tmp_file}"},
138141
{"config": "protocol.ext.allow=always"},
142+
{"conf": "protocol.ext.allow=always"},
139143
{"c": "protocol.ext.allow=always"},
140144
]
141145
for unsafe_option in unsafe_options:

‎test/test_commit.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import copy
77
from datetime import datetime
88
from io import BytesIO
9+
import tempfile
910
import os.path as osp
1011
import re
1112
import sys
@@ -15,6 +16,7 @@
1516
from gitdb import IStream
1617

1718
from git import Actor, Commit, Repo
19+
from git.exc import UnsafeOptionError
1820
from git.objects.util import tzoffset, utc
1921
from git.repo.fun import touch
2022

@@ -288,6 +290,17 @@ def test_iter_items(self):
288290
# pretty not allowed.
289291
self.assertRaises(ValueError, Commit.iter_items, self.rorepo, "master", pretty="raw")
290292

293+
def test_iter_items_rejects_unsafe_revision(self):
294+
with tempfile.TemporaryDirectory() as tdir:
295+
marker = osp.join(tdir, "pwn")
296+
self.assertRaises(UnsafeOptionError, Commit.iter_items, self.rorepo, f"--output={marker}")
297+
298+
def test_iter_items_rejects_unsafe_options(self):
299+
with tempfile.TemporaryDirectory() as tdir:
300+
marker = osp.join(tdir, "pwn")
301+
with self.assertRaises(UnsafeOptionError):
302+
list(Commit.iter_items(self.rorepo, "HEAD", output=marker))
303+
291304
def test_rev_list_bisect_all(self):
292305
"""
293306
'git rev-list --bisect-all' returns additional information

0 commit comments

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