Merge pull request #2137 from gitpython-developers/fix-config-injection

reject control chars in written values in configuration

Author: Byron

git/config.py

@@ -882,6 +882,24 @@ def _value_to_string(self, value: Union[str, bytes, int, float, bool]) -> str:
             return str(value)
         return force_text(value)
 
+    def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> str:
+        value_str = self._value_to_string(value)
+        if re.search(r"[\r\n\x00]", value_str):
+            raise ValueError("Git config values must not contain CR, LF, or NUL")
+        return value_str
+
+    @needs_values
+    @set_dirty_and_flush_changes
+    def set(
+        self,
+        section: str,
+        option: str,
+        value: Union[str, bytes, int, float, bool, None] = None,
+    ) -> None:
+        if value is not None:
+            value = self._value_to_string_safe(value)
+        return super().set(section, option, value)
+
     @needs_values
     @set_dirty_and_flush_changes
     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
         :return:
             This instance
         """
+        value_str = self._value_to_string_safe(value)
         if not self.has_section(section):
             self.add_section(section)
-        self.set(section, option, self._value_to_string(value))
+        super().set(section, option, value_str)
         return self
 
     @needs_values
@@ -929,9 +948,10 @@ def add_value(self, section: str, option: str, value: Union[str, bytes, int, flo
         :return:
             This instance
         """
+        value_str = self._value_to_string_safe(value)
         if not self.has_section(section):
             self.add_section(section)
-        self._sections[section].add(option, self._value_to_string(value))
+        self._sections[section].add(option, value_str)
         return self
 
     def rename_section(self, section: str, new_name: str) -> "GitConfigParser":

test/test_config.py

@@ -150,6 +150,39 @@ def test_config_value_with_trailing_new_line(self):
         git_config = GitConfigParser(config_file)
         git_config.read()  # This should not throw an exception
 
+    @with_rw_directory
+    def test_set_value_rejects_config_injection(self, rw_dir):
+        config_path = osp.join(rw_dir, "config")
+        payload = "foo\n[core]\nhooksPath=/tmp/hooks"
+
+        with GitConfigParser(config_path, read_only=False) as git_config:
+            with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                git_config.set_value("user", "name", payload)
+
+        with GitConfigParser(config_path, read_only=True) as git_config:
+            self.assertFalse(git_config.has_section("user"))
+            self.assertFalse(git_config.has_section("core"))
+
+    @with_rw_directory
+    def test_set_and_add_value_reject_unsafe_value_characters(self, rw_dir):
+        config_path = osp.join(rw_dir, "config")
+        bad_values = ("foo\rbar", "foo\nbar", "foo\x00bar", b"foo\nbar")
+
+        with GitConfigParser(config_path, read_only=False) as git_config:
+            git_config.add_section("user")
+            for bad_value in bad_values:
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set("user", "name", bad_value)
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.set_value("user", "name", bad_value)
+                with pytest.raises(ValueError, match="CR, LF, or NUL"):
+                    git_config.add_value("user", "name", bad_value)
+
+            git_config.set_value("user", "name", "safe")
+
+        with GitConfigParser(config_path, read_only=True) as git_config:
+            self.assertEqual(git_config.get_value("user", "name"), "safe")
+
     def test_base(self):
         path_repo = fixture_path("git_config")
         path_global = fixture_path("git_config_global")