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

reject control chars in written values in configuration · gitpython-developers/GitPython@c417af4 · GitHub

/ GitPython Public

Commit c417af4

Browse files
codex
and
Byron
committed
reject control chars in written values in configuration
Reject CR, LF, and NUL in GitConfigParser values before writing them to git config files (which also is a deviation from Git which escapes them). GitConfigParser._write() serializes embedded newlines as indented continuation lines by replacing "\n" with "\n\t". Git itself skips leading whitespace before parsing config tokens, so an injected value such as: foo [core] hooksPath=/tmp/hooks is written in a form where the indented "[core]" line is still parsed by Git as a real section header. This lets attacker-controlled input passed to config_writer().set_value() poison repository config, including core.hooksPath, and redirect hook execution for later Git operations. Fail closed instead of stripping or normalizing these characters. Silent normalization can hide unsanitized caller input, and GitPython does not currently round-trip Git-style escaped values such as "\n" as embedded newlines. Apply the validation to set_value(), add_value(), and the public set() path so callers cannot bypass the safer helper API. Add regression tests for the advisory payload and for CR, LF, NUL, and bytes values. This preserves existing read behavior for config files that already contain multiline values while preventing GitPython from writing new unsafe values. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
1 parent 5a15361 commit c417af4

2 files changed

Lines changed: 55 additions & 2 deletions

File tree

‎git/config.py‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,24 @@ def _value_to_string(self, value: Union[str, bytes, int, float, bool]) -> str:
882882
return str(value)
883883
return force_text(value)
884884

885+
def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> str:
886+
value_str = self._value_to_string(value)
887+
if re.search(r"[\r\n\x00]", value_str):
888+
raise ValueError("Git config values must not contain CR, LF, or NUL")
889+
return value_str
890+
891+
@needs_values
892+
@set_dirty_and_flush_changes
893+
def set(
894+
self,
895+
section: str,
896+
option: str,
897+
value: Union[str, bytes, int, float, bool, None] = None,
898+
) -> None:
899+
if value is not None:
900+
value = self._value_to_string_safe(value)
901+
return super().set(section, option, value)
902+
885903
@needs_values
886904
@set_dirty_and_flush_changes
887905
def set_value(self, section: str, option: str, value: Union[str, bytes, int, float, bool]) -> "GitConfigParser":
@@ -902,9 +920,10 @@ def set_value(self, section: str, option: str, value: Union[str, bytes, int, flo
902920
:return:
903921
This instance
904922
"""
923+
value_str = self._value_to_string_safe(value)
905924
if not self.has_section(section):
906925
self.add_section(section)
907-
self.set(section, option, self._value_to_string(value))
926+
self.set(section, option, value_str)
908927
return self
909928

910929
@needs_values
@@ -929,9 +948,10 @@ def add_value(self, section: str, option: str, value: Union[str, bytes, int, flo
929948
:return:
930949
This instance
931950
"""
951+
value_str = self._value_to_string_safe(value)
932952
if not self.has_section(section):
933953
self.add_section(section)
934-
self._sections[section].add(option, self._value_to_string(value))
954+
self._sections[section].add(option, value_str)
935955
return self
936956

937957
def rename_section(self, section: str, new_name: str) -> "GitConfigParser":

‎test/test_config.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,39 @@ def test_config_value_with_trailing_new_line(self):
150150
git_config = GitConfigParser(config_file)
151151
git_config.read() # This should not throw an exception
152152

153+
@with_rw_directory
154+
def test_set_value_rejects_config_injection(self, rw_dir):
155+
config_path = osp.join(rw_dir, "config")
156+
payload = "foo\n[core]\nhooksPath=/tmp/hooks"
157+
158+
with GitConfigParser(config_path, read_only=False) as git_config:
159+
with pytest.raises(ValueError, match="CR, LF, or NUL"):
160+
git_config.set_value("user", "name", payload)
161+
162+
with GitConfigParser(config_path, read_only=True) as git_config:
163+
self.assertFalse(git_config.has_section("user"))
164+
self.assertFalse(git_config.has_section("core"))
165+
166+
@with_rw_directory
167+
def test_set_and_add_value_reject_unsafe_value_characters(self, rw_dir):
168+
config_path = osp.join(rw_dir, "config")
169+
bad_values = ("foo\rbar", "foo\nbar", "foo\x00bar", b"foo\nbar")
170+
171+
with GitConfigParser(config_path, read_only=False) as git_config:
172+
git_config.add_section("user")
173+
for bad_value in bad_values:
174+
with pytest.raises(ValueError, match="CR, LF, or NUL"):
175+
git_config.set("user", "name", bad_value)
176+
with pytest.raises(ValueError, match="CR, LF, or NUL"):
177+
git_config.set_value("user", "name", bad_value)
178+
with pytest.raises(ValueError, match="CR, LF, or NUL"):
179+
git_config.add_value("user", "name", bad_value)
180+
181+
git_config.set_value("user", "name", "safe")
182+
183+
with GitConfigParser(config_path, read_only=True) as git_config:
184+
self.assertEqual(git_config.get_value("user", "name"), "safe")
185+
153186
def test_base(self):
154187
path_repo = fixture_path("git_config")
155188
path_global = fixture_path("git_config_global")

0 commit comments

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