Visitar URL original
fix: reject joined unsafe short options · gitpython-developers/GitPython@8b53090 · GitHub
Skip to content

Commit 8b53090

Browse files
codexByron
codex
authored and
Byron
committed
fix: reject joined unsafe short options
GitPython blocks options such as --upload-pack/-u and --config/-c because they can execute arbitrary commands unless unsafe options are explicitly allowed. Git also accepts a short option with its value joined to the same token, including after clusterable flags. Forms such as -uVALUE, -fuVALUE, -cVALUE, and -vcVALUE could therefore bypass an exact-token check. Parse single-dash option tokens sufficiently to recognize blocked short options while preserving safe attached values such as -oupstream. Also distinguish clone multi-option values from bare kwargs so positional values are not treated as long-option abbreviations. This completes the option-validation hardening for GHSA-2f96-g7mh-g2hx.
1 parent 25c684b commit 8b53090

4 files changed

Lines changed: 78 additions & 6 deletions

File tree

‎git/cmd.py‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -961,10 +961,23 @@ def _canonicalize_option_name(cls, option: str) -> str:
961961

962962
@classmethod
963963
def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None:
964-
"""Check for unsafe options.
965-
966-
Some options that are passed to ``git <command>`` can be used to execute
967-
arbitrary commands. These are blocked by default.
964+
"""Raise :class:`~git.exc.UnsafeOptionError` for blocked option spellings.
965+
966+
In addition to exact matches, this rejects abbreviated long options accepted
967+
by Git (for example, ``--upl`` for ``--upload-pack``) and unsafe short options
968+
whose values are joined to the same token, including after clusterable flags
969+
(for example, ``-uVALUE`` and ``-fuVALUE``).
970+
971+
A list containing only bare names is treated as normalized keyword arguments,
972+
so multi-character names such as ``upload_p`` are checked as long-option
973+
abbreviations. If any item starts with ``-``, the list is treated as tokenized
974+
command-line input: bare items can be option values and are not checked as
975+
abbreviations. Thus ``["--origin", "upload"]`` is allowed. Single-dash options
976+
use short-option parsing rather than broad prefix matching, preserving safe
977+
attached values such as ``-oupstream`` and ``-bcurrent``.
978+
979+
Some options passed to ``git <command>`` can execute arbitrary commands and
980+
are therefore blocked by default unless the caller explicitly allows them.
968981
"""
969982
# Options can be of the form `foo`, `--foo`, `--foo bar`, or `--foo=bar`.
970983
# Git accepts any unambiguous prefix of a long option, so an abbreviated
@@ -974,6 +987,15 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) ->
974987
# can be abbreviations; single-character short options remain exact-match
975988
# only.
976989
canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options}
990+
unsafe_short_options = {
991+
canonical: option
992+
for canonical, option in canonical_unsafe_options.items()
993+
if option.startswith("-") and not option.startswith("--") and len(canonical) == 1
994+
}
995+
# These value-less Git flags can be clustered before another short option
996+
# (for example, ``-fuVALUE``). Stop at any other character because it may
997+
# begin an attached value, as ``o`` does in the safe option ``-oupstream``.
998+
clusterable_short_options = frozenset("46flnqsv")
977999
options_are_kwargs = all(not option.startswith("-") for option in options)
9781000
for option in options:
9791001
candidate = cls._canonicalize_option_name(option)
@@ -982,6 +1004,16 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) ->
9821004
unsafe_option = canonical_unsafe_options.get(candidate)
9831005
if unsafe_option is not None:
9841006
raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
1007+
option_token = option.split("=", 1)[0].split(None, 1)[0]
1008+
if option_token.startswith("-") and not option_token.startswith("--"):
1009+
for option_char in option_token[1:]:
1010+
unsafe_option = unsafe_short_options.get(option_char)
1011+
if unsafe_option is not None:
1012+
raise UnsafeOptionError(
1013+
f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
1014+
)
1015+
if option_char not in clusterable_short_options:
1016+
break
9851017
if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)):
9861018
continue
9871019
for canonical, unsafe_option in canonical_unsafe_options.items():

‎test/test_clone.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,13 @@ def test_clone_unsafe_options(self, rw_repo):
118118
unsafe_options = [
119119
f"--upload-pack='touch {tmp_file}'",
120120
f"-u 'touch {tmp_file}'",
121+
f"-utouch {tmp_file}; false",
122+
f"-futouch${{IFS}}{tmp_file}; false",
123+
f"-qutouch${{IFS}}{tmp_file}; false",
121124
"--config=protocol.ext.allow=always",
122125
"-c protocol.ext.allow=always",
126+
"-cprotocol.ext.allow=always",
127+
"-vcprotocol.ext.allow=always",
123128
]
124129
for unsafe_option in unsafe_options:
125130
with self.assertRaises(UnsafeOptionError):
@@ -216,7 +221,9 @@ def test_clone_safe_options(self, rw_repo):
216221
options = [
217222
"--depth=1",
218223
"--single-branch",
224+
"--origin upload",
219225
"-q",
226+
"-oupstream",
220227
]
221228
for option in options:
222229
destination = tmp_dir / option
@@ -232,8 +239,13 @@ def test_clone_from_unsafe_options(self, rw_repo):
232239
unsafe_options = [
233240
f"--upload-pack='touch {tmp_file}'",
234241
f"-u 'touch {tmp_file}'",
242+
f"-utouch {tmp_file}; false",
243+
f"-futouch${{IFS}}{tmp_file}; false",
244+
f"-qutouch${{IFS}}{tmp_file}; false",
235245
"--config=protocol.ext.allow=always",
236246
"-c protocol.ext.allow=always",
247+
"-cprotocol.ext.allow=always",
248+
"-vcprotocol.ext.allow=always",
237249
]
238250
for unsafe_option in unsafe_options:
239251
with self.assertRaises(UnsafeOptionError):

‎test/test_git.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,24 @@ def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self
180180
def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self):
181181
Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"])
182182

183+
def test_check_unsafe_options_rejects_joined_unsafe_short_options(self):
184+
cases = [
185+
(["-utouch /tmp/pwn"], ["-u"]),
186+
(["-futouch /tmp/pwn"], ["-u"]),
187+
(["-qutouch${IFS}/tmp/pwn"], ["-u"]),
188+
(["-cprotocol.ext.allow=always"], ["-c"]),
189+
(["-vcprotocol.ext.allow=always"], ["-c"]),
190+
]
191+
192+
for options, unsafe_options in cases:
193+
with self.assertRaises(UnsafeOptionError):
194+
Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
195+
196+
def test_check_unsafe_options_allows_attached_safe_short_option_values(self):
197+
unsafe_options = ["--upload-pack", "-u", "--config", "-c"]
198+
199+
Git.check_unsafe_options(options=["-oupstream", "-bcurrent"], unsafe_options=unsafe_options)
200+
183201
_shell_cases = (
184202
# value_in_call, value_from_class, expected_popen_arg
185203
(None, False, False),

‎test/test_remote.py‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,11 @@ def test_fetch_unsafe_options(self, rw_repo):
832832
remote = rw_repo.remote("origin")
833833
tmp_dir = Path(tdir)
834834
tmp_file = tmp_dir / "pwn"
835-
unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
835+
unsafe_options = [
836+
{"upload-pack": f"touch {tmp_file}"},
837+
{"upload_pack": f"touch {tmp_file}"},
838+
{"upload_p": f"touch {tmp_file}"},
839+
]
836840
for unsafe_option in unsafe_options:
837841
with self.assertRaises(UnsafeOptionError):
838842
remote.fetch(**unsafe_option)
@@ -900,7 +904,11 @@ def test_pull_unsafe_options(self, rw_repo):
900904
remote = rw_repo.remote("origin")
901905
tmp_dir = Path(tdir)
902906
tmp_file = tmp_dir / "pwn"
903-
unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
907+
unsafe_options = [
908+
{"upload-pack": f"touch {tmp_file}"},
909+
{"upload_pack": f"touch {tmp_file}"},
910+
{"upload_p": f"touch {tmp_file}"},
911+
]
904912
for unsafe_option in unsafe_options:
905913
with self.assertRaises(UnsafeOptionError):
906914
remote.pull(**unsafe_option)
@@ -971,7 +979,9 @@ def test_push_unsafe_options(self, rw_repo):
971979
unsafe_options = [
972980
{"receive-pack": f"touch {tmp_file}"},
973981
{"receive_pack": f"touch {tmp_file}"},
982+
{"receive_p": f"touch {tmp_file}"},
974983
{"exec": f"touch {tmp_file}"},
984+
{"exe": f"touch {tmp_file}"},
975985
]
976986
for unsafe_option in unsafe_options:
977987
assert not tmp_file.exists()

0 commit comments

Comments
 (0)