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

Merge pull request #2168 from gitpython-developers/advisory-fix-1 · gitpython-developers/GitPython@65a7283 · GitHub

/ GitPython Public

Commit 65a7283

Browse files
Byron
authored
Merge pull request #2168 from gitpython-developers/advisory-fix-1
Reject abbreviated forms of unsafe git options
2 parents 3e59876 + 5680608 commit 65a7283

4 files changed

Lines changed: 131 additions & 7 deletions

File tree

‎git/cmd.py‎

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -961,17 +961,66 @@ 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`.
983+
# Git accepts any unambiguous prefix of a long option, so an abbreviated
984+
# spelling such as `--upl` for `--upload-pack` must be rejected too. An
985+
# option is unsafe if its canonical name is a prefix of any blocked
986+
# option's canonical name. Only long options and multi-character kwargs
987+
# can be abbreviations; single-character short options remain exact-match
988+
# only.
970989
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")
999+
options_are_kwargs = all(not option.startswith("-") for option in options)
9711000
for option in options:
972-
unsafe_option = canonical_unsafe_options.get(cls._canonicalize_option_name(option))
1001+
candidate = cls._canonicalize_option_name(option)
1002+
if not candidate:
1003+
continue
1004+
unsafe_option = canonical_unsafe_options.get(candidate)
9731005
if unsafe_option is not None:
9741006
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
1017+
if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)):
1018+
continue
1019+
for canonical, unsafe_option in canonical_unsafe_options.items():
1020+
if canonical.startswith(candidate):
1021+
raise UnsafeOptionError(
1022+
f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
1023+
)
9751024

9761025
AutoInterrupt: TypeAlias = _AutoInterrupt
9771026

‎test/test_clone.py‎

Lines changed: 37 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):
@@ -138,6 +143,31 @@ def test_clone_unsafe_options(self, rw_repo):
138143
rw_repo.clone(tmp_dir, **unsafe_option)
139144
assert not tmp_file.exists()
140145

146+
@with_rw_repo("HEAD")
147+
def test_clone_unsafe_options_abbreviated(self, rw_repo):
148+
with tempfile.TemporaryDirectory() as tdir:
149+
tmp_dir = pathlib.Path(tdir)
150+
tmp_file = tmp_dir / "pwn"
151+
unsafe_options = [
152+
f"--upl='touch {tmp_file}'",
153+
f"--upload-pac='touch {tmp_file}'",
154+
"--conf=protocol.ext.allow=always",
155+
]
156+
for unsafe_option in unsafe_options:
157+
with self.assertRaises(UnsafeOptionError):
158+
rw_repo.clone(tmp_dir, multi_options=[unsafe_option])
159+
assert not tmp_file.exists()
160+
161+
unsafe_kwargs = [
162+
{"upl": f"touch {tmp_file}"},
163+
{"upload_pac": f"touch {tmp_file}"},
164+
{"conf": "protocol.ext.allow=always"},
165+
]
166+
for unsafe_option in unsafe_kwargs:
167+
with self.assertRaises(UnsafeOptionError):
168+
rw_repo.clone(tmp_dir, **unsafe_option)
169+
assert not tmp_file.exists()
170+
141171
@with_rw_repo("HEAD")
142172
def test_clone_unsafe_options_are_checked_after_splitting_multi_options(self, rw_repo):
143173
with tempfile.TemporaryDirectory() as tdir:
@@ -191,7 +221,9 @@ def test_clone_safe_options(self, rw_repo):
191221
options = [
192222
"--depth=1",
193223
"--single-branch",
224+
"--origin upload",
194225
"-q",
226+
"-oupstream",
195227
]
196228
for option in options:
197229
destination = tmp_dir / option
@@ -207,8 +239,13 @@ def test_clone_from_unsafe_options(self, rw_repo):
207239
unsafe_options = [
208240
f"--upload-pack='touch {tmp_file}'",
209241
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",
210245
"--config=protocol.ext.allow=always",
211246
"-c protocol.ext.allow=always",
247+
"-cprotocol.ext.allow=always",
248+
"-vcprotocol.ext.allow=always",
212249
]
213250
for unsafe_option in unsafe_options:
214251
with self.assertRaises(UnsafeOptionError):

‎test/test_git.py‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,34 @@ def test_check_unsafe_options_normalizes_kwargs(self):
170170
with self.assertRaises(UnsafeOptionError):
171171
Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
172172

173+
def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self):
174+
unsafe_options = ["--upload-pack"]
175+
176+
Git.check_unsafe_options(options=["-u", "u", "-upl"], unsafe_options=unsafe_options)
177+
with self.assertRaises(UnsafeOptionError):
178+
Git.check_unsafe_options(options=["--u"], unsafe_options=unsafe_options)
179+
180+
def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self):
181+
Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"])
182+
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+
173201
_shell_cases = (
174202
# value_in_call, value_from_class, expected_popen_arg
175203
(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)
Back | FazBrowse Home | New Git URL